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 <marcandre.lureau@redhat.com>
---
 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) {
-- 
2.20.0.rc1

Marc-André Lureau <marcandre.lureau@redhat.com> 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 <marcandre.lureau@redhat.com>
> ---
>  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?

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.

Hi

On Wed, Dec 5, 2018 at 12:43 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> 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 <marcandre.lureau@redhat.com>
> > ---
> >  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

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Wed, Dec 5, 2018 at 12:43 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@redhat.com> 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 <marcandre.lureau@redhat.com>
>> > ---
>> >  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.

If the pre-existing bug is just as bad as an assertion failure, then I'm
fine with turning it into an assertion failure, with a brief explanation
in the commit message.

>> 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.