[RFC PATCH 02/18] qemu-storage-daemon: Add --object option

Kevin Wolf posted 18 patches 23 weeks ago

[RFC PATCH 02/18] qemu-storage-daemon: Add --object option

Posted by Kevin Wolf 23 weeks ago
Add a command line option to create user-creatable QOM objects.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-storage-daemon.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
index a251dc255c..48d6af43a6 100644
--- a/qemu-storage-daemon.c
+++ b/qemu-storage-daemon.c
@@ -35,6 +35,8 @@
 #include "qemu/log.h"
 #include "qemu/main-loop.h"
 #include "qemu/module.h"
+#include "qemu/option.h"
+#include "qom/object_interfaces.h"
 
 #include "trace/control.h"
 
@@ -51,10 +53,26 @@ static void help(void)
 "                         specify tracing options\n"
 "  -V, --version          output version information and exit\n"
 "\n"
+"  --object <properties>  define a QOM object such as 'secret' for\n"
+"                         passwords and/or encryption keys\n"
+"\n"
 QEMU_HELP_BOTTOM "\n",
     error_get_progname());
 }
 
+enum {
+    OPTION_OBJECT = 256,
+};
+
+static QemuOptsList qemu_object_opts = {
+    .name = "object",
+    .implied_opt_name = "qom-type",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
+    .desc = {
+        { }
+    },
+};
+
 static int process_options(int argc, char *argv[], Error **errp)
 {
     int c;
@@ -63,6 +81,7 @@ static int process_options(int argc, char *argv[], Error **errp)
 
     static const struct option long_options[] = {
         {"help", no_argument, 0, 'h'},
+        {"object", required_argument, 0, OPTION_OBJECT},
         {"version", no_argument, 0, 'V'},
         {"trace", required_argument, NULL, 'T'},
         {0, 0, 0, 0}
@@ -88,6 +107,22 @@ static int process_options(int argc, char *argv[], Error **errp)
             g_free(trace_file);
             trace_file = trace_opt_parse(optarg);
             break;
+        case OPTION_OBJECT:
+            {
+                QemuOpts *opts;
+                const char *type;
+
+                opts = qemu_opts_parse(&qemu_object_opts,
+                                       optarg, true, &error_fatal);
+                type = qemu_opt_get(opts, "qom-type");
+
+                if (user_creatable_print_help(type, opts)) {
+                    exit(EXIT_SUCCESS);
+                }
+                user_creatable_add_opts(opts, &error_fatal);
+                qemu_opts_del(opts);
+                break;
+            }
         }
     }
     if (optind != argc) {
-- 
2.20.1


Re: [RFC PATCH 02/18] qemu-storage-daemon: Add --object option

Posted by Markus Armbruster 20 weeks ago
Kevin Wolf <kwolf@redhat.com> writes:

> Add a command line option to create user-creatable QOM objects.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qemu-storage-daemon.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
> index a251dc255c..48d6af43a6 100644
> --- a/qemu-storage-daemon.c
> +++ b/qemu-storage-daemon.c
> @@ -35,6 +35,8 @@
>  #include "qemu/log.h"
>  #include "qemu/main-loop.h"
>  #include "qemu/module.h"
> +#include "qemu/option.h"
> +#include "qom/object_interfaces.h"
>  
>  #include "trace/control.h"
>  
> @@ -51,10 +53,26 @@ static void help(void)
>  "                         specify tracing options\n"
>  "  -V, --version          output version information and exit\n"
>  "\n"
> +"  --object <properties>  define a QOM object such as 'secret' for\n"
> +"                         passwords and/or encryption keys\n"

This is less helpful than qemu-system-FOO's help:

-object TYPENAME[,PROP1=VALUE1,...]
                create a new object of type TYPENAME setting properties
                in the order they are specified.  Note that the 'id'
                property must be set.  These objects are placed in the
                '/objects' path.

> +"\n"
>  QEMU_HELP_BOTTOM "\n",
>      error_get_progname());
>  }
>  
> +enum {
> +    OPTION_OBJECT = 256,
> +};
> +
> +static QemuOptsList qemu_object_opts = {
> +    .name = "object",
> +    .implied_opt_name = "qom-type",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
> +    .desc = {
> +        { }
> +    },
> +};
> +

Note for later: copied from vl.c.

