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

[Victor Toso]

Hi,

On Wed, Aug 10, 2016 at 11:21:30AM -0400, Frediano Ziglio wrote:
> > 
> > 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.

Yes

> 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:
>        ...
> ...
>

Right, something like this can improve the switch there which makes
everything easier to read. I'll send a v2, many thanks!

  toso


> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel