* [PATCH 0/4] qemu-storage-daemon: QAPIfy --chardev the stupid way @ 2020-10-26 10:10 Markus Armbruster 2020-10-26 10:10 ` [PATCH 1/4] char/stdio: Fix QMP default for 'signal' Markus Armbruster ` (4 more replies) 0 siblings, 5 replies; 18+ messages in thread From: Markus Armbruster @ 2020-10-26 10:10 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, marcandre.lureau, qemu-block, pbonzini Kevin's "[PATCH v2 0/6] qemu-storage-daemon: QAPIfy --chardev" involves surgery to the QAPI generator. Some (most?) of it should go away if we deprecate the "data" wrappers due to simple unions in QMP. Do we really need to mess with the code generator to solve the problem at hand? Let's recapitulate the problem: * We want to QAPIfy --chardev, i.e. define its argument as a QAPI type. * We want --chardev to use the same internal interface as chardev-add. * The obvious way is to reuse chardev-add's arguments type for --chardev. We don't want to, because it's excessively nested: it's a struct containing a simple union, with more simple unions inside. The result would be awful like this: --chardev 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 Kevin's series solves this as follows: 1. Internal flat representation: improve the QAPI generator to represent simple unions more efficiently internally. 2. Optional external flat representation: improve the QAPI code generator and visitors to let code opt in to ditch the "data" wrappers of simple unions in the external format. Depends on 1. 3. qemu-storage-daemon --chardev opts in. This gets rid of the unwanted nesting except for "backend." 4. qemu-storage-daemon --chardev manually flattens "backend." Possible future work: 5. Accept external flat representation in addition to nested, deprecate nested. 6. After the nested representation is gone for all simple unions, simplify QAPI code generators and visitors again. 7. Only then can we drop simple unions. Note that this tackles a wider problem than just qemu-storage-daemon --chardev: the infrastructure changes support ditching "data" wrappers of simple unions *everywhere*, it just doesn't opt in elsewhere. Moreover, it provides a path towards deprecation and removal of these wrappers in QMP. A few things give me an uneasy feeling, though: * Given how close to the freeze we are, this feels awfully ambitious. * The QAPI code generator changes look okay, but they do make things more complex. If we manage to kill nested representation everywhere, most (all?) of the complexity goes away. To be frank, the "if" doesn't inspire confidence in me. Even if we pull it off, it'll probably take quite some time. * Ditching simple unions may become much harder until we kill nested representation everywhere. Right now, simple unions are a syntactical convenience. We could rewrite them to flat in the schema, and simplify the QAPI code generator. Let's take a step back and review the use of simple unions in our QAPI schema. We have just eight of them. Three occur only in command results: * query-named-block-nodes and query-block use ImageInfoSpecific * query-memory-devices uses MemoryDeviceInfo * query-tpm uses TpmTypeOptions None of them will matter for a CLI. Getting rid of "data" wrappers in results is even harder than for arguments. I doubt it's worthwhile. Five occur only in command arguments: * chardev-add and chardev-change use ChardevBackend and SocketAddressLegacy This is the problem at hand. * nbd-server-start uses SocketAddressLegacy This is a solved problem: qemu-storage-daemon --nbd-server uses SocketAddress instead. * send-key and send-event use KeyValue * transaction uses TransactionAction These are non-problems, and likely to remain non-problems forever. The --chardev is the *only* instance of the wider "simple unions make the CLI unbearably ugly" problem, as far as I can tell. What's the stupidest solution that could possibly work? The same as we used for --nbd-server: define a new, flat QAPI type. Only stupider: leave the internal interface as is, and convert the new, flat QAPI type to the old, nested one. We should really convert the other way, but the freeze makes me go for "stupidest" hard. This series does exactly that. Lightly tested. Compare to Kevin's series: * Diffstat less PATCH 1+2 (which the two have in common): mine: 8 files changed, 315 insertions(+), 18 deletions(-) *.json 1 file changed, 98 insertions(+), 8 deletions(-) *.[ch] 6 files changed, 216 insertions(+), 10 deletions(-) Kevin's: 71 files changed, 504 insertions(+), 340 deletions(-) tests/ 24 files changed, 128 insertions(+), 97 deletions(-) *.json 1 file changed, 2 insertions(+), 1 deletion(-) *.[ch] 40 files changed, 226 insertions(+), 209 deletions(-) * The bulk of my changes is straightforward and as safe as it gets: I add schema definitions, and a mostly mechanical conversion function that is only used by qemu-storage-daemon --chardev. Kevin's changes are much more complex. QAPI code generator and visitor changes potentially affect everything. As far as I can tell, they don't, and they are solid. Right now, the complexity doesn't really buy us anything. See "Possible future work" above for a few ideas on what it could buy us down the road. Tell me what you think. KEVIN Wolf (3): char/stdio: Fix QMP default for 'signal' char: Factor out qemu_chr_print_types() qemu-storage-daemon: QAPIfy --chardev Markus Armbruster (1): char: Flat alternative to overly nested chardev-add arguments qapi/char.json | 109 +++++++++++++++++++-- include/chardev/char.h | 6 ++ include/qemu/sockets.h | 3 + chardev/char-legacy.c | 140 +++++++++++++++++++++++++++ chardev/char-socket.c | 3 +- chardev/char-stdio.c | 4 +- chardev/char.c | 16 +-- storage-daemon/qemu-storage-daemon.c | 37 +++++-- util/qemu-sockets.c | 38 ++++++++ chardev/meson.build | 1 + 10 files changed, 328 insertions(+), 29 deletions(-) create mode 100644 chardev/char-legacy.c -- 2.26.2 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/4] char/stdio: Fix QMP default for 'signal' 2020-10-26 10:10 [PATCH 0/4] qemu-storage-daemon: QAPIfy --chardev the stupid way Markus Armbruster @ 2020-10-26 10:10 ` Markus Armbruster 2020-10-26 10:10 ` [PATCH 2/4] char: Factor out qemu_chr_print_types() Markus Armbruster ` (3 subsequent siblings) 4 siblings, 0 replies; 18+ messages in thread From: Markus Armbruster @ 2020-10-26 10:10 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, marcandre.lureau, qemu-block, pbonzini From: Kevin Wolf <kwolf@redhat.com> Commit 02c4bdf1 tried to make signal=on the default for stdio chardevs except for '-serial mon:stdio', but it forgot about QMP and accidentally switched the QMP default from true (except for -nographic) to false (always). The QMP documentation was kept unchanged and still describes the opposite of the old behaviour (which is an even older documentation bug). Fix all of this by making signal=true the default in ChardevStdio and documenting it as such. Fixes: 02c4bdf1d2ca8c02a9bae16398f260b5c08d08bf Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Message-Id: <20201023161312.460406-2-kwolf@redhat.com> --- qapi/char.json | 3 +-- chardev/char-stdio.c | 4 +--- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/qapi/char.json b/qapi/char.json index b4d66ec90b..43486d1daa 100644 --- a/qapi/char.json +++ b/qapi/char.json @@ -321,8 +321,7 @@ # Configuration info for stdio chardevs. # # @signal: Allow signals (such as SIGINT triggered by ^C) -# be delivered to qemu. Default: true in -nographic mode, -# false otherwise. +# be delivered to qemu. Default: true. # # Since: 1.5 ## diff --git a/chardev/char-stdio.c b/chardev/char-stdio.c index 82eaebc1db..403da308c9 100644 --- a/chardev/char-stdio.c +++ b/chardev/char-stdio.c @@ -112,9 +112,7 @@ static void qemu_chr_open_stdio(Chardev *chr, qemu_chr_open_fd(chr, 0, 1); - if (opts->has_signal) { - stdio_allow_signal = opts->signal; - } + stdio_allow_signal = !opts->has_signal || opts->signal; qemu_chr_set_echo_stdio(chr, false); } #endif -- 2.26.2 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/4] char: Factor out qemu_chr_print_types() 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 ` Markus Armbruster 2020-10-26 10:10 ` [PATCH 3/4] char: Flat alternative to overly nested chardev-add arguments Markus Armbruster ` (2 subsequent siblings) 4 siblings, 0 replies; 18+ messages in thread From: Markus Armbruster @ 2020-10-26 10:10 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, marcandre.lureau, qemu-block, pbonzini From: Kevin Wolf <kwolf@redhat.com> We'll want to call the same from a non-QemuOpts code path. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Message-Id: <20201023161312.460406-3-kwolf@redhat.com> --- include/chardev/char.h | 1 + chardev/char.c | 16 ++++++++++------ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/include/chardev/char.h b/include/chardev/char.h index db42f0a8c6..3b91645081 100644 --- a/include/chardev/char.h +++ b/include/chardev/char.h @@ -212,6 +212,7 @@ void qemu_chr_be_update_read_handlers(Chardev *s, */ void qemu_chr_be_event(Chardev *s, QEMUChrEvent event); +void qemu_chr_print_types(void); int qemu_chr_add_client(Chardev *s, int fd); Chardev *qemu_chr_find(const char *name); diff --git a/chardev/char.c b/chardev/char.c index 78553125d3..4d188bfa6f 100644 --- a/chardev/char.c +++ b/chardev/char.c @@ -633,6 +633,15 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts, Error **errp) return backend; } +void qemu_chr_print_types(void) +{ + g_autoptr(GString) str = g_string_new(""); + + chardev_name_foreach(help_string_append, str); + + qemu_printf("Available chardev backend types: %s\n", str->str); +} + Chardev *qemu_chr_new_from_opts(QemuOpts *opts, GMainContext *context, Error **errp) { @@ -644,12 +653,7 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts, GMainContext *context, char *bid = NULL; if (name && is_help_option(name)) { - GString *str = g_string_new(""); - - chardev_name_foreach(help_string_append, str); - - qemu_printf("Available chardev backend types: %s\n", str->str); - g_string_free(str, true); + qemu_chr_print_types(); return NULL; } -- 2.26.2 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/4] char: Flat alternative to overly nested chardev-add arguments 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 ` Markus Armbruster 2020-10-27 18:23 ` Eric Blake 2020-10-26 10:10 ` [PATCH 4/4] qemu-storage-daemon: QAPIfy --chardev Markus Armbruster 2020-10-27 18:36 ` [PATCH 0/4] qemu-storage-daemon: QAPIfy --chardev the stupid way Paolo Bonzini 4 siblings, 1 reply; 18+ messages in thread From: Markus Armbruster @ 2020-10-26 10:10 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, marcandre.lureau, qemu-block, pbonzini 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. 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. 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 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' } +## +# @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' } + +## +# @ChardevSocketFlat: +# +# Note: This type should eventually replace ChardevSocket. The +# difference between the two: ChardevSocketFlat uses +# SocketAddressLegacy, ChardevSocket uses SocketAddress. +## +{ 'struct': 'ChardevSocketFlat', + # Do not add to 'data', it breaks chardev_options_crumple()! Add to + # ChardevSocketBase's 'data' instead. + 'data': { 'addr': 'SocketAddress' }, + 'base': 'ChardevSocketBase' } + ## # @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. +## +{ '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' } } + ## # @ChardevReturn: # diff --git a/include/chardev/char.h b/include/chardev/char.h index 3b91645081..78090a4178 100644 --- a/include/chardev/char.h +++ b/include/chardev/char.h @@ -284,6 +284,11 @@ extern int term_escape_char; GSource *qemu_chr_timeout_add_ms(Chardev *chr, guint ms, GSourceFunc func, void *private); +/* char-legacy.c */ + +/* TODO replace by conversion in opposite direction */ +q_obj_chardev_add_arg *chardev_options_crumple(ChardevOptions *chr); + /* console.c */ void qemu_chr_parse_vc(QemuOpts *opts, ChardevBackend *backend, Error **errp); diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h index 7d1f813576..75dc58b4c8 100644 --- a/include/qemu/sockets.h +++ b/include/qemu/sockets.h @@ -111,4 +111,7 @@ SocketAddress *socket_remote_address(int fd, Error **errp); */ SocketAddress *socket_address_flatten(SocketAddressLegacy *addr); +/* TODO remove along with chardev_options_crumple() */ +SocketAddressLegacy *socket_address_crumple(SocketAddress *addr); + #endif /* QEMU_SOCKETS_H */ diff --git a/chardev/char-legacy.c b/chardev/char-legacy.c new file mode 100644 index 0000000000..d4706d7954 --- /dev/null +++ b/chardev/char-legacy.c @@ -0,0 +1,140 @@ +/* + * Convert between legacy and modern interfaces + * + * Copyright (C) 2020 Red Hat, Inc. + * + * Authors: + * Markus Armbruster <armbru@redhat.com> + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ + +#include "qemu/osdep.h" +#include "qapi/clone-visitor.h" +#include "qapi/qapi-visit-char.h" +#include "qemu/sockets.h" +#include "chardev/char.h" + +/* + * 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; + case CHARDEV_BACKEND_TYPE_SERIAL: + be->type = CHARDEV_BACKEND_KIND_SERIAL; + be->u.serial.data = QAPI_CLONE(ChardevHostdev, &chr->u.serial); + break; + case CHARDEV_BACKEND_TYPE_PARALLEL: + be->type = CHARDEV_BACKEND_KIND_PARALLEL; + be->u.parallel.data = QAPI_CLONE(ChardevHostdev, &chr->u.parallel); + break; + case CHARDEV_BACKEND_TYPE_PIPE: + be->type = CHARDEV_BACKEND_KIND_PIPE; + be->u.pipe.data = QAPI_CLONE(ChardevHostdev, &chr->u.pipe); + break; + 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; + case CHARDEV_BACKEND_TYPE_UDP: + be->type = CHARDEV_BACKEND_KIND_UDP; + /* + * Clone with SocketAddress crumpled to SocketAddressLegacy. + * All other members in are the base type. + */ + be->u.udp.data = g_memdup(&chr->u.udp, sizeof(chr->u.udp)); + QAPI_CLONE_MEMBERS(ChardevCommon, + qapi_ChardevUdp_base(be->u.udp.data), + qapi_ChardevUdpFlat_base(&chr->u.udp)); + be->u.udp.data->remote = socket_address_crumple(chr->u.udp.remote); + be->u.udp.data->local = socket_address_crumple(chr->u.udp.local); + break; + case CHARDEV_BACKEND_TYPE_PTY: + be->type = CHARDEV_BACKEND_KIND_PTY; + be->u.pty.data = QAPI_CLONE(ChardevCommon, &chr->u.pty); + break; + case CHARDEV_BACKEND_TYPE_NULL: + be->type = CHARDEV_BACKEND_KIND_NULL; + be->u.null.data = QAPI_CLONE(ChardevCommon, &chr->u.null); + break; + case CHARDEV_BACKEND_TYPE_MUX: + be->type = CHARDEV_BACKEND_KIND_MUX; + be->u.mux.data = QAPI_CLONE(ChardevMux, &chr->u.mux); + break; + case CHARDEV_BACKEND_TYPE_MSMOUSE: + be->type = CHARDEV_BACKEND_KIND_MSMOUSE; + be->u.msmouse.data = QAPI_CLONE(ChardevCommon, &chr->u.msmouse); + break; + case CHARDEV_BACKEND_TYPE_WCTABLET: + be->type = CHARDEV_BACKEND_KIND_WCTABLET; + be->u.wctablet.data = QAPI_CLONE(ChardevCommon, &chr->u.wctablet); + break; + case CHARDEV_BACKEND_TYPE_BRAILLE: + be->type = CHARDEV_BACKEND_KIND_BRAILLE; + be->u.braille.data = QAPI_CLONE(ChardevCommon, &chr->u.braille); + break; + case CHARDEV_BACKEND_TYPE_TESTDEV: + be->type = CHARDEV_BACKEND_KIND_TESTDEV; + be->u.testdev.data = QAPI_CLONE(ChardevCommon, &chr->u.testdev); + break; + case CHARDEV_BACKEND_TYPE_STDIO: + be->type = CHARDEV_BACKEND_KIND_STDIO; + be->u.stdio.data = QAPI_CLONE(ChardevStdio, &chr->u.stdio); + break; + case CHARDEV_BACKEND_TYPE_CONSOLE: + be->type = CHARDEV_BACKEND_KIND_CONSOLE; + be->u.console.data = QAPI_CLONE(ChardevCommon, &chr->u.console); + break; +#ifdef CONFIG_SPICE + case CHARDEV_BACKEND_TYPE_SPICEVMC: + be->type = CHARDEV_BACKEND_KIND_SPICEVMC; + be->u.spicevmc.data = QAPI_CLONE(ChardevSpiceChannel, + &chr->u.spicevmc); + break; + case CHARDEV_BACKEND_TYPE_SPICEPORT: + be->type = CHARDEV_BACKEND_KIND_SPICEPORT; + be->u.spiceport.data = QAPI_CLONE(ChardevSpicePort, + &chr->u.spiceport); + break; +#endif + case CHARDEV_BACKEND_TYPE_VC: + be->type = CHARDEV_BACKEND_KIND_VC; + be->u.vc.data = QAPI_CLONE(ChardevVC, &chr->u.vc); + break; + case CHARDEV_BACKEND_TYPE_RINGBUF: + be->type = CHARDEV_BACKEND_KIND_RINGBUF; + be->u.ringbuf.data = QAPI_CLONE(ChardevRingbuf, &chr->u.ringbuf); + break; + default: + abort(); + } + + return arg; +} diff --git a/chardev/char-socket.c b/chardev/char-socket.c index 95e45812d5..37a725da7f 100644 --- a/chardev/char-socket.c +++ 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))); sock->has_nodelay = qemu_opt_get(opts, "delay"); sock->nodelay = !qemu_opt_get_bool(opts, "delay", true); diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index 05e5c73f9d..033f112868 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -1387,3 +1387,41 @@ SocketAddress *socket_address_flatten(SocketAddressLegacy *addr_legacy) return addr; } + +/* TODO remove along with chardev_options_crumple() */ +SocketAddressLegacy *socket_address_crumple(SocketAddress *addr) +{ + SocketAddressLegacy *addr_legacy; + + if (!addr) { + return NULL; + } + + addr_legacy = g_malloc(sizeof(*addr_legacy)); + + switch (addr->type) { + case SOCKET_ADDRESS_TYPE_INET: + addr_legacy->type = SOCKET_ADDRESS_LEGACY_KIND_INET; + addr_legacy->u.inet.data = QAPI_CLONE(InetSocketAddress, + &addr->u.inet); + break; + case SOCKET_ADDRESS_TYPE_UNIX: + addr_legacy->type = SOCKET_ADDRESS_LEGACY_KIND_UNIX; + addr_legacy->u.q_unix.data = QAPI_CLONE(UnixSocketAddress, + &addr->u.q_unix); + break; + case SOCKET_ADDRESS_TYPE_VSOCK: + addr_legacy->type = SOCKET_ADDRESS_LEGACY_KIND_VSOCK; + addr_legacy->u.vsock.data = QAPI_CLONE(VsockSocketAddress, + &addr->u.vsock); + break; + case SOCKET_ADDRESS_TYPE_FD: + addr_legacy->type = SOCKET_ADDRESS_LEGACY_KIND_FD; + addr_legacy->u.fd.data = QAPI_CLONE(String, &addr->u.fd); + break; + default: + abort(); + } + + return addr_legacy; +} diff --git a/chardev/meson.build b/chardev/meson.build index 859d8b04d4..25074097fb 100644 --- a/chardev/meson.build +++ b/chardev/meson.build @@ -2,6 +2,7 @@ chardev_ss.add(files( 'char-fe.c', 'char-file.c', 'char-io.c', + 'char-legacy.c', 'char-mux.c', 'char-null.c', 'char-pipe.c', -- 2.26.2 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] char: Flat alternative to overly nested chardev-add arguments 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 0 siblings, 1 reply; 18+ messages in thread From: Eric Blake @ 2020-10-27 18:23 UTC (permalink / raw) To: Markus Armbruster, qemu-devel Cc: kwolf, marcandre.lureau, qemu-block, pbonzini 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 <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. > > 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 <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] char: Flat alternative to overly nested chardev-add arguments 2020-10-27 18:23 ` Eric Blake @ 2020-10-28 7:33 ` Markus Armbruster 0 siblings, 0 replies; 18+ messages in thread From: Markus Armbruster @ 2020-10-28 7:33 UTC (permalink / raw) To: Eric Blake; +Cc: kwolf, marcandre.lureau, qemu-devel, qemu-block, pbonzini 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! ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/4] qemu-storage-daemon: QAPIfy --chardev 2020-10-26 10:10 [PATCH 0/4] qemu-storage-daemon: QAPIfy --chardev the stupid way Markus Armbruster ` (2 preceding siblings ...) 2020-10-26 10:10 ` [PATCH 3/4] char: Flat alternative to overly nested chardev-add arguments Markus Armbruster @ 2020-10-26 10:10 ` Markus Armbruster 2020-10-27 18:59 ` Eric Blake 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 4 siblings, 2 replies; 18+ messages in thread From: Markus Armbruster @ 2020-10-26 10:10 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, marcandre.lureau, qemu-block, pbonzini 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. - 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. 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" #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: -- 2.26.2 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] qemu-storage-daemon: QAPIfy --chardev 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 1 sibling, 1 reply; 18+ messages in thread From: Eric Blake @ 2020-10-27 18:59 UTC (permalink / raw) To: Markus Armbruster, qemu-devel Cc: kwolf, marcandre.lureau, qemu-block, pbonzini 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] qemu-storage-daemon: QAPIfy --chardev 2020-10-27 18:59 ` Eric Blake @ 2020-10-28 7:42 ` Markus Armbruster 0 siblings, 0 replies; 18+ messages in thread From: Markus Armbruster @ 2020-10-28 7:42 UTC (permalink / raw) To: Eric Blake; +Cc: kwolf, marcandre.lureau, qemu-devel, qemu-block, pbonzini Eric Blake <eblake@redhat.com> writes: > 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? On the one hand, it's in 5.1, which means we get to worry about compatibility. On the other hand, it is documented to default to true in QMP, which means we can legitimately treat the change as bug fix. I'll look into it. >> - 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. Yes. >> #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> Thanks! ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] qemu-storage-daemon: QAPIfy --chardev 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 9:18 ` Markus Armbruster 1 sibling, 0 replies; 18+ messages in thread From: Markus Armbruster @ 2020-10-28 9:18 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, marcandre.lureau, qemu-block, pbonzini Markus Armbruster <armbru@redhat.com> writes: > 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: [...] > * Backend type "socket": [...] > - 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. It's even worse than that. I'll start a new thread. [...] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] qemu-storage-daemon: QAPIfy --chardev the stupid way 2020-10-26 10:10 [PATCH 0/4] qemu-storage-daemon: QAPIfy --chardev the stupid way Markus Armbruster ` (3 preceding siblings ...) 2020-10-26 10:10 ` [PATCH 4/4] qemu-storage-daemon: QAPIfy --chardev Markus Armbruster @ 2020-10-27 18:36 ` Paolo Bonzini 2020-10-28 7:01 ` Markus Armbruster 4 siblings, 1 reply; 18+ messages in thread From: Paolo Bonzini @ 2020-10-27 18:36 UTC (permalink / raw) To: Markus Armbruster, qemu-devel; +Cc: kwolf, marcandre.lureau, qemu-block On 26/10/20 11:10, Markus Armbruster wrote: > Kevin's "[PATCH v2 0/6] qemu-storage-daemon: QAPIfy --chardev" > involves surgery to the QAPI generator. Some (most?) of it should go > away if we deprecate the "data" wrappers due to simple unions in QMP. > > Do we really need to mess with the code generator to solve the problem > at hand? > > > Let's recapitulate the problem: > > * We want to QAPIfy --chardev, i.e. define its argument as a QAPI > type. Considering that this is not 5.2 stuff at this point, I would like to suggest again moving chardevs to -object, and ask you to evaluate that option with the agreement that I do the work instead of you. :) Paolo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] qemu-storage-daemon: QAPIfy --chardev the stupid way 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 0 siblings, 1 reply; 18+ messages in thread From: Markus Armbruster @ 2020-10-28 7:01 UTC (permalink / raw) To: Paolo Bonzini Cc: kwolf, marcandre.lureau, qemu-devel, qemu-block, Eduardo Habkost Paolo Bonzini <pbonzini@redhat.com> writes: > On 26/10/20 11:10, Markus Armbruster wrote: >> Kevin's "[PATCH v2 0/6] qemu-storage-daemon: QAPIfy --chardev" >> involves surgery to the QAPI generator. Some (most?) of it should go >> away if we deprecate the "data" wrappers due to simple unions in QMP. >> >> Do we really need to mess with the code generator to solve the problem >> at hand? >> >> >> Let's recapitulate the problem: >> >> * We want to QAPIfy --chardev, i.e. define its argument as a QAPI >> type. > > Considering that this is not 5.2 stuff at this point, I would like to > suggest again moving chardevs to -object, and ask you to evaluate that > option with the agreement that I do the work instead of you. :) Replacing -chardev with -object without regressing features would be lovely. One feature in particular: introspection. If we manage to fully QAPIfy -object, we should be good. I understand Eduardo is cutting a path through the jungle. I can offer assistance with bridging QAPI schema to QOM. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] qemu-storage-daemon: QAPIfy --chardev the stupid way 2020-10-28 7:01 ` Markus Armbruster @ 2020-10-28 11:46 ` Kevin Wolf 2020-10-28 14:39 ` Paolo Bonzini 0 siblings, 1 reply; 18+ messages in thread From: Kevin Wolf @ 2020-10-28 11:46 UTC (permalink / raw) To: Markus Armbruster Cc: Paolo Bonzini, Eduardo Habkost, qemu-devel, qemu-block, marcandre.lureau Am 28.10.2020 um 08:01 hat Markus Armbruster geschrieben: > Paolo Bonzini <pbonzini@redhat.com> writes: > > > On 26/10/20 11:10, Markus Armbruster wrote: > >> Kevin's "[PATCH v2 0/6] qemu-storage-daemon: QAPIfy --chardev" > >> involves surgery to the QAPI generator. Some (most?) of it should go > >> away if we deprecate the "data" wrappers due to simple unions in QMP. > >> > >> Do we really need to mess with the code generator to solve the problem > >> at hand? > >> > >> > >> Let's recapitulate the problem: > >> > >> * We want to QAPIfy --chardev, i.e. define its argument as a QAPI > >> type. > > > > Considering that this is not 5.2 stuff at this point, I would like to > > suggest again moving chardevs to -object, and ask you to evaluate that > > option with the agreement that I do the work instead of you. :) I don't think this is the right thing to do at this point. Making more use of QOM is an orthogonal problem and would only make solving this one harder. The problem we have and we're trying to solve is that we have chardev-add (which has a QAPI schema) and -chardev (which doesn't). We want to get an option that is described by the schema, doesn't duplicate things and is still convenient to use. Whether this option starts with -chardev or with -object doesn't really make much of a difference. The QAPI schema you need behind it will be almost or even exactly the same. > Replacing -chardev with -object without regressing features would be > lovely. One feature in particular: introspection. > > If we manage to fully QAPIfy -object, we should be good. I understand > Eduardo is cutting a path through the jungle. I don't expect many difficulties with the existing -object (famous last words). But if you make chardevs user creatable objects first, it becomes much harder because you just combined two problems (one of which is already known to be hard) into one large problem. > I can offer assistance with bridging QAPI schema to QOM. So, the steps that I would suggest are: 1a. Finish the QAPI schema for object-add 1b. Find some way to unify chardev-add's ChardevBackend and a CLI-friendly version of it in the schema 2. Generate QOM boilerplate code from ObjectOptions instead of duplicating it in the implementation 3. Convert chardevs to using QOM properites only now If we do 3. earlier, we'll write code that we want to replace later anyway. If we even do it in parallel with 1b. we'll additionally get merge conflicts. And if we do it before 1b. we'll do it without considering QAPI in detail and will lose any flexibility to change things in the new interface, which will make 1b. much harder. Kevin ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] qemu-storage-daemon: QAPIfy --chardev the stupid way 2020-10-28 11:46 ` Kevin Wolf @ 2020-10-28 14:39 ` Paolo Bonzini 2020-10-28 14:59 ` Kevin Wolf 0 siblings, 1 reply; 18+ messages in thread From: Paolo Bonzini @ 2020-10-28 14:39 UTC (permalink / raw) To: Kevin Wolf, Markus Armbruster Cc: marcandre.lureau, qemu-devel, qemu-block, Eduardo Habkost On 28/10/20 12:46, Kevin Wolf wrote: > I don't think this is the right thing to do at this point. Making more > use of QOM is an orthogonal problem and would only make solving this one > harder. Making more use of QOM will make this a non-problem. You'll just use object-add and -object and, when you figure out the QOM schema, it will just work. Paolo > The problem we have and we're trying to solve is that we have > chardev-add (which has a QAPI schema) and -chardev (which doesn't). We > want to get an option that is described by the schema, doesn't duplicate > things and is still convenient to use. > > Whether this option starts with -chardev or with -object doesn't really > make much of a difference. The QAPI schema you need behind it will be > almost or even exactly the same. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] qemu-storage-daemon: QAPIfy --chardev the stupid way 2020-10-28 14:39 ` Paolo Bonzini @ 2020-10-28 14:59 ` Kevin Wolf 2020-10-28 15:09 ` Paolo Bonzini 0 siblings, 1 reply; 18+ messages in thread From: Kevin Wolf @ 2020-10-28 14:59 UTC (permalink / raw) To: Paolo Bonzini Cc: marcandre.lureau, Eduardo Habkost, Markus Armbruster, qemu-block, qemu-devel Am 28.10.2020 um 15:39 hat Paolo Bonzini geschrieben: > On 28/10/20 12:46, Kevin Wolf wrote: > > I don't think this is the right thing to do at this point. Making more > > use of QOM is an orthogonal problem and would only make solving this one > > harder. > > Making more use of QOM will make this a non-problem. You'll just use > object-add and -object and, when you figure out the QOM schema, it will > just work. Yes, but figuring out the QOM schema (or rather, what the interface represented by the schema should look like) is the hard part. It is exactly the same hard part that we're currently trying to figure out without also worrying about QOM at the same time. -object and object-add instead of -chardev and chardev-add change the spelling, but solve none of these problems. Kevin ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] qemu-storage-daemon: QAPIfy --chardev the stupid way 2020-10-28 14:59 ` Kevin Wolf @ 2020-10-28 15:09 ` Paolo Bonzini 2020-10-28 15:39 ` Kevin Wolf 0 siblings, 1 reply; 18+ messages in thread From: Paolo Bonzini @ 2020-10-28 15:09 UTC (permalink / raw) To: Kevin Wolf Cc: marcandre.lureau, Eduardo Habkost, Markus Armbruster, qemu-block, qemu-devel On 28/10/20 15:59, Kevin Wolf wrote: >> Making more use of QOM will make this a non-problem. You'll just use >> object-add and -object and, when you figure out the QOM schema, it will >> just work. > > Yes, but figuring out the QOM schema (or rather, what the interface > represented by the schema should look like) is the hard part. I don't disagree with that, but it's a problem you have to solve anyway, isn't it? Once you figure out how to introspect QOM classes, that would apply just as well to character devices. On the other hand, creating character devices with -object does solve another problem, namely the distinction between "early" and "late" objects in vl.c, in a way that QAPIfied -chardev doesn't solve. Paolo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] qemu-storage-daemon: QAPIfy --chardev the stupid way 2020-10-28 15:09 ` Paolo Bonzini @ 2020-10-28 15:39 ` Kevin Wolf 2020-10-28 16:01 ` Paolo Bonzini 0 siblings, 1 reply; 18+ messages in thread From: Kevin Wolf @ 2020-10-28 15:39 UTC (permalink / raw) To: Paolo Bonzini Cc: marcandre.lureau, Eduardo Habkost, Markus Armbruster, qemu-block, qemu-devel Am 28.10.2020 um 16:09 hat Paolo Bonzini geschrieben: > On 28/10/20 15:59, Kevin Wolf wrote: > >> Making more use of QOM will make this a non-problem. You'll just use > >> object-add and -object and, when you figure out the QOM schema, it will > >> just work. > > > > Yes, but figuring out the QOM schema (or rather, what the interface > > represented by the schema should look like) is the hard part. > > I don't disagree with that, but it's a problem you have to solve anyway, > isn't it? Once you figure out how to introspect QOM classes, that would > apply just as well to character devices. Yes, it's the problem I tried to address with my series, and Markus with this alternative series. We need to do this either way. > On the other hand, creating character devices with -object does solve > another problem, namely the distinction between "early" and "late" > objects in vl.c, in a way that QAPIfied -chardev doesn't solve. Right. Both are solving different problems, and solving one won't automatically make the other a non-problem as you suggested above. I just suggested leaving QOM for later because two people making big changes on the same subsystem is going to be painful for at least one of them, and because for adding QOM properties, you need to know what these properties should look like (unless you want to change them again later). If you don't wait for the QAPI work, you'll have solved the problem of having two separate ways to describe chardev options by making it three separate ways. Technically this might fulfill the condition of not having two separate ways, but it's not really what we had in mind. :-) Kevin ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] qemu-storage-daemon: QAPIfy --chardev the stupid way 2020-10-28 15:39 ` Kevin Wolf @ 2020-10-28 16:01 ` Paolo Bonzini 0 siblings, 0 replies; 18+ messages in thread From: Paolo Bonzini @ 2020-10-28 16:01 UTC (permalink / raw) To: Kevin Wolf Cc: marcandre.lureau, Eduardo Habkost, Markus Armbruster, qemu-block, qemu-devel On 28/10/20 16:39, Kevin Wolf wrote: >> I don't disagree with that, but it's a problem you have to solve anyway, >> isn't it? Once you figure out how to introspect QOM classes, that would >> apply just as well to character devices. > Yes, it's the problem I tried to address with my series, and Markus with > this alternative series. We need to do this either way. Right, I appreciate that QOMifying chardev would only be a solution if QOM introspection gets into 6.0. This is why I only brought it up because neither of these series will be ready in time for 5.2. So maybe QOMifying chardev wouldn't make it a non-problem; it would make it someone else's (Eduardo's) problem. > you need to know what these properties should look like True that. But I think the existing QAPI structures do help for that. > If you don't wait for the QAPI work, you'll have solved the problem of > having two separate ways to describe chardev options by making it three > separate ways. Technically this might fulfill the condition of not > having two separate ways, but it's not really what we had in mind. :-) Actually four ways (-chardev, chardev-add, -object, object-add) but two of them would be implemented by the same code and qsd would be able to standardize on them. Paolo ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-10-28 16:06 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).