On 10/26/20 5:10 AM, Markus Armbruster wrote:
> From: Kevin Wolf <kw...@redhat.com>
>
> This removes the dependency on QemuOpts from the --chardev option of
> the storage daemon.
>
> Help on option parameters is still wrong. Marked FIXME.
>
> There are quite a few differences between qemu-system-FOO -chardev,
> QMP chardev-add, and qemu-storage-daemon --chardev:
>
> * QMP chardev-add wraps arguments other than "id" in a "backend"
> object. Parameters other than "type" are further wrapped in a
> "data" object. Example:
>
> {"execute": "chardev-add",
> "arguments": {
> "id":"sock0",
> "backend": {
> "type": "socket",
> "data": {
> "addr": {
> "type": "inet",
> ...
> }}}}}
>
> qemu-system-FOO -chardev does not wrap. Neither does
> qemu-storage-daemon --chardev.
>
> * qemu-system-FOO -chardev parameter "backend" corresponds to QMP
> chardev-add "backend" member "type". qemu-storage-daemon names it
> "backend".
>
> * qemu-system-FOO -chardev parameter "backend" recognizes a few
> additional aliases for compatibility. QMP chardev-add does not.
> Neither does qemu-storage-daemon --chardev.
>
> * qemu-system-FOO -chardev' with types "serial", "parallel" and "pipe"
> parameter "path" corresponds to QMP chardev-add member "device".
> qemu-storage-daemon --chardev follows QMP.
>
> * Backend type "socket":
>
> - Intentionally different defaults (documented as such):
> qemu-system-FOO -chardev defaults to server=false and
> wait=true (if server=true), but QMP chardev-add defaults to
> server=true and wait=false. qemu-storage-daemon --chardev follows
> QMP.
>
> - Accidentally different defaults: qemu-system-FOO -chardev defaults
> to tight=true, QMP chardev-add defaults to tight=false in
> QMP (this is a bug in commit 776b97d3). qemu-storage-daemon
> follows QMP.
Should we be fixing that bug for 5.2?
>
> - QMP chardev-add wraps socket address arguments "path", "host",
> "port", etc in a "data" object. qemu-system-FOO -chardev does not
> wrap. Neither does qemu-storage-daemon --chardev.
>
> - qemu-system-FOO -chardev parameter "delay" corresponds to QMP
> chardev-add member "nodelay" with the sense reversed.
> qemu-storage-daemon --chardev follows QMP.
>
> * Backend type "udp":
>
> - QMP chardev-add wraps remote and local address arguments in a
> "remote" and a "local" object, respectively. qemu-system-FOO
> -chardev does not wrap, but prefixes the local address parameter
> names with "local" instead.
>
> - QMP chardev-add wraps socket address arguments in a "data" object.
> qemu-system-FOO -chardev does not wrap. Neither does
> qemu-storage-daemon --chardev. Same as for type "socket".
>
> * I'm not sure qemu-system-FOO -chardev supports everything QMP
> chardev-add does. I am sure qemu-storage-daemon --chardev does.
Quite the list, but it is a good start for what remains to merge things
in the correct direction for 6.0.
>
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
> ---
> storage-daemon/qemu-storage-daemon.c | 37 +++++++++++++++++++++-------
> 1 file changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/storage-daemon/qemu-storage-daemon.c
> b/storage-daemon/qemu-storage-daemon.c
> index e419ba9f19..f1f3bdc320 100644
> --- a/storage-daemon/qemu-storage-daemon.c
> +++ b/storage-daemon/qemu-storage-daemon.c
> @@ -37,10 +37,13 @@
> #include "qapi/error.h"
> #include "qapi/qapi-visit-block-core.h"
> #include "qapi/qapi-visit-block-export.h"
> +#include "qapi/qapi-visit-char.h"
> +#include "qapi/qapi-visit-char.h"
Duplicate.
> #include "qapi/qapi-visit-control.h"
> #include "qapi/qmp/qdict.h"
> #include "qapi/qmp/qstring.h"
> #include "qapi/qobject-input-visitor.h"
> +#include "qapi/qobject-output-visitor.h"
>
> #include "qemu-common.h"
> #include "qemu-version.h"
> @@ -207,18 +210,34 @@ static void process_options(int argc, char *argv[])
> }
> case OPTION_CHARDEV:
> {
> - /* TODO This interface is not stable until we QAPIfy it */
> - QemuOpts *opts = qemu_opts_parse_noisily(&qemu_chardev_opts,
> - optarg, true);
> - if (opts == NULL) {
> - exit(EXIT_FAILURE);
> - }
> + QDict *args;
> + Visitor *v;
> + ChardevOptions *chr;
> + q_obj_chardev_add_arg *arg;
> + bool help;
>
> - if (!qemu_chr_new_from_opts(opts, NULL, &error_fatal)) {
> - /* No error, but NULL returned means help was printed */
> + args = keyval_parse(optarg, "backend", &help, &error_fatal);
> + if (help) {
> + if (qdict_haskey(args, "backend")) {
> + /* FIXME wrong where QAPI differs from QemuOpts */
> + qemu_opts_print_help(&qemu_chardev_opts, true);
> + } else {
> + qemu_chr_print_types();
> + }
> exit(EXIT_SUCCESS);
> }
> - qemu_opts_del(opts);
> +
> + v = qobject_input_visitor_new_keyval(QOBJECT(args));
> + visit_type_ChardevOptions(v, NULL, &chr, &error_fatal);
> + visit_free(v);
> +
> + arg = chardev_options_crumple(chr);
> +
> + qmp_chardev_add(arg->id, arg->backend, &error_fatal);
> + g_free(arg->id);
> + qapi_free_ChardevBackend(arg->backend);
> + qapi_free_ChardevOptions(chr);
> + qobject_unref(args);
> break;
> }
> case OPTION_EXPORT:
>
Reviewed-by: Eric Blake <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