>  static int process_options(int argc, char *argv[], Error **errp)
>  {
>      int c;
> @@ -63,6 +81,7 @@ static int process_options(int argc, char *argv[], Error **errp)
>  
>      static const struct option long_options[] = {
>          {"help", no_argument, 0, 'h'},
> +        {"object", required_argument, 0, OPTION_OBJECT},
>          {"version", no_argument, 0, 'V'},
>          {"trace", required_argument, NULL, 'T'},
>          {0, 0, 0, 0}
> @@ -88,6 +107,22 @@ static int process_options(int argc, char *argv[], Error **errp)
>              g_free(trace_file);
>              trace_file = trace_opt_parse(optarg);
>              break;
> +        case OPTION_OBJECT:
> +            {
> +                QemuOpts *opts;
> +                const char *type;
> +
> +                opts = qemu_opts_parse(&qemu_object_opts,
> +                                       optarg, true, &error_fatal);
> +                type = qemu_opt_get(opts, "qom-type");
> +
> +                if (user_creatable_print_help(type, opts)) {
> +                    exit(EXIT_SUCCESS);
> +                }
> +                user_creatable_add_opts(opts, &error_fatal);
> +                qemu_opts_del(opts);
> +                break;
> +            }
>          }
>      }
>      if (optind != argc) {

PATCH 01 duplicates case QEMU_OPTION_trace pretty much verbatim.  Makes
sense, as qemu-storage-daemon is basically qemu-system-FOO with "FOO"
and most "system" cut away.

This patch adds vl.c's case QEMU_OPTION_object in a much simpler form.
This is one of my least favourite options, and I'll tell you why below.
Let's compare the two versions.

vl.c:

            case QEMU_OPTION_object:
                opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
                                               optarg, true);
                if (!opts) {
                    exit(1);
                }
                break;

Further down:

    qemu_opts_foreach(qemu_find_opts("object"),
                      user_creatable_add_opts_foreach,
                      object_create_initial, &error_fatal);

Still further down:

    qemu_opts_foreach(qemu_find_opts("object"),
                      user_creatable_add_opts_foreach,
                      object_create_delayed, &error_fatal);

These are basically

    for opts in qemu_object_opts {
        type = qemu_opt_get(opts, "qom-type");
        if (type) {
            if (user_creatable_print_help(type, opts)) {
                exit(0);
            }
            if (!predicate(type)) {
                continue;
            }
        }
        obj = user_creatable_add_opts(opts, &error_fatal);
        object_unref(obj);
    }

where predicate(type) is true in exactly one of the two places for each
QOM type.

The reason for these gymnastics is to create objects at the right time
during startup, except there is no right time, but two.

Differences:

* Options are processed left to right without gymnastics.  Getting their
  order right is the user's problem.  I consider this an improvement.

* You use &qemu_object_opts instead of qemu_find_opts("object").  Also
  an improvement.

* You use qemu_opts_parse() instead of qemu_opts_parse_noisily().
  The latter can print help.  I failed to find a case where we lose help
  compared to qemu-system-FOO.  I didn't try very hard.

* You neglect to guard user_creatable_print_help():

    $ qemu-storage-daemon --object wrong=1,help
    Segmentation fault (core dumped)

* You neglect to object_unref().  I just double-checked the final
  reference count: it's 2.

These bugs shouldn't be hard to fix.


At this point you might wonder why I dislike this option so much.
vl.c's gymnastics are ugly, but not unusually ugly, and they're gone
here.  To explain my distaste, I have to go back a little bit.

Like quite a few options, --object is paired with QMP command, namely
object-add.  Both have the same parameters: QOM type, object ID, and
additional type-specific object properties.  There's a difference,
though: object-add wraps the latter in a 'props' object, while --object
does not.

QAPI schema:

    { 'command': 'object-add',
      'data': {'qom-type': 'str', 'id': 'str', '*props': 'any'} }

QAPIfying this part of the CLI isn't easy.

The obvious QAPIfied CLI buddy of object-add is incompatible to current
--object.  That's not a concern for the storage daemon.  But it's also
ugly, because object-add's nesting of the type-specific properties
within @props is.  In QMP, it's merely yet another pair of curlies.  In
the CLI, we get to prefix props. to each type-specific property.

