Re: [PATCH 08/13] char: Add mux option to ChardevOptions

[Kevin Wolf]

Am 13.11.2020 um 12:50 hat Paolo Bonzini geschrieben:
> On 12/11/20 18:59, Kevin Wolf wrote:
> > The final missing piece to achieve compatibility between
> > qemu_chr_parse_cli_str()/qemu_chr_new_cli() and the legacy command line
> > is support for the 'mux' option. Implement it.
> > 
> > Signed-off-by: Kevin Wolf<kwolf@redhat.com>
> > ---
> >   qapi/char.json |  4 +++-
> >   chardev/char.c | 41 +++++++++++++++++++++++++++++++++++------
> >   2 files changed, 38 insertions(+), 7 deletions(-)
> > 
> > diff --git a/qapi/char.json b/qapi/char.json
> > index e1f9347044..d6733a5473 100644
> > --- a/qapi/char.json
> > +++ b/qapi/char.json
> > @@ -453,12 +453,14 @@
> >   #
> >   # @id: the chardev's ID, must be unique
> >   # @backend: backend type and parameters
> > +# @mux: enable multiplexing mode (default: false)
> >   #
> >   # Since: 6.0
> >   ##
> >   { 'struct': 'ChardevOptions',
> >     'data': { 'id': 'str',
> > -            'backend': 'ChardevBackend' },
> > +            'backend': 'ChardevBackend',
> > +            '*mux': 'bool' },
> >     'aliases': [ { 'source': ['backend'] } ] }
> 
> I think this shows that -chardev and chardev-add are both kind of
> unrepairable.  The right way to do a mux with a serial and monitor on
> top should be explicit:
> 
>             stdio
>                ↑
>          mux-controller
>           ↑        ↑
>          mux      mux
>           ↑        ↑
>        serial   monitor
> 
>       -object chardev-stdio,id=stdio
>       -object chardev-mux-controller,id=mux,backend=stdio
>       -object chardev-mux,id=serial-chardev,controller=mux
>       -object chardev-mux,id=mon-chardev,controller=mux
>       -serial chardev:serial-chardev
>       -monitor chardev:mon-chardev

I don't think these "mux" chardevs plugged to a "mux-controller"
actually exist, at least today. You can directly plug serial and monitor
to a mux chardev that sits on top of stdio.

This is the current syntax for explicitly configuring things:

    -chardev stdio,id=stdio
    -chardev mux,chardev=stdio,id=mux
    -mon chardev=mux
    -serial chardev:mux

And of course this is still possible after my series, and it is what
management tools should be using.

> Adding automagic "mux" creation to -chardev is wrong.

I'm not really adding it, it's already there.

While the code is temporarily duplicated and it looks like an addition
here, at the end of the series it's effectively just some preexisting
code moved (and QAPIfied) from qemu_chr_new_from_opts().

Of course, we can deprecate it, but I don't think it's really in the way
because it just desugars to two normal chardev definitions. On the other
hand, I've never used it (apart from testing this patch), so I don't
really care in practice if it exists or not.

> I don't entirely object to the series since it's quite nice, but I see
> it as more of a cleanup than the final stage.  It hinges on what
> Markus thinks of aliases, I guess.

Yes, I completely agree that this is mostly a cleanup. Most QAPIfication
actually is, because QemuOpts and hand written parsers do work and we
have been successfully using them for years. They just work in less
consistent ways and take a bit more code.

I never said it has to be the final stage, but I think whatever the
final stage is, having external interfaces that are consistent and use
the QAPI schema as the one source of truth will be helpful.

This is basically what I meant when I said your QOM conversion and my
QAPIfication work aren't conflicting (conceptually), but addressing
separate problems.

Kevin