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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel