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
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. [...]
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
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!
© 2016 - 2025 Red Hat, Inc.