If we want to give the storage daemon a QAPIfied command line from the
start (and I think we do), we'll have to decide how to address this
issue, and possibly more (I'm only at PATCH 02/18).

We have a long history of rather careless interface design, and now some
of these chickens come home to roost.


Re: [RFC PATCH 02/18] qemu-storage-daemon: Add --object option

Posted by Kevin Wolf 19 weeks ago
Am 07.11.2019 um 21:36 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Add a command line option to create user-creatable QOM objects.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qemu-storage-daemon.c | 35 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >
> > diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
> > index a251dc255c..48d6af43a6 100644
> > --- a/qemu-storage-daemon.c
> > +++ b/qemu-storage-daemon.c
> > @@ -35,6 +35,8 @@
> >  #include "qemu/log.h"
> >  #include "qemu/main-loop.h"
> >  #include "qemu/module.h"
> > +#include "qemu/option.h"
> > +#include "qom/object_interfaces.h"
> >  
> >  #include "trace/control.h"
> >  
> > @@ -51,10 +53,26 @@ static void help(void)
> >  "                         specify tracing options\n"
> >  "  -V, --version          output version information and exit\n"
> >  "\n"
> > +"  --object <properties>  define a QOM object such as 'secret' for\n"
> > +"                         passwords and/or encryption keys\n"
> 
> This is less helpful than qemu-system-FOO's help:
> 
> -object TYPENAME[,PROP1=VALUE1,...]
>                 create a new object of type TYPENAME setting properties
>                 in the order they are specified.  Note that the 'id'
>                 property must be set.  These objects are placed in the
>                 '/objects' path.

Hm, yes. I took the description from the tools. I can switch to the vl.c
one (should the tools, too?).

But honestly, neither of the two is enough to tell anyone how to
actually use this... Considering how many different objects there are,
maybe the best we can do is referring to the man page for details.

> > +"\n"
> >  QEMU_HELP_BOTTOM "\n",
> >      error_get_progname());
> >  }
> >  
> > +enum {
> > +    OPTION_OBJECT = 256,
> > +};
> > +
> > +static QemuOptsList qemu_object_opts = {
> > +    .name = "object",
> > +    .implied_opt_name = "qom-type",
> > +    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
> > +    .desc = {
> > +        { }
> > +    },
> > +};
> > +
> 
> Note for later: copied from vl.c.
> 
> >  static int process_options(int argc, char *argv[], Error **errp)
> >  {
> >      int c;
> > @@ -63,6 +81,7 @@ static int process_options(int argc, char *argv[], Error **errp)
> >  
> >      static const struct option long_options[] = {
> >          {"help", no_argument, 0, 'h'},
> > +        {"object", required_argument, 0, OPTION_OBJECT},
> >          {"version", no_argument, 0, 'V'},
> >          {"trace", required_argument, NULL, 'T'},
> >          {0, 0, 0, 0}
> > @@ -88,6 +107,22 @@ static int process_options(int argc, char *argv[], Error **errp)
> >              g_free(trace_file);
> >              trace_file = trace_opt_parse(optarg);
> >              break;
> > +        case OPTION_OBJECT:
> > +            {
> > +                QemuOpts *opts;
> > +                const char *type;
> > +
> > +                opts = qemu_opts_parse(&qemu_object_opts,
> > +                                       optarg, true, &error_fatal);
> > +                type = qemu_opt_get(opts, "qom-type");
> > +
> > +                if (user_creatable_print_help(type, opts)) {
> > +                    exit(EXIT_SUCCESS);
> > +                }
> > +                user_creatable_add_opts(opts, &error_fatal);
> > +                qemu_opts_del(opts);
> > +                break;
> > +            }
> >          }
> >      }
> >      if (optind != argc) {
> 
> PATCH 01 duplicates case QEMU_OPTION_trace pretty much verbatim.  Makes
> sense, as qemu-storage-daemon is basically qemu-system-FOO with "FOO"
> and most "system" cut away.
> 
> This patch adds vl.c's case QEMU_OPTION_object in a much simpler form.
> This is one of my least favourite options, and I'll tell you why below.
> Let's compare the two versions.
> 
> vl.c:
> 
>             case QEMU_OPTION_object:
>                 opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
>                                                optarg, true);
>                 if (!opts) {
>                     exit(1);
>                 }
>                 break;
> 
> Further down:
> 
>     qemu_opts_foreach(qemu_find_opts("object"),
>                       user_creatable_add_opts_foreach,
>                       object_create_initial, &error_fatal);
> 
> Still further down:
> 
>     qemu_opts_foreach(qemu_find_opts("object"),
>                       user_creatable_add_opts_foreach,
>                       object_create_delayed, &error_fatal);
> 
> These are basically
> 
>     for opts in qemu_object_opts {
>         type = qemu_opt_get(opts, "qom-type");
>         if (type) {
>             if (user_creatable_print_help(type, opts)) {
>                 exit(0);
>             }
>             if (!predicate(type)) {
>                 continue;
>             }
>         }
>         obj = user_creatable_add_opts(opts, &error_fatal);
>         object_unref(obj);
>     }
> 
> where predicate(type) is true in exactly one of the two places for each
> QOM type.
> 
> The reason for these gymnastics is to create objects at the right time
> during startup, except there is no right time, but two.
> 
> Differences:
> 
> * Options are processed left to right without gymnastics.  Getting their
>   order right is the user's problem.  I consider this an improvement.
> 
> * You use &qemu_object_opts instead of qemu_find_opts("object").  Also
>   an improvement.
> 
> * You use qemu_opts_parse() instead of qemu_opts_parse_noisily().
>   The latter can print help.  I failed to find a case where we lose help
>   compared to qemu-system-FOO.  I didn't try very hard.

I tried to reuse that code from qemu_opts_parse_noisily(), until I
realised that it wasn't even used for -object.

I don't remember the details why qemu_opts_print_help() wasn't even
called, but it's obvious that we can't lose anything from it:
qemu_object_opts has an empty list of properties, it accepts anything.
QemuOpts can't print any useful help when this is all the information it
has.

> * You neglect to guard user_creatable_print_help():
> 
>     $ qemu-storage-daemon --object wrong=1,help
>     Segmentation fault (core dumped)

Thanks for catching this. (You don't even need the ",help" part, just
--object wrong=1 is enough.)

> * You neglect to object_unref().  I just double-checked the final
>   reference count: it's 2.

Hm, yes. Weird interface, no caller actually needs this reference.

> These bugs shouldn't be hard to fix.
> 
> 
> At this point you might wonder why I dislike this option so much.
> vl.c's gymnastics are ugly, but not unusually ugly, and they're gone
> here.  To explain my distaste, I have to go back a little bit.
> 
> Like quite a few options, --object is paired with QMP command, namely
> object-add.  Both have the same parameters: QOM type, object ID, and
> additional type-specific object properties.  There's a difference,
> though: object-add wraps the latter in a 'props' object, while --object
> does not.
> 
> QAPI schema:
> 
>     { 'command': 'object-add',
>       'data': {'qom-type': 'str', 'id': 'str', '*props': 'any'} }
> 
> QAPIfying this part of the CLI isn't easy.
> 
> The obvious QAPIfied CLI buddy of object-add is incompatible to current
> --object.  That's not a concern for the storage daemon.  But it's also
> ugly, because object-add's nesting of the type-specific properties
> within @props is.  In QMP, it's merely yet another pair of curlies.  In
> the CLI, we get to prefix props. to each type-specific property.
> 
> If we want to give the storage daemon a QAPIfied command line from the
> start (and I think we do), we'll have to decide how to address this
> issue, and possibly more (I'm only at PATCH 02/18).
> 
> We have a long history of rather careless interface design, and now some
> of these chickens come home to roost.

On IRC, we discussed last week that we could turn object-add into a
'gen': false command and accept things both as options in props and as
flat options on the top level.

However, looking at this again, I'm afraid we forgot the context while
discussing specifics: How would this be used in a command line parser?

We don't start with a QDict here, but with a string. Getting a QDict
that could serve as an input to a modified qmp_object_add() would still
involve going through QemuOpts for parsing the option string, and then
converting it to a QDict. Using a visitor isn't possible with '*props':
'any' and even less so with 'gen': false.

So would this really improve things? Or do we have to wait until we have
an actual schema for object-add before calling qmp_object_add() actually
makes sense?

Kevin


Re: [RFC PATCH 02/18] qemu-storage-daemon: Add --object option

Posted by Markus Armbruster 19 weeks ago
Kevin Wolf <kwolf@redhat.com> writes:

> Am 07.11.2019 um 21:36 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Add a command line option to create user-creatable QOM objects.
>> >
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > ---
>> >  qemu-storage-daemon.c | 35 +++++++++++++++++++++++++++++++++++
>> >  1 file changed, 35 insertions(+)
>> >
>> > diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
>> > index a251dc255c..48d6af43a6 100644
>> > --- a/qemu-storage-daemon.c
>> > +++ b/qemu-storage-daemon.c
>> > @@ -35,6 +35,8 @@
>> >  #include "qemu/log.h"
>> >  #include "qemu/main-loop.h"
>> >  #include "qemu/module.h"
>> > +#include "qemu/option.h"
>> > +#include "qom/object_interfaces.h"
>> >  
>> >  #include "trace/control.h"
>> >  
>> > @@ -51,10 +53,26 @@ static void help(void)
>> >  "                         specify tracing options\n"
>> >  "  -V, --version          output version information and exit\n"
>> >  "\n"
>> > +"  --object <properties>  define a QOM object such as 'secret' for\n"
>> > +"                         passwords and/or encryption keys\n"
>> 
>> This is less helpful than qemu-system-FOO's help:
>> 
>> -object TYPENAME[,PROP1=VALUE1,...]
>>                 create a new object of type TYPENAME setting properties
>>                 in the order they are specified.  Note that the 'id'
>>                 property must be set.  These objects are placed in the
>>                 '/objects' path.
>
> Hm, yes. I took the description from the tools. I can switch to the vl.c
> one (should the tools, too?).
>
> But honestly, neither of the two is enough to tell anyone how to
> actually use this... Considering how many different objects there are,
> maybe the best we can do is referring to the man page for details.

For a simple program, --help can provide pretty much the same usage
information as the manual page.

For a beast like QEMU, that's hopeless.  But --help can still serve as a
quick reminder of syntax and such.

Another argument is consistency: as long as --help shows syntax for the
other options, it should probably show syntax for --object, too.

We can certainly discuss level of detail.  For instance,

    --blockdev [driver=]<driver>[,node-name=<N>][,discard=ignore|unmap]
               [,cache.direct=on|off][,cache.no-flush=on|off]
               [,read-only=on|off][,auto-read-only=on|off]
               [,force-share=on|off][,detect-zeroes=on|off|unmap]
               [,driver specific parameters...]
                           configure a block backend

is arguably hardly more useful than

    --blockdev [driver=]<driver>[,node-name=<node-name>][,<prop>=<value>,...]
                           configure a block backend

The screen space would arguably be better spend on explaining <driver>
and <node-name>.

By the way, we should pick *one* way to mark up variable parts, and
stick to it.  As it is, we have

    -machine [type=]name[,prop[=value][,...]]
    -object TYPENAME[,PROP1=VALUE1,...]
    -blockdev [driver=]driver[,node-name=N][,discard=ignore|unmap]

Frankly, this sucks.  Let's not mindlessly duplicate the suckage into
the storage daemon's help.

>> > +"\n"
>> >  QEMU_HELP_BOTTOM "\n",
>> >      error_get_progname());
>> >  }
>> >  
>> > +enum {
>> > +    OPTION_OBJECT = 256,
>> > +};
>> > +
>> > +static QemuOptsList qemu_object_opts = {
>> > +    .name = "object",
>> > +    .implied_opt_name = "qom-type",
>> > +    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
>> > +    .desc = {
>> > +        { }
>> > +    },
>> > +};
>> > +
>> 
>> Note for later: copied from vl.c.
>> 
>> >  static int process_options(int argc, char *argv[], Error **errp)
>> >  {
>> >      int c;
>> > @@ -63,6 +81,7 @@ static int process_options(int argc, char *argv[], Error **errp)
>> >  
>> >      static const struct option long_options[] = {
>> >          {"help", no_argument, 0, 'h'},
>> > +        {"object", required_argument, 0, OPTION_OBJECT},
>> >          {"version", no_argument, 0, 'V'},
>> >          {"trace", required_argument, NULL, 'T'},
>> >          {0, 0, 0, 0}
>> > @@ -88,6 +107,22 @@ static int process_options(int argc, char *argv[], Error **errp)
>> >              g_free(trace_file);
>> >              trace_file = trace_opt_parse(optarg);
>> >              break;
>> > +        case OPTION_OBJECT:
>> > +            {
>> > +                QemuOpts *opts;
>> > +                const char *type;
>> > +
>> > +                opts = qemu_opts_parse(&qemu_object_opts,
>> > +                                       optarg, true, &error_fatal);
>> > +                type = qemu_opt_get(opts, "qom-type");
>> > +
>> > +                if (user_creatable_print_help(type, opts)) {
>> > +                    exit(EXIT_SUCCESS);
>> > +                }
>> > +                user_creatable_add_opts(opts, &error_fatal);
>> > +                qemu_opts_del(opts);
>> > +                break;
>> > +            }
>> >          }
>> >      }
>> >      if (optind != argc) {
>> 
>> PATCH 01 duplicates case QEMU_OPTION_trace pretty much verbatim.  Makes
>> sense, as qemu-storage-daemon is basically qemu-system-FOO with "FOO"
>> and most "system" cut away.
>> 
>> This patch adds vl.c's case QEMU_OPTION_object in a much simpler form.
>> This is one of my least favourite options, and I'll tell you why below.
>> Let's compare the two versions.
>> 
>> vl.c:
>> 
>>             case QEMU_OPTION_object:
>>                 opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
>>                                                optarg, true);
>>                 if (!opts) {
>>                     exit(1);
>>                 }
>>                 break;
>> 
>> Further down:
>> 
>>     qemu_opts_foreach(qemu_find_opts("object"),
>>                       user_creatable_add_opts_foreach,
>>                       object_create_initial, &error_fatal);
>> 
>> Still further down:
>> 
>>     qemu_opts_foreach(qemu_find_opts("object"),
>>                       user_creatable_add_opts_foreach,
>>                       object_create_delayed, &error_fatal);
>> 
>> These are basically
>> 
>>     for opts in qemu_object_opts {
>>         type = qemu_opt_get(opts, "qom-type");
>>         if (type) {
>>             if (user_creatable_print_help(type, opts)) {
>>                 exit(0);
>>             }
>>             if (!predicate(type)) {
>>                 continue;
>>             }
>>         }
>>         obj = user_creatable_add_opts(opts, &error_fatal);
>>         object_unref(obj);
>>     }
>> 
>> where predicate(type) is true in exactly one of the two places for each
>> QOM type.
>> 
>> The reason for these gymnastics is to create objects at the right time
>> during startup, except there is no right time, but two.
>> 
>> Differences:
>> 
>> * Options are processed left to right without gymnastics.  Getting their
>>   order right is the user's problem.  I consider this an improvement.
>> 
>> * You use &qemu_object_opts instead of qemu_find_opts("object").  Also
>>   an improvement.
>> 
>> * You use qemu_opts_parse() instead of qemu_opts_parse_noisily().
>>   The latter can print help.  I failed to find a case where we lose help
>>   compared to qemu-system-FOO.  I didn't try very hard.
>
> I tried to reuse that code from qemu_opts_parse_noisily(), until I
> realised that it wasn't even used for -object.

