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

[Jason Andryuk]

On Wed, Nov 13, 2019 at 8:18 AM Marc-André Lureau
<address@hidden> wrote:
>
> 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?

I was working on QMP over Xen's argo inter-vm communication for the
OpenXT project.  We had a lock up because we weren't properly dropping
QMP events in the chardev, even though we called CHR_EVENT_CLOSED.  We
fixed the argo chardev to drop messages like other chardevs, but my
investigation found QMP was still sending events even through it was
closed.

Xen's libxl opens a UNIX domain QMP socket for the duration of issuing
a command and then closes it.  OpenXT does the same over Argo.  In the
case of connecting and disconnecting, QMP events continue to be
generated after the first connection, even though there is nothing
connected for an unbounded length of time.  I was just trying to
optimize that scenario by skipping QMP events when disconnected.

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

Yes, I agree.

Thanks for your time looking at this.

Regards,
Jason