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

[Markus Armbruster <armbru@redhat.com>]

Jason Andryuk <jandryuk@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 <jandryuk@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.