Re: [PATCH] qmp: Reset mon->commands on CHR_EVENT_CLOSED

[Marc-André Lureau]

Hi

On Wed, Nov 13, 2019 at 4:41 PM Markus Armbruster <address@hidden> wrote:
>
> Jason Andryuk <address@hidden> writes:
>
> > Currently, mon->commands is uninitialized until CHR_EVENT_OPENED where
> > it is set to &qmp_cap_negotiation_commands.  After capability
> > negotiation, it is set to &qmp_commands.  If the chardev is closed,
> > CHR_EVENT_CLOSED, mon->commands remains as &qmp_commands.  Only once the
> > chardev is re-opened with CHR_EVENT_OPENED, is it reset to
> > &qmp_cap_negotiation_commands.
> >
> > monitor_qapi_event_emit compares mon->commands to
> > &qmp_cap_negotiation_commands, and skips sending events when they are
> > equal.
>
> The check's purpose is to ensure we don't send events in capabilities
> negotiation mode, i.e. between connect and a successful qmp_capabilities
> command.
>
> >         In the case of a closed chardev, QMP events are still sent down
> > to the closed chardev which needs to drop them.
>
> True, but I wonder how this can happen.  Can you explain?
>
> > Set mon->commands to &qmp_cap_negotiation_commands for CHR_EVENT_CLOSED
> > to stop sending events.  Setting for the CHR_EVENT_OPENED case remains
> > since that is how mon->commands is set for a newly opened connections.
> >
> > Signed-off-by: Jason Andryuk <address@hidden>
> > ---
> >  monitor/qmp.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/monitor/qmp.c b/monitor/qmp.c
> > index 9d9e5d8b27..5e2073c5eb 100644
> > --- a/monitor/qmp.c
> > +++ b/monitor/qmp.c
> > @@ -333,6 +333,7 @@ static void monitor_qmp_event(void *opaque, int event)
>        case CHR_EVENT_CLOSED:
>            /*
>             * Note: this is only useful when the output of the chardev
>             * backend is still open.  For example, when the backend is
>             * stdio, it's possible that stdout is still open when stdin
> >           * is closed.
> >           */
> >          monitor_qmp_cleanup_queues(mon);
> > +        mon->commands = &qmp_cap_negotiation_commands;
> >          json_message_parser_destroy(&mon->parser);
> >          json_message_parser_init(&mon->parser, handle_qmp_command,
> >                                   mon, NULL);
>
> Patchew reports this loses SHUTDOWN events.  Local testing confirms it
> does.  Simplified reproducer:
>
>     $ for ((i=0; i<100; i++)); do printf '{"execute": 
> "qmp_capabilities"}\n{"execute": "quit"}' | upstream-qemu -S -display none 
> -qmp stdio; done | grep -c SHUTDOWN
>     84
>
> In this test, the SHUTDOWN event was lost 16 out of 100 times.
>
> Its emission obviously races with the assignment you add.
>
> The comment preceding it provides a clue: after CHR_EVENT_CLOSED, we
> know the input direction is gone, and we duly clean up that part of the
> monitor.  But the output direction may or may not be gone, so we don't
> touch it.  Your assignment touches it.  This is not the correct spot.
> I can't tell you offhand where the correct spot is.  Perhaps Marc-André
> can.

The same "closed" event is used to signal read-disconnected,
write-disconnected and hup.

To implement half-closed signaling, we would need more events/state
(probably change the chardev API to use input and output streams).
Tbh, I am not sure it's really worth at this point, unless we have a
"visible" bug to fix.





-- 
Marc-André Lureau