[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 6/6] qemu-storage-daemon: Use qmp_chardev_add() for --chardev
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 6/6] qemu-storage-daemon: Use qmp_chardev_add() for --chardev |
Date: |
Mon, 26 Oct 2020 14:33:32 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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>
- Re: [PATCH 2/6] char: Factor out qemu_chr_print_types(), (continued)
[PATCH 5/6] tests/qapi-schema: Flat representation of simple unions, Kevin Wolf, 2020/10/23
[PATCH 6/6] qemu-storage-daemon: Use qmp_chardev_add() for --chardev, Kevin Wolf, 2020/10/23
- Re: [PATCH 6/6] qemu-storage-daemon: Use qmp_chardev_add() for --chardev,
Markus Armbruster <=
[PATCH 4/6] qapi: Optionally parse simple unions as flat, Kevin Wolf, 2020/10/23
Re: [PATCH 0/6] qemu-storage-daemon: QAPIfy --chardev, Daniel P . Berrangé, 2020/10/23