Hi
On Wed, Nov 13, 2019 at 4:41 PM Markus Armbruster <arm...@redhat.com> wrote:
>
> Jason Andryuk <jandr...@gmail.com> 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 <jandr...@gmail.com>
> > ---
> > 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