What do you mean by "not used"?  vl.c:

            case QEMU_OPTION_object:
                opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
                                               optarg, true);
                if (!opts) {
                    exit(1);
                }
                break;

> I don't remember the details why qemu_opts_print_help() wasn't even
> called, but it's obvious that we can't lose anything from it:
> qemu_object_opts has an empty list of properties, it accepts anything.
> QemuOpts can't print any useful help when this is all the information it
> has.

I see.

Do we want to bake that knowledge into main()?  I guess your answer
would be "we already do, we call user_creatable_print_help()".

Shouldn't we do the same in both main()s then?

>> * You neglect to guard user_creatable_print_help():
>> 
>>     $ qemu-storage-daemon --object wrong=1,help
>>     Segmentation fault (core dumped)
>
> Thanks for catching this. (You don't even need the ",help" part, just
> --object wrong=1 is enough.)
>
>> * You neglect to object_unref().  I just double-checked the final
>>   reference count: it's 2.
>
> Hm, yes. Weird interface, no caller actually needs this reference.

Feel free to simplify.

>> These bugs shouldn't be hard to fix.
>> 
>> 
>> At this point you might wonder why I dislike this option so much.
>> vl.c's gymnastics are ugly, but not unusually ugly, and they're gone
>> here.  To explain my distaste, I have to go back a little bit.
>> 
>> Like quite a few options, --object is paired with QMP command, namely
>> object-add.  Both have the same parameters: QOM type, object ID, and
>> additional type-specific object properties.  There's a difference,
>> though: object-add wraps the latter in a 'props' object, while --object
>> does not.
>> 
>> QAPI schema:
>> 
>>     { 'command': 'object-add',
>>       'data': {'qom-type': 'str', 'id': 'str', '*props': 'any'} }
>> 
>> QAPIfying this part of the CLI isn't easy.
>> 
>> The obvious QAPIfied CLI buddy of object-add is incompatible to current
>> --object.  That's not a concern for the storage daemon.  But it's also
>> ugly, because object-add's nesting of the type-specific properties
>> within @props is.  In QMP, it's merely yet another pair of curlies.  In
>> the CLI, we get to prefix props. to each type-specific property.
>> 
>> If we want to give the storage daemon a QAPIfied command line from the
>> start (and I think we do), we'll have to decide how to address this
>> issue, and possibly more (I'm only at PATCH 02/18).
>> 
>> We have a long history of rather careless interface design, and now some
>> of these chickens come home to roost.
>
> On IRC, we discussed last week that we could turn object-add into a
> 'gen': false command and accept things both as options in props and as
> flat options on the top level.

... to get rid of the nesting both for qemu-system-FOO (where we need
backward compatibility) and qemu-storage-daemon (where we don't).

We can also discuss getting rid of it only for qemu-storage-daemon.

> However, looking at this again, I'm afraid we forgot the context while
> discussing specifics: How would this be used in a command line parser?
>
> We don't start with a QDict here, but with a string. Getting a QDict
> that could serve as an input to a modified qmp_object_add() would still
> involve going through QemuOpts for parsing the option string, and then
> converting it to a QDict. Using a visitor isn't possible with '*props':
> 'any' and even less so with 'gen': false.
>
> So would this really improve things? Or do we have to wait until we have
> an actual schema for object-add before calling qmp_object_add() actually
> makes sense?

Good points.

qmp_object_add(), hmp_object_add() and vl.c's main() each use their own
interface to a common core.

QMP command handlers accept arguments specified in the schema as
separate C arguments.  For qmp_object_add(), that's @qom-type, @id (both
strings) and @props (a QDict).  It uses user_creatable_add_type() to
create an instance of QOM class @qom-type, set QOM properties for @props
with a QObject input visitor, insert into the QOM composition tree as
directed by @id.

This is simply how QMP commands work.  They parse JSON text into a
QDict, then feed it to the QObject input visitor to convert to native C
types.  The only slightly unusual thing is that when we feed the QDict
to the visitor in the generated qmp_marshal_object_add(), the visitor
passes through @props verbatim, because it's of type 'any', leaving the
conversion to native C types to the command handler.

With 'gen': false, we bypass qmp_marshal_object_add().  qmp_object_add()
gets the QDict, and does all the conversion to C types work.  Same as
now, plus two more visit_type_str() for @qom-type and @id.

main() parses the option argument as QemuOpts "monitor".  It uses
user_creatable_add_opts(), which splits it into @qom-type, @id and
@props for user_creatable_add_type(), then calls it with the options
visitor.

This is how the options visitor wants to be used.  We parse a
key=value,... string into a QemuOpts, then feed it to the options
visitor.

When I QAPIfied -blockdev (crudely!), I didn't use the options visitor,
because it is by design too limited for the job.  I created a new flow
instead: parse a key=value,... string into a QDict with keyval_parse(),
then feed it to the QObject input visitor.

This leads me to the flow I'd like us to try in the storage daemon:
parse into a QDict with keyval_parse(), feed to the 'gen': false
qmp_object_add().

Does that make some sense?

I've glossed over hmp_object_add() so far, because it's tangential to
the problem at hand.  Just for completeness (and laughs, possibly
bitter): HMP command handlers accept arguments specified in .hx as a
single QDict.  For hmp_object_add(), that's a string parsed as QemuOpts
"monitor" converted to QDict.  hmp_object_add() converts it right back
to QemuOpts "monitor", then uses user_creatable_add_opts().

Ugh!  Need I say more?