| Message ID | 20201112175905.404472-9-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | char: QAPIfy the command line parsing | expand |
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 Adding automagic "mux" creation to -chardev is wrong. 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. Paolo
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
On 13/11/20 14:20, Kevin Wolf wrote: >> -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. Yes, you're right. There's 2 CharBackends plugged into a single mux Chardev. > This is basically what I meant when I said your QOM conversion and my > QAPIfication work aren't conflicting (conceptually), but addressing > separate problems. Fair enough, I think I understand what you mean now. And I can't really argue with the diffstat and the usage of aliases to clean up dash vs. underscores. Paolo
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'] } ] } ## diff --git a/chardev/char.c b/chardev/char.c index a5d6be9dc8..3bb6a743f7 100644 --- a/chardev/char.c +++ b/chardev/char.c @@ -742,11 +742,6 @@ void qemu_chr_translate_legacy_options(QDict *args) /* name may refer to a QDict entry, so delete it only now */ qdict_del(args, "backend"); - - /* - * TODO: - * All backend types: "mux" - */ } Chardev *qemu_chr_new_noreplay(const char *label, const char *filename, @@ -1105,7 +1100,41 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend, Chardev *qemu_chr_new_cli(ChardevOptions *options, Error **errp) { - return chardev_new_qapi(options->id, options->backend, errp); + Chardev *chr; + char *bid = NULL; + + if (options->mux) { + bid = g_strdup_printf("%s-base", options->id); + } + + chr = chardev_new_qapi(bid ?: options->id, options->backend, errp); + if (!chr) { + goto out; + } + + if (options->mux) { + Chardev *mux; + ChardevMux mux_data = { + .chardev = bid, + }; + ChardevBackend backend = { + .type = CHARDEV_BACKEND_KIND_MUX, + .u.mux.data = &mux_data, + }; + + mux = qemu_chardev_new(options->id, TYPE_CHARDEV_MUX, &backend, NULL, + errp); + if (mux == NULL) { + object_unparent(OBJECT(chr)); + chr = NULL; + goto out; + } + chr = mux; + } + +out: + g_free(bid); + return chr; } ChardevOptions *qemu_chr_parse_cli_dict(QDict *args, bool help,
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(-)