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

[Markus Armbruster <armbru@redhat.com>]

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!