diff mbox series

[4/4] qemu-storage-daemon: QAPIfy --chardev

Message ID 20201026101005.2940615-5-armbru@redhat.com
State New
Headers show
Series qemu-storage-daemon: QAPIfy --chardev the stupid way | expand

Commit Message

Markus Armbruster Oct. 26, 2020, 10:10 a.m. UTC
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(-)

Comments

Eric Blake Oct. 27, 2020, 6:59 p.m. UTC | #1
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>
Markus Armbruster Oct. 28, 2020, 7:42 a.m. UTC | #2
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!
Markus Armbruster Oct. 28, 2020, 9:18 a.m. UTC | #3
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.

[...]
diff mbox series

Patch

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: