Kevin Wolf <kw...@redhat.com> writes:
> Am 07.11.2019 um 21:36 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kw...@redhat.com> writes:
>>
>> > Add a command line option to create user-creatable QOM objects.
>> >
>> > Signed-off-by: Kevin Wolf <kw...@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?
