[Spice-devel] [vdagent-win v2 1/2] vdagent: rework on event_dispatcher

[Frediano Ziglio]

> 
> As _stop_event is not mandatory, we are initializing the events array
> with something that might be NULL. The problem is with the following
> patch as I'm introducing a new non mandatory event and logic starts to
> get a bit hard to follow.
> 
> So this patch makes explicit if we are setting or receiving a
> _stop_event or not and uses an auxiliary enum and array 'actions' to
> work around this non mandatory events and the return value of
> MsgWaitForMultipleObjectsEx()
> ---
>  vdagent/vdagent.cpp | 39 ++++++++++++++++++++++++++++++---------
>  1 file changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> index 959881d..d3cbd5f 100644
> --- a/vdagent/vdagent.cpp
> +++ b/vdagent/vdagent.cpp
> @@ -478,12 +478,27 @@ void VDAgent::input_desktop_message_loop()
>      CloseDesktop(hdesk);
>  }
>  
> +#define G_N_ELEMENTS(arr)              (sizeof (arr) / sizeof ((arr)[0]))
> +

Oh.. you can use SPICE_N_ELEMENTS, should be available

>  void VDAgent::event_dispatcher(DWORD timeout, DWORD wake_mask)
>  {
> -    HANDLE events[] = {_control_event, _stop_event};
> -    DWORD event_count = _stop_event ? 2 : 1;
> +    HANDLE events[2];
> +    DWORD event_count = 1;
>      DWORD wait_ret;
> +    DWORD action;

action should be of enumerator type ...

>      MSG msg;
> +    enum {
> +        CONTROL_ACTION,
> +        STOP_ACTION,
> +    } actions[G_N_ELEMENTS(events)];

... I would put here as

} actions[G_N_ELEMENTS(events)], action;

> +
> +    events[0] = _control_event;
> +    actions[0] = CONTROL_ACTION;
> +    if (_stop_event) {
> +        events[event_count] = _stop_event;
> +        actions[event_count] = STOP_ACTION;
> +        event_count++;
> +    }
>  
>      wait_ret = MsgWaitForMultipleObjectsEx(event_count, events, timeout,
>      wake_mask, MWMO_ALERTABLE);
>      if (wait_ret == WAIT_OBJECT_0 + event_count) {
> @@ -492,19 +507,25 @@ void VDAgent::event_dispatcher(DWORD timeout, DWORD
> wake_mask)
>              DispatchMessage(&msg);
>          }
>          return;
> +    } else if (wait_ret == WAIT_IO_COMPLETION || wait_ret == WAIT_TIMEOUT) {
> +        return;
> +    } else if (wait_ret < WAIT_OBJECT_0 || wait_ret > WAIT_OBJECT_0 +
> event_count) {
> +        vd_printf("MsgWaitForMultipleObjectsEx failed: %lu %lu", wait_ret,
> GetLastError());
> +        _running = false;
> +        return;
>      }
> -    switch (wait_ret) {
> -    case WAIT_OBJECT_0:
> +
> +    action = actions[wait_ret - WAIT_OBJECT_0];
> +    switch (action) {
> +    case CONTROL_ACTION:
>          handle_control_event();
>          break;
> -    case WAIT_OBJECT_0 + 1:
> +    case STOP_ACTION:
> +        vd_printf("%s: received stop event", __func__);
>          _running = false;
>          break;
> -    case WAIT_IO_COMPLETION:
> -    case WAIT_TIMEOUT:
> -        break;
>      default:
> -        vd_printf("MsgWaitForMultipleObjectsEx failed: %lu %lu", wait_ret,
> GetLastError());
> +        vd_printf("%s: action not handled (%lu)", __func__, action);
>          _running = false;
>      }
>  }

Beside these small changes looks fine.

Acked-by: Frediano Ziglio <fziglio at redhat.com>

Frediano