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

[Markus Armbruster <armbru@redhat.com>]

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?