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

[Markus Armbruster <armbru@redhat.com>]

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.