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
>
> 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 <arm...@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.
>
> 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.
> +{ '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.
> ##
> # @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'
> +{ '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 <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
