Re: [PATCH] server/console: Allow waiting on console_input

[Sebastian Lackner]

On 16.08.2016 20:47, Keno Fischer wrote:
> Please see attached. This fixes console input under wine for julia and
> probably other programs using libuv. I am not particularly familiar
> with wine internals, so please review closely.
>
> Thanks,
> Keno
>
> 0001-server-console-Allow-waiting-on-console_input.patch
>
> From 5fe78027f0034b83145c879adb17531e9487c6f7 Mon Sep 17 00:00:00 2001
> From: Keno Fischer 
> Date: Tue, 16 Aug 2016 14:35:27 -0400
> Subject: [PATCH] server/console: Allow waiting on console_input
>
> It appears that Windows allows this for notification of new input
> events in the console. This functionality was used e.g., by the libuv
> I/O library, causing any users thereof to not receive input under wine,
> e.g. fixes https://github.com/JuliaLang/julia/issues/18006.

Thanks for working on this. I think the implementation is fine, however there
are some small things which could be improved a bit.
> Signed-off-by: Keno Fischer
>
> dlls/kernel32/tests/console.c | 37 +++++++++++++++++++++++++++++++++++++
>  server/console.c              | 16 +++++++++++++---
>  2 files changed, 50 insertions(+), 3 deletions(-)
>
> diff --git a/dlls/kernel32/tests/console.c b/dlls/kernel32/tests/console.c
> index 5b66d9f..f97b415 100644
> --- a/dlls/kernel32/tests/console.c
> +++ b/dlls/kernel32/tests/console.c
> @@ -908,6 +908,41 @@ static void testScreenBuffer(HANDLE hConOut)
>      SetConsoleOutputCP(oldcp);
>  }
>
> +static void CALLBACK signaled_function(PVOID p, BOOLEAN TimerOrWaitFired)
> +{

Usually wine prefers "void *" instead of PVOID and also lowercase variable names,
so for example just "timeout" instead of "TimerOrWaitFired" as on MSDN.
> - HANDLE event = p;
> - SetEvent(event);
> - ok(!TimerOrWaitFired, "wait shouldn't have timed out\n");
>
> +}
> -
>
> +static void testWaitForConsoleInput(HANDLE input_handle)
> +{
> - HANDLE wait_handle;
> - HANDLE complete_event;
> - INPUT_RECORD record;
> - DWORD events_written;
> - DWORD wait_ret;
> - BOOL ret;
> -
> - complete_event = CreateEventW(NULL, FALSE, FALSE, NULL);
> -
> - ret = RegisterWaitForSingleObject(&wait_handle, input_handle, signaled_function, complete_event, INFINITE, WT_EXECUTEONLYONCE);
> - ok(ret == TRUE, "Expected RegisterWaitForSingleObject to return TRUE, got %d\n", ret);

To keep the testcase similar to the original code thats probably fine, however I
would like to point out that you do not really need an asynchronous wait here.
You could basically test the same with WaitForSingleObject(input_handle, INFINITE).
> - /* give worker thread a chance to start up */
> - Sleep(100);
> - record.EventType = KEY_EVENT;
> - record.Event.KeyEvent.bKeyDown = 1;
> - record.Event.KeyEvent.wRepeatCount = 1;
> - record.Event.KeyEvent.wVirtualKeyCode = VK_RETURN;
> - record.Event.KeyEvent.wVirtualScanCode = VK_RETURN;
> - record.Event.KeyEvent.uChar.AsciiChar = '\r';

If I don't miss anything this does not initialize the upper byte of the union.
> - record.Event.KeyEvent.dwControlKeyState = 0;
> - ret = WriteConsoleInputW(input_handle, &record, 1, &events_written);
> - ok(ret == TRUE, "Expected WriteConsoleInputW to return TRUE, got %d\n", ret);
> - wait_ret = WaitForSingleObject(complete_event, INFINITE);
> - ok(wait_ret == WAIT_OBJECT_0, "Expected the handle to be signaled");

There is a linebreak missing in the line above. For completeness, it also wouldn't
hurt to add another test after calling FlushConsoleInputBuffer() to make sure it
times out as expected.

If you want to keep the asynchronous wait, please also add cleanup of the
complete_event and the wait_handle.
> +}
> -
>
> static void test_GetSetConsoleInputExeName(void)
>  {
>      BOOL ret;
> @@ -3044,6 +3079,8 @@ START_TEST(console)
>      testScroll(hConOut, sbi.dwSize);
>      /* will test sb creation / modification / codepage handling */
>      testScreenBuffer(hConOut);
> -
>
> /* Test waiting for a console handle */
> -
>
> testWaitForConsoleInput(hConIn);
>
> /* clear duplicated console font table */
> CloseHandle(hConIn);
>
> diff --git a/server/console.c b/server/console.c
> index 93f785a..376395d 100644
> --- a/server/console.c
> +++ b/server/console.c
> @@ -67,6 +67,7 @@ struct console_input
>
> static void console_input_dump( struct object *obj, int verbose );
>  static void console_input_destroy( struct object *obj );
> +static int console_input_signaled( struct object *obj, struct wait_queue_entry *entry );
>  static struct fd *console_input_get_fd( struct object *obj );
>
> static const struct object_ops console_input_ops =
> @@ -74,9 +75,9 @@ static const struct object_ops console_input_ops =
>      sizeof(struct console_input),     /* size */
>      console_input_dump,               /* dump */
>      no_get_type,                      /* get_type */
> - no_add_queue,                     /* add_queue */
> - NULL,                             /* remove_queue */
> - NULL,                             /* signaled */
> - add_queue,                        /* add_queue */
> - remove_queue,                     /* remove_queue */
> - console_input_signaled,           /* signaled */
> no_satisfied,                     /* satisfied */
> no_signal,                        /* signal */
> console_input_get_fd,             /* get_fd */
>
> @@ -543,6 +544,14 @@ static struct console_input* console_input_get( obj_handle_t handle, unsigned ac
>      return console;
>  }
>
> +/* the console input is signaled when it's not empty */
> +static int console_input_signaled( struct object *obj, struct wait_queue_entry *entry )
> +{
> - struct console_input *console = (struct console_input *)obj;
> - assert( obj->ops == &console_input_ops );
> - return (console->recnum != 0);
>
> +}
> -
>
> struct console_signal_info
>  {
>      struct console_input        *console;
> @@ -677,6 +686,7 @@ static int write_console_input( struct console_input* console, int count,
>      }
>      if (!console->recnum && count) set_event( console->event );
>      console->recnum += count;
> - wake_up( &console->obj, 0 );
> return count;
>
> }
>
> -- 2.7.4

[PATCH] server/console: Allow waiting on console_input

[Keno Fischer]

> To keep the testcase similar to the original code thats probably fine, however I
> would like to point out that you do not really need an asynchronous wait here.
> You could basically test the same with WaitForSingleObject(input_handle, INFINITE).

I wanted to make sure to keep the ordering of events the same as in
the original, which is WaitForSingleEvent->WriteConsoleInput. It seems
like reversing the order here might go down a different code path,
though perhaps here's not the place to test that, since that's handled
in the generic waiting code.
> > - /* give worker thread a chance to start up */
> > - Sleep(100);
> > - record.EventType = KEY_EVENT;
> > - record.Event.KeyEvent.bKeyDown = 1;
> > - record.Event.KeyEvent.wRepeatCount = 1;
> > - record.Event.KeyEvent.wVirtualKeyCode = VK_RETURN;
> > - record.Event.KeyEvent.wVirtualScanCode = VK_RETURN;
> > - record.Event.KeyEvent.uChar.AsciiChar = '\r';
>
> If I don't miss anything this does not initialize the upper byte of the union.

Yes you're correct, since I'm calling WriteConsoleInputW, I should be
using the unicode variant.
> > - record.Event.KeyEvent.dwControlKeyState = 0;
> > - ret = WriteConsoleInputW(input_handle, &record, 1, &events_written);
> > - ok(ret == TRUE, "Expected WriteConsoleInputW to return TRUE, got %d\n", ret);
> > - wait_ret = WaitForSingleObject(complete_event, INFINITE);
> > - ok(wait_ret == WAIT_OBJECT_0, "Expected the handle to be signaled");
>
> There is a linebreak missing in the line above. For completeness, it also wouldn't
> hurt to add another test after calling FlushConsoleInputBuffer() to make sure it
> times out as expected.

Sounds good.