From: Markus Armbruster <armbru@redhat.com>
To: Eric Blake <eblake@redhat.com>
Cc: kwolf@redhat.com, marcandre.lureau@redhat.com,
qemu-devel@nongnu.org, qemu-block@nongnu.org,
pbonzini@redhat.com
Subject: Re: [PATCH 3/4] char: Flat alternative to overly nested chardev-add arguments
Date: Wed, 28 Oct 2020 08:33:02 +0100 [thread overview]
Message-ID: <87zh4624b5.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <df6a32e6-3b4c-d9d7-b473-9bdd7c674843@redhat.com> (Eric Blake's message of "Tue, 27 Oct 2020 13:23:13 -0500")
Eric Blake <eblake@redhat.com> writes:
> On 10/26/20 5:10 AM, Markus Armbruster wrote:
>> chardev-add's arguments use an annoying amount of nesting. Example:
>>
>> {"execute": "chardev-add",
>> "arguments": {
>> "id":"sock0",
>> "backend": {
>> "type": "socket",
>> "data": {
>> "addr": {
>> "type": "inet",
>> "data": {
>> "host": "0.0.0.0",
>> "port": "2445"}}}}}}
>>
>> This is because chardev-add predates QAPI features that enable flatter
>> data structures, both on the wire and in C: base types, flat unions,
>> commands taking a union or alternate as 'data'.
>>
>> The nesting would be even more annoying in dotted key syntax:
>>
>> id=sock0,\
>> backend.type=socket,\
>> backend.data.addr.type=inet,\
>> backend.data.addr.data.host=0.0.0.0,\
>> backend.data.addr.data.port=2445
>>
>> Relevant, because the next commit will QAPIfy qemu-storage-daemon
>> --chardev. We really want this instead:
>>
>> --chardev socket,id=sock0,\
>> addr.type=inet,\
>> addr.host=0.0.0.0,\
>> addr.port=2445
>>
>> To get it, define a new QAPI type ChardevOptions that is the flat
>> equivalent to chardev-add's arguments.
>>
>> What we should do now is convert the internal interfaces to take this
>> new type, and limit the nested old type to the external interface,
>> similar to what commit bd269ebc82 "sockets: Limit SocketAddressLegacy
>> to external interfaces" did. But we're too close to the freeze to
>> pull that off safely.
>>
>> What I can do now is convert the new type to the old nested type, and
>> promise to replace this by what should be done in the next development
>> cycle.
>
> Nice evaluation of the trade-off.
>
>>
>> In more detail:
>>
>> * Flat union ChardevOptions corresponds to chardev-add's implicit
>> arguments type. It flattens a struct containing a simple union into
>> a flat union.
>>
>> * The flat union's discriminator is named @backend, not @type. This
>> avoids clashing with member @type of ChardevSpiceChannel. For what
>> it's worth, -chardev also uses this name.
>>
>> * Its branches @socket, @udp use ChardevSocketFlat, ChardevUdpFlat
>> instead of ChardevSocket, ChardevUdp. This flattens simple union
>> SocketAddressLegacy members to flat union SocketAddress members.
>>
>> * New chardev_options_crumple() converts ChardevOptions to
>> chardev-add's implict arguments type.
>
> implicit
Yes.
>>
>> Only one existing QAPI definition is affected: some of ChardevSocket's
>> members get moved to a new base type ChardevSocketBase, to reduce
>> duplication. No change to the generated C type and the wire format.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> qapi/char.json | 106 ++++++++++++++++++++++++++++---
>> include/chardev/char.h | 5 ++
>> include/qemu/sockets.h | 3 +
>> chardev/char-legacy.c | 140 +++++++++++++++++++++++++++++++++++++++++
>> chardev/char-socket.c | 3 +-
>> util/qemu-sockets.c | 38 +++++++++++
>> chardev/meson.build | 1 +
>> 7 files changed, 287 insertions(+), 9 deletions(-)
>> create mode 100644 chardev/char-legacy.c
>
> Big but worth it. I'm liking the simplicity of this alternative over
> Kevin's proposal, especially if we're aiming to get this in 5.2 soft freeze.
Kevin and I decided not to. We both would've like to get a sane,
QAPIfied --chardev for qemu-storage-daemon in 5.2, but neither of the
proposed solutions inspires sufficient confidence to rush it in at this
point.
>> diff --git a/qapi/char.json b/qapi/char.json
>> index 43486d1daa..31b693bbb2 100644
>> --- a/qapi/char.json
>> +++ b/qapi/char.json
>> @@ -244,12 +244,8 @@
>> 'base': 'ChardevCommon' }
>>
>> ##
>> -# @ChardevSocket:
>> +# @ChardevSocketBase:
>> #
>> -# Configuration info for (stream) socket chardevs.
>> -#
>> -# @addr: socket address to listen on (server=true)
>> -# or connect to (server=false)
>> # @tls-creds: the ID of the TLS credentials object (since 2.6)
>> # @tls-authz: the ID of the QAuthZ authorization object against which
>> # the client's x509 distinguished name will be validated. This
>> @@ -274,9 +270,8 @@
>> #
>> # Since: 1.4
>> ##
>> -{ 'struct': 'ChardevSocket',
>> - 'data': { 'addr': 'SocketAddressLegacy',
>> - '*tls-creds': 'str',
>> +{ 'struct': 'ChardevSocketBase',
>> + 'data': { '*tls-creds': 'str',
>> '*tls-authz' : 'str',
>> '*server': 'bool',
>> '*wait': 'bool',
>> @@ -287,6 +282,35 @@
>> '*reconnect': 'int' },
>> 'base': 'ChardevCommon' }
>
> Here we are subdividing ChardevSocket into everything that is already
> flat, and excluding the awkward 'addr'...
>
>>
>> +##
>> +# @ChardevSocket:
>> +#
>> +# Configuration info for (stream) socket chardevs.
>> +#
>> +# @addr: socket address to listen on (server=true)
>> +# or connect to (server=false)
>> +#
>> +# Since: 1.4
>> +##
>> +{ 'struct': 'ChardevSocket',
>> + # Do not add to 'data', it breaks chardev_options_crumple()! Add to
>> + # ChardevSocketBase's 'data' instead.
>> + 'data': { 'addr': 'SocketAddressLegacy' },
>> + 'base': 'ChardevSocketBase' }
>
> ...legacy use pulls in the legacy 'addr'...
>
>> +
>> +##
>> +# @ChardevSocketFlat:
>> +#
>> +# Note: This type should eventually replace ChardevSocket. The
>> +# difference between the two: ChardevSocketFlat uses
>> +# SocketAddressLegacy, ChardevSocket uses SocketAddress.
>> +##
>
> Missing a 'Since: 5.2' tag, if you want one.
Yes.
>> +{ 'struct': 'ChardevSocketFlat',
>> + # Do not add to 'data', it breaks chardev_options_crumple()! Add to
>> + # ChardevSocketBase's 'data' instead.
>> + 'data': { 'addr': 'SocketAddress' },
>> + 'base': 'ChardevSocketBase' }
>> +
>
> ...and this is the new type with a saner 'addr'. Works for me so far.
Kevin dislikes the "Do not add to 'data'" part. It's needed because
chardev_options_crumple() open-codes the conversion of all local
members.
Vague idea: conversion visitor. Similar to clone, except you can hook a
conversion function into the cloning of certain members.
Perhaps less ambitious: somehow make the build fail when you add local
members without updating chardev_options_crumple().
Kevin also dislikes the schema duplication. His solution avoids it by
generating both flat and nested from the same schema. I doubt it's
worthwhile, because there ist just one QAPI type in need of it. A more
widely applicable evolution of the idea might be more useful, but also
still more complex.
Kevin's generator-based approach has another advantage: it enables
deprecation of the nested form. On the one hand, I'd love to get rid of
it. On the other hand, it's just syntactical cleanup, and whether
making the syntax neater and more consistent outweighs the pain of
changing things seems doubtful.
>> ##
>> # @ChardevUdp:
>> #
>> @@ -298,10 +322,26 @@
>> # Since: 1.5
>> ##
>> { 'struct': 'ChardevUdp',
>> + # Do not add to 'data', it breaks chardev_options_crumple()! Create
>> + # ChardevUdpBase instead, similar to ChardevSocketBase.
>> 'data': { 'remote': 'SocketAddressLegacy',
>> '*local': 'SocketAddressLegacy' },
>> 'base': 'ChardevCommon' }
>>
>> +##
>> +# @ChardevUdpFlat:
>> +#
>> +# Note: This type should eventually replace ChardevUdp. The
>> +# difference between the two: ChardevUdpFlat uses
>> +# SocketAddressLegacy, ChardevUdp uses SocketAddress.
>> +##
>
> Another missing 'Since: 5.2'
Yes.
>> +{ 'struct': 'ChardevUdpFlat',
>> + # Do not add to 'data', it breaks chardev_options_crumple()! Create
>> + # ChardevUdpBase instead, similar to ChardevSocketBase.
>> + 'data': { 'remote': 'SocketAddress',
>> + '*local': 'SocketAddress' },
>> + 'base': 'ChardevCommon' }
>> +
>> ##
>> # @ChardevMux:
>> #
>> @@ -422,6 +462,56 @@
>> # next one is just for compatibility
>> 'memory': 'ChardevRingbuf' } }
>>
>> +##
>> +# @ChardevBackendType:
>> +#
>> +# Since: 5.2
>> +##
>> +{ 'enum': 'ChardevBackendType',
>> +
>> + 'data': [ 'file', 'serial', 'parallel', 'pipe', 'socket', 'udp',
>> + 'pty', 'null', 'mux', 'msmouse', 'wctablet', 'braille',
>> + 'testdev', 'stdio', 'console', 'spicevmc', 'spiceport',
>> + 'vc', 'ringbuf' ] }
>> +
>> +##
>> +# @ChardevOptions:
>> +#
>> +# Note: This type should eventually replace the implicit arguments
>> +# type of chardev-add and chardev-chardev. The differences
>> +# between the two: 1. ChardevSocketOptions is a flat union
>> +# rather than a struct with a simple union member, and 2. it
>> +# uses SocketAddress instead of SocketAddressLegacy. This
>> +# avoids nesting on the wire, i.e. we need fewer {}.
>> +#
>> +# Since: 5.2
>> +##
>> +{ 'union': 'ChardevOptions',
>> + 'base': { 'backend': 'ChardevBackendType',
>> + 'id': 'str' },
>> + 'discriminator': 'backend',
>> + 'data': { 'file': 'ChardevFile',
>> + 'serial': 'ChardevHostdev',
>> + 'parallel': 'ChardevHostdev',
>> + 'pipe': 'ChardevHostdev',
>> + 'socket': 'ChardevSocketFlat',
>> + 'udp': 'ChardevUdpFlat',
>> + 'pty': 'ChardevCommon',
>> + 'null': 'ChardevCommon',
>> + 'mux': 'ChardevMux',
>> + 'msmouse': 'ChardevCommon',
>> + 'wctablet': 'ChardevCommon',
>> + 'braille': 'ChardevCommon',
>> + 'testdev': 'ChardevCommon',
>> + 'stdio': 'ChardevStdio',
>> + 'console': 'ChardevCommon',
>> + 'spicevmc': { 'type': 'ChardevSpiceChannel',
>> + 'if': 'defined(CONFIG_SPICE)' },
>> + 'spiceport': { 'type': 'ChardevSpicePort',
>> + 'if': 'defined(CONFIG_SPICE)' },
>> + 'vc': 'ChardevVC',
>> + 'ringbuf': 'ChardevRingbuf' } }
>
> Looks good from the QAPI point of view.
>
>
>> +/*
>> + * TODO Convert internal interfaces to ChardevOptions, replace this
>> + * function by one that flattens (const char *str, ChardevBackend
>> + * *backend) -> ChardevOptions.
>> + */
>> +q_obj_chardev_add_arg *chardev_options_crumple(ChardevOptions *chr)
>> +{
>> + q_obj_chardev_add_arg *arg;
>> + ChardevBackend *be;
>> +
>> + if (!chr) {
>> + return NULL;
>> + }
>> +
>> + arg = g_malloc(sizeof(*arg));
>> + arg->id = g_strdup(chr->id);
>> + arg->backend = be = g_malloc(sizeof(*be));
>> +
>> + switch (chr->backend) {
>> + case CHARDEV_BACKEND_TYPE_FILE:
>> + be->type = CHARDEV_BACKEND_KIND_FILE;
>> + be->u.file.data = QAPI_CLONE(ChardevFile, &chr->u.file);
>> + break;
>
> Most branches are straightforward,...
>
>> + case CHARDEV_BACKEND_TYPE_SOCKET:
>> + be->type = CHARDEV_BACKEND_KIND_SOCKET;
>> + /*
>> + * Clone with SocketAddress crumpled to SocketAddressLegacy.
>> + * All other members are in the base type.
>> + */
>> + be->u.socket.data = g_memdup(&chr->u.socket, sizeof(chr->u.socket));
>> + QAPI_CLONE_MEMBERS(ChardevSocketBase,
>> + qapi_ChardevSocket_base(be->u.socket.data),
>> + qapi_ChardevSocketFlat_base(&chr->u.socket));
>> + be->u.socket.data->addr = socket_address_crumple(chr->u.socket.addr);
>> + break;
>
> ...and this looks correct as well.
>
>> +++ b/chardev/char-socket.c
>> @@ -1404,7 +1404,8 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
>>
>> backend->type = CHARDEV_BACKEND_KIND_SOCKET;
>> sock = backend->u.socket.data = g_new0(ChardevSocket, 1);
>> - qemu_chr_parse_common(opts, qapi_ChardevSocket_base(sock));
>> + qemu_chr_parse_common(opts,
>> + qapi_ChardevSocketBase_base(qapi_ChardevSocket_base(sock)));
>
> The double function call (for a double cast) looks unusual, but I don't
> see any shorter expression, so it is fine.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks!
next prev parent reply other threads:[~2020-10-28 7:34 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-26 10:10 [PATCH 0/4] qemu-storage-daemon: QAPIfy --chardev the stupid way Markus Armbruster
2020-10-26 10:10 ` [PATCH 1/4] char/stdio: Fix QMP default for 'signal' Markus Armbruster
2020-10-26 10:10 ` [PATCH 2/4] char: Factor out qemu_chr_print_types() Markus Armbruster
2020-10-26 10:10 ` [PATCH 3/4] char: Flat alternative to overly nested chardev-add arguments Markus Armbruster
2020-10-27 18:23 ` Eric Blake
2020-10-28 7:33 ` Markus Armbruster [this message]
2020-10-26 10:10 ` [PATCH 4/4] qemu-storage-daemon: QAPIfy --chardev Markus Armbruster
2020-10-27 18:59 ` Eric Blake
2020-10-28 7:42 ` Markus Armbruster
2020-10-28 9:18 ` Markus Armbruster
2020-10-27 18:36 ` [PATCH 0/4] qemu-storage-daemon: QAPIfy --chardev the stupid way Paolo Bonzini
2020-10-28 7:01 ` Markus Armbruster
2020-10-28 11:46 ` Kevin Wolf
2020-10-28 14:39 ` Paolo Bonzini
2020-10-28 14:59 ` Kevin Wolf
2020-10-28 15:09 ` Paolo Bonzini
2020-10-28 15:39 ` Kevin Wolf
2020-10-28 16:01 ` Paolo Bonzini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87zh4624b5.fsf@dusky.pond.sub.org \
--to=armbru@redhat.com \
--cc=eblake@redhat.com \
--cc=kwolf@redhat.com \
--cc=marcandre.lureau@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.