Re: [PATCH 6/6] qemu-storage-daemon: Use qmp_chardev_add() for --chardev

From: Markus Armbruster <armbru@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: marcandre.lureau@redhat.com, qemu-devel@nongnu.org,
	qemu-block@nongnu.org, pbonzini@redhat.com
Subject: Re: [PATCH 6/6] qemu-storage-daemon: Use qmp_chardev_add() for --chardev
Date: Mon, 26 Oct 2020 14:33:32 +0100	[thread overview]
Message-ID: <87wnzdnmc2.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20201023101222.250147-7-kwolf@redhat.com> (Kevin Wolf's message of "Fri, 23 Oct 2020 12:12:22 +0200")

Kevin Wolf <kwolf@redhat.com> writes:

> While this makes the code quite a bit longer and arguably isn't very
> elegant, it removes the dependency on QemuOpts from the --chardev option
> of the storage daemon.
>
> Going through qmp_chardev_add() already now ensures that semantics and
> accessible features won't change incompatibly once we QAPIfy it fully.
>
> Note that there are a few differences between the command line option
> -chardev in the system emulator and chardev-add in QMP. The --chardev
> option of qemu-storage-daemon will follow QMP in these respects:
>
> * All chardev types using ChardevHostdev accept a "path" option on the
>   command line, but QMP renamed it to "device"
>
> * ChardevSocket:
>
>   - Intentionally different defaults (documented as such): server=false
>     and wait=true (if server=true) on the command line, but server=true
>     and wait=false in QMP.
>
>   - Accidentally different defaults: tight=true on the command line, but
>     tight=false in QMP (this is a bug in commit 776b97d3).
>
>   - QMP has a nested SocketAddressLegacy field, whereas the command line
>     accepts "path"/"host"/"port"/"fd"/etc. on the top level.
>
>   - The command line has a "delay" option, but QMP has "nodelay"
>
> * ChardevUdp has two SocketAddressLegacy fields, the command line has
>   "host"/"port"/"localaddr"/"localport" on the top level with defaults
>   for values that are mandatory in SocketAddressLegacy

I found a few more differences when I picked this patch into my "[PATCH
0/4] qemu-storage-daemon: QAPIfy --chardev the stupid way" series.  I
worked them into the commit message there.  Please have a look and steal
the parts that are good.

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  storage-daemon/qemu-storage-daemon.c | 47 ++++++++++++++++++++++------
>  1 file changed, 38 insertions(+), 9 deletions(-)
>
> diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
> index e419ba9f19..b543c30951 100644
> --- a/storage-daemon/qemu-storage-daemon.c
> +++ b/storage-daemon/qemu-storage-daemon.c
> @@ -37,6 +37,7 @@
>  #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-control.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qstring.h"
> @@ -207,18 +208,46 @@ 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;
> +                ChardevBackend *chr_options;
> +                char *id;
> +                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, "type", &help, &error_fatal);
> +                if (help) {
> +                    if (qdict_haskey(args, "type")) {
> +                        /* TODO Print help based on the QAPI schema */

I phrased this as

                           /* FIXME wrong where QAPI differs from QemuOpts */

I think that's more accurate.

I plan to work on generating bare-bones help for use with keyval_parse()
from the QAPI schema.

> +                        qemu_opts_print_help(&qemu_chardev_opts, true);
> +                    } else {
> +                        qemu_chr_print_types();
> +                    }
>                      exit(EXIT_SUCCESS);
>                  }
> -                qemu_opts_del(opts);
> +
> +                /*
> +                 * TODO Flattened version of chardev-add in the QAPI schema
> +                 *
> +                 * While it's not there, parse 'id' manually from the QDict
> +                 * and treat everything else as the 'backend' option for
> +                 * chardev-add.
> +                 */

This is basically a manual flattening of chardev-add's implicit
arguments type.  Okay.

> +                id = g_strdup(qdict_get_try_str(args, "id"));
> +                if (!id) {
> +                    error_report("Parameter 'id' is missing");
> +                    exit(EXIT_FAILURE);
> +                }
> +                qdict_del(args, "id");
> +
> +                v = qobject_input_visitor_new_keyval(QOBJECT(args));
> +                visit_set_flat_simple_unions(v, true);
> +                visit_type_ChardevBackend(v, NULL, &chr_options, &error_fatal);
> +                visit_free(v);
> +
> +                qmp_chardev_add(id, chr_options, &error_fatal);
> +                qapi_free_ChardevBackend(chr_options);
> +                g_free(id);
> +                qobject_unref(args);
>                  break;
>              }
>          case OPTION_EXPORT:

Preferably with the commit message improved:
Reviewed-by: Markus Armbruster <armbru@redhat.com>