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

[Jason Andryuk]

On Wed, Nov 6, 2019 at 9:53 AM Marc-André Lureau
<address@hidden> wrote:
>
> Hi
>
> On Wed, Nov 6, 2019 at 5:04 PM Jason Andryuk <address@hidden> wrote:
> >
> > 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.  In the case of a closed chardev, QMP events are still sent down
> > to the closed chardev which needs to drop them.
>
> This is a minor improvement, not really a bug fix or do I read that 
> incorrectly?

Yes, it is more of a minor improvement since disconnected chardevs
already drop the QMP events.  This will just stop generating them in
the first place.

> >
> > 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)
> >           * 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);
> > --
> > 2.21.0
> >
> >
>
> Looks good to me,
> Reviewed-by: Marc-André Lureau <address@hidden>

Thank you.

-Jason