Skip to content

multiple fixes for win7 #206

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Sep 16, 2017
Merged

Conversation

bingbing8
Copy link

@bingbing8 bingbing8 commented Sep 13, 2017

  1. fix some exception when appverifier is enabled on win7 (https://gitthub.com/PowerShell/Win32-OpenSSH/issues/872)
  2. enable sshdconfig tests on win7(win7: sshd_config E2E tests failed on win7 Win32-OpenSSH#873)
  3. fix for Win7: ssh session does not return until hit ENTER Win32-OpenSSH#874 ( ReadFile does not return on win7 when no content in console )
  4. Remove logging to console in Readthread because write hangs here since write thread already closed (win10: ssh.exe hangs when -v option is supplied. Win32-OpenSSH#879)
  5. fix VCTargetsPath

…/Win32-OpenSSH#872)

2. enable sshdconfig tests on win7(PowerShell/Win32-OpenSSH#873)
3. fix for PowerShell/Win32-OpenSSH#874 does not return on win7 when no content in console )
4. remove loggin to console in Readthread because write hangs here since write thread already closed (PowerShell/Win32-OpenSSH#879)
5. fix VCTargetsPath
@@ -247,6 +245,19 @@ syncio_initiate_write(struct w32_io* pio, DWORD num_bytes)
return 0;
}

static BOOL

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

declare it in misc_internals.h and define in misc.c so that we can reuse..

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to use IsWindows8OrGreater()

@@ -247,6 +245,19 @@ syncio_initiate_write(struct w32_io* pio, DWORD num_bytes)
return 0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the log in the if (0 == QueueUserAPC(ReadAPCProc...) { ... }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

* UF_UNIX sockets that are not connected yet
*/
}
else if (SetHandleInformation(h, HANDLE_FLAG_INHERIT, shi_flags) == FALSE) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if ((!IS_INVALID_HANDLE(h)) && SetHandleInformation(h, HANDLE_FLAG_INHERIT, shi_flags) == FALSE)

Copy link
Author

@bingbing8 bingbing8 Sep 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

ZeroMemory(&osvi, sizeof(OSVERSIONINFO));
osvi.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);

GetVersionEx(&osvi);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per MSDN,
[GetVersionEx may be altered or unavailable for releases after Windows 8.1. Instead, use the Version Helper functions]

we have a simpler function IsWindows8OrGreater()
https://msdn.microsoft.com/en-us/library/windows/desktop/dn424961(v=vs.85).aspx

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to use IsWindows8OrGreater()

* UF_UNIX sockets that are not connected yet
*/
if (!IS_INVALID_HANDLE(h) && (SetHandleInformation(h, HANDLE_FLAG_INHERIT, shi_flags) == FALSE)) {
debug3("fcntl - SetHandleInformation failed %d, io:%p",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error:%d

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link

@manojampalam manojampalam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-blocking comments inline...

@@ -103,12 +103,11 @@ ReadThread(_In_ LPVOID lpParameter)

if (!SetConsoleMode(GetStdHandle(STD_INPUT_HANDLE), dwAttributes))
error("SetConsoleMode on STD_INPUT_HANDLE failed with %d\n", GetLastError());
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert this unwanted change

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -257,7 +254,7 @@ syncio_close(struct w32_io* pio)
/* If io is pending, let worker threads exit. */
if (pio->read_details.pending) {
/* For console - the read thread is blocked so terminate it. */
if (FILETYPE(pio) == FILE_TYPE_CHAR && in_raw_mode)
if (FILETYPE(pio) == FILE_TYPE_CHAR && (!IsWindows8OrGreater() || in_raw_mode))
Copy link

@manojampalam manojampalam Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recommend adding a MACRO like this for readability
#define IsWin7OrLess() (!IsWindows8OrGreater())

Also add detailed comments on why we are adding this condition.

* Ignore if handle is not valid yet. It will not be valid for
* UF_UNIX sockets that are not connected yet
*/
if (!IS_INVALID_HANDLE(h) && (SetHandleInformation(h, HANDLE_FLAG_INHERIT, shi_flags) == FALSE)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend adding a MACRO like this for readability
#define IS_VALID_HANDLE(h) (!IS_INVALID_HANDLE(h))

@manojampalam manojampalam merged commit 18b1e59 into latestw_all Sep 16, 2017
@manojampalam manojampalam deleted the win7appverifierexceptions branch September 16, 2017 04:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants