Re: [Qemu-devel] [PATCH v3 4/6] monitor: check if chardev can switch gcontext for OOB

[Marc-André Lureau]

Hi

On Wed, Dec 5, 2018 at 12:43 PM Markus Armbruster <address@hidden> wrote:
>
> Marc-André Lureau <address@hidden> writes:
>
> > Not all backends are able to switch gcontext. Those backends cannot
> > drive a OOB monitor (the monitor would then be blocking on main
> > thread).
> >
> > For example, ringbuf, spice, or more esoteric input chardevs like
> > braille or MUX.
> >
> > We currently forbid MUX because not all frontends are ready to run
> > outside main loop. Extend to add a context-switching feature check.
> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  monitor.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 79afe99079..25cf4223e8 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4562,9 +4562,11 @@ void monitor_init(Chardev *chr, int flags)
> >      bool use_oob = flags & MONITOR_USE_OOB;
> >
> >      if (use_oob) {
> > -        if (CHARDEV_IS_MUX(chr)) {
> > +        if (CHARDEV_IS_MUX(chr) ||
> > +            !qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT)) {
> >              error_report("Monitor out-of-band is not supported with "
> > -                         "MUX typed chardev backend");
> > +                         "%s typed chardev backend",
> > +                         object_get_typename(OBJECT(chr)));
> >              exit(1);
> >          }
> >          if (use_readline) {
>
> Aha, this answers my question on the previous patch: yes, it is possible
> to trip the new assertion.
>
> Are there any ways other than this one?

Good question, if there are, we have latent bugs if switching gcontext
on a non-capable chardev.

Doing a quick review of qemu_chr_fe_set_handlers() for context != NULL
reveals net/colo-compare.c:

I think we should have the capability check added to find_and_check_chardev().

I don't see other candidates. Considering the problem is pre-existing,
I can either update the patch or add a follow-up patch.

>
> We could squash the two patches.  But I figure you kept the previous
> patch separate on purpose.  That's okay, but it should mention the
> assertion can be tripped, and the next patch (this one) will fix it.
>


-- 
Marc-André Lureau