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

[Markus Armbruster]

Kevin Wolf  writes:

> Am 07.11.2019 um 21:36 hat Markus Armbruster geschrieben:
>> Kevin Wolf  writes:
>> 
>> > Add a command line option to create user-creatable QOM objects.
>> >
>> > Signed-off-by: Kevin Wolf 
>> > ---
>> >  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   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=][,node-name=][,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=][,node-name=][,=,...]
   configure a block backend

The screen space would arguably be better spend on explaining 
and .

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 

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

[Kevin Wolf]

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

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

[Markus Armbruster]

Kevin Wolf  writes:

> Add a command line option to create user-creatable QOM objects.
>
> Signed-off-by: Kevin Wolf 
> ---
>  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   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_

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

[Kevin Wolf]

Add a command line option to create user-creatable QOM objects.

Signed-off-by: Kevin Wolf 
---
 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   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