Re: [PATCH 4/4] qemu-storage-daemon: QAPIfy --chardev

[Eric Blake]

On 10/26/20 5:10 AM, Markus Armbruster wrote:
> From: Kevin Wolf <kwolf@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 <kwolf@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@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 <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org