[Spice-devel] [vdagent-win v1 1/2] vdagent: small 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.
> 
> So this patch makes explicit if we are setting or receiving a
> _stop_event or not.
> ---
>  vdagent/vdagent.cpp | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> index 959881d..45ff55d 100644
> --- a/vdagent/vdagent.cpp
> +++ b/vdagent/vdagent.cpp
> @@ -480,11 +480,17 @@ 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;
>  
> +    events[0] = _control_event;
> +    if (_stop_event) {
> +        events[event_count] = _stop_event;
> +        event_count++;
> +    }
> +
>      wait_ret = MsgWaitForMultipleObjectsEx(event_count, events, timeout,
>      wake_mask, MWMO_ALERTABLE);
>      if (wait_ret == WAIT_OBJECT_0 + event_count) {
>          while (PeekMessage(&msg, NULL, 0, 0, PM_REMOVE)) {
> @@ -498,8 +504,13 @@ void VDAgent::event_dispatcher(DWORD timeout, DWORD
> wake_mask)
>          handle_control_event();
>          break;
>      case WAIT_OBJECT_0 + 1:
> -        _running = false;
> -        break;
> +        /* As _stop_event is not mandatory, the wait_ret index might refer
> to a
> +         * different event, we should explicit check it */
> +        if (_stop_event) {
> +            vd_printf("%s: received stop event", __func__);
> +            _running = false;
> +            break;
> +        }

I understand what you are trying to do (looking also at 2/2) but looks like
it's hard to maintain.

Why don't you do an additional enumeration for actions to do (or use
a function pointer). Something like

HANDLE events[2];
DWORD event_count = 1;
enum {
   CONTROL_ACTION,
   STOP_ACTION,
} actions[G_N_ELEMENTS(events)];
...

events[0] = _control_event;
actions[0] = CONTROL_ACTION;
if (_stop_event) {
    events[event_count] = _stop_event;
    actions[event_count] = STOP_ACTION;
    event_count++;
}

...
res_wait = 
wait_ret = MsgWaitForMultipleObjectsEx(event_count, events, timeout, wake_mask, MWMO_ALERTABLE);
...

if (wait_ret >= WAIT_OBJECT_0 && wait_ret < WAIT_OBJECT_0 + event_count) {
    switch (actions[wait_ret - WAIT_OBJECT_0]) {
    case CONTROL_ACTION:
       ...
    case STOP_ACTION:
       ...
...

Frediano