Re: [Qemu-devel] [PATCH for-2.12 3/8] monitor: new parameter "x-oob"

[Eric Blake]

On 03/26/2018 04:10 AM, Marc-André Lureau wrote:
> Hi
>
> On Mon, Mar 26, 2018 at 8:38 AM, Peter Xu <address@hidden> wrote:
> > Add new parameter to optionally enable Out-Of-Band for a QMP server.
> >
> > An example command line:
> >
> >    ./qemu-system-x86_64 -chardev stdio,id=char0 \
> >                         -mon chardev=char0,mode=control,x-oob=on
> >
> > By default, Out-Of-Band is off.
> >
> > It is not allowed if either MUX or non-QMP is detected, since
> > Out-Of-Band is currently only for QMP, and non-MUX chardev backends.

Worth documenting in the commit message at least that even when OOB is 
enabled, the client must STILL opt-in to using it by replying correctly 
to qmp_capabilities, as well as mention that in the future, we may 
remove x-oob and rely on JUST qmp_capabilities for enabling OOB.


> > @@ -4568,12 +4569,26 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
> >   void monitor_init(Chardev *chr, int flags)
> >   {
> >       Monitor *mon = g_malloc(sizeof(*mon));
> > +    bool use_readline = flags & MONITOR_USE_READLINE;
> > +    bool use_oob = flags & MONITOR_USE_OOB;
> > +
> > +    if (use_oob) {
> > +        if (CHARDEV_IS_MUX(chr)) {
> > +            error_report("Monitor Out-Of-Band is not supported with "
> > +                         "MUX typed chardev backend");
> > +            exit(1);
> > +        }
> > +        if (use_readline) {
> > +            error_report("Monitor Out-Of-band is only supported by QMP");
> > +            exit(1);

Should these two checks be swapped?  Otherwise, if you use a MUX-typed 
chardev for HMP, the message implies that switching chardev backend 
might make it work, even though if you actually do that you'd then get 
the failure for not being QMP.

> > +        }
> > +    }
>
> I would rather see the error reporting / exit in vl.c:mon_init_func()
> function, to have a single place for exit()

To do that, monitor_init() should change signatures to take Error 
**errp.  Probably worth doing if you spin a v2 of this series (adding 
the parameter can be done as a separate patch, although there are only 5 
callers in the tree so adjusting the callers at the same time is 
probably not that hard to review).

> Other than that, it looks fine.
> Reviewed-by: Marc-André Lureau <address@hidden>

Okay, I'll see how my review goes on the rest of the series before 
deciding whether to request a v2.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org