[Spice-devel] [vdagent-win v3 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()
> 
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> Acked-by: Frediano Ziglio <fziglio at redhat.com>
> ---
> 
> Suggestions taken from Frediano;
> The diff from previous patch:
> https://paste.fedoraproject.org/406316/47093493/
> 
> ---
>  vdagent/vdagent.cpp | 39 ++++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> index c67f30c..8e7ba2b 100644
> --- a/vdagent/vdagent.cpp
> +++ b/vdagent/vdagent.cpp
> @@ -20,6 +20,7 @@
>  #include "display_setting.h"
>  #include "file_xfer.h"
>  #include "ximage.h"
> +#include "spice-protocol/spice/macros.h"

Looking at Makefile.am and other sources this should work
and be more consistent:

#include <spice/macros.h>

>  #undef max
>  #undef min
>  #include <wtsapi32.h>
> @@ -480,10 +481,22 @@ void VDAgent::input_desktop_message_loop()
>  
>  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;
>      MSG msg;
> +    enum {
> +        CONTROL_ACTION,
> +        STOP_ACTION,
> +    } actions[SPICE_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 +505,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 (%d)", __func__, action);
>          _running = false;
>      }
>  }
> @@ -906,8 +925,6 @@ bool VDAgent::handle_max_clipboard(VDAgentMaxClipboard
> *msg, uint32_t size)
>      return true;
>  }
>  
> -#define MIN(a, b) ((a) > (b) ? (b) : (a))
> -
>  bool VDAgent::write_clipboard(VDAgentMessage* msg, uint32_t size)
>  {
>      uint32_t pos = 0;

Beside that patch is fine.

Frediano