Patchwork [for-1.4,v2,4/6] qemu-option: Disable two helpful messages that got broken recently

login
register
mail settings
Submitter Markus Armbruster
Date Feb. 8, 2013, 4:17 p.m.
Message ID <1360340232-4670-5-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/219211/
State New
Headers show

Comments

Markus Armbruster - Feb. 8, 2013, 4:17 p.m.
commit 8be7e7e4 and commit ec7b2ccb messed up the ordering of error
message and the helpful explanation that should follow it, like this:

    $ qemu-system-x86_64 --nodefaults -S --vnc :0 --chardev null,id=,
    Identifiers consist of letters, digits, '-', '.', '_', starting with a letter.
    qemu-system-x86_64: -chardev null,id=,: Parameter 'id' expects an identifier

    $ qemu-system-x86_64 --nodefaults -S --vnc :0 --machine kvm_shadow_mem=dunno
    You may use k, M, G or T suffixes for kilobytes, megabytes, gigabytes and terabytes.
    qemu-system-x86_64: -machine kvm_shadow_mem=dunno: Parameter 'kvm_shadow_mem' expects a size

Pity.  Disable them for now.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/qemu-option.c | 4 ++++
 1 file changed, 4 insertions(+)
Luiz Capitulino - Feb. 8, 2013, 5:53 p.m.
On Fri,  8 Feb 2013 17:17:10 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> commit 8be7e7e4 and commit ec7b2ccb messed up the ordering of error
> message and the helpful explanation that should follow it, like this:
> 
>     $ qemu-system-x86_64 --nodefaults -S --vnc :0 --chardev null,id=,
>     Identifiers consist of letters, digits, '-', '.', '_', starting with a letter.
>     qemu-system-x86_64: -chardev null,id=,: Parameter 'id' expects an identifier
> 
>     $ qemu-system-x86_64 --nodefaults -S --vnc :0 --machine kvm_shadow_mem=dunno
>     You may use k, M, G or T suffixes for kilobytes, megabytes, gigabytes and terabytes.
>     qemu-system-x86_64: -machine kvm_shadow_mem=dunno: Parameter 'kvm_shadow_mem' expects a size
> 
> Pity.  Disable them for now.

Oops, sorry. I think I'd prefer to drop them, but as this fixes the problem:

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

Also, the real problem here is that general functions like parse_option_size()
should never assume/try to guess in which context they're running. So, the
best solution here is either to have a general enough error message that's
not tied to any context, or let up layers (say vl.c) concatenate the
context-dependent part of the error message.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  util/qemu-option.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index c12e724..5a1d03c 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -231,8 +231,10 @@ static void parse_option_size(const char *name, const char *value,
>              break;
>          default:
>              error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, "a size");
> +#if 0 /* conversion from qerror_report() to error_set() broke this: */
>              error_printf_unless_qmp("You may use k, M, G or T suffixes for "
>                      "kilobytes, megabytes, gigabytes and terabytes.\n");
> +#endif
>              return;
>          }
>      } else {
> @@ -771,7 +773,9 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
>      if (id) {
>          if (!id_wellformed(id)) {
>              error_set(errp,QERR_INVALID_PARAMETER_VALUE, "id", "an identifier");
> +#if 0 /* conversion from qerror_report() to error_set() broke this: */
>              error_printf_unless_qmp("Identifiers consist of letters, digits, '-', '.', '_', starting with a letter.\n");
> +#endif
>              return NULL;
>          }
>          opts = qemu_opts_find(list, id);
Markus Armbruster - Feb. 8, 2013, 6:58 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri,  8 Feb 2013 17:17:10 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> commit 8be7e7e4 and commit ec7b2ccb messed up the ordering of error
>> message and the helpful explanation that should follow it, like this:
>> 
>>     $ qemu-system-x86_64 --nodefaults -S --vnc :0 --chardev null,id=,
>>     Identifiers consist of letters, digits, '-', '.', '_', starting
>> with a letter.
>>     qemu-system-x86_64: -chardev null,id=,: Parameter 'id' expects
>> an identifier
>> 
>>     $ qemu-system-x86_64 --nodefaults -S --vnc :0 --machine
>> kvm_shadow_mem=dunno
>>     You may use k, M, G or T suffixes for kilobytes, megabytes,
>> gigabytes and terabytes.
>>     qemu-system-x86_64: -machine kvm_shadow_mem=dunno: Parameter
>> kvm_shadow_mem' expects a size
>> 
>> Pity.  Disable them for now.
>
> Oops, sorry. I think I'd prefer to drop them, but as this fixes the problem:
>
> Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
>
> Also, the real problem here is that general functions like parse_option_size()
> should never assume/try to guess in which context they're running. So, the
> best solution here is either to have a general enough error message that's
> not tied to any context, or let up layers (say vl.c) concatenate the
> context-dependent part of the error message.

I'm open to suggestions on how to better code the pattern "report an
error (a short string without newlines) together with some explanation
or hint (possibly longer string, newlines okay).
Luiz Capitulino - Feb. 8, 2013, 7:16 p.m.
On Fri, 08 Feb 2013 19:58:42 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Fri,  8 Feb 2013 17:17:10 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> commit 8be7e7e4 and commit ec7b2ccb messed up the ordering of error
> >> message and the helpful explanation that should follow it, like this:
> >> 
> >>     $ qemu-system-x86_64 --nodefaults -S --vnc :0 --chardev null,id=,
> >>     Identifiers consist of letters, digits, '-', '.', '_', starting
> >> with a letter.
> >>     qemu-system-x86_64: -chardev null,id=,: Parameter 'id' expects
> >> an identifier
> >> 
> >>     $ qemu-system-x86_64 --nodefaults -S --vnc :0 --machine
> >> kvm_shadow_mem=dunno
> >>     You may use k, M, G or T suffixes for kilobytes, megabytes,
> >> gigabytes and terabytes.
> >>     qemu-system-x86_64: -machine kvm_shadow_mem=dunno: Parameter
> >> kvm_shadow_mem' expects a size
> >> 
> >> Pity.  Disable them for now.
> >
> > Oops, sorry. I think I'd prefer to drop them, but as this fixes the problem:
> >
> > Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
> >
> > Also, the real problem here is that general functions like parse_option_size()
> > should never assume/try to guess in which context they're running. So, the
> > best solution here is either to have a general enough error message that's
> > not tied to any context, or let up layers (say vl.c) concatenate the
> > context-dependent part of the error message.
> 
> I'm open to suggestions on how to better code the pattern "report an
> error (a short string without newlines) together with some explanation
> or hint (possibly longer string, newlines okay).

The real problem here is that the k, M, G suffixes, for example, are not
good to be reported by QMP. So maybe we should refactor the code in a way
that we separate what's done in QMP from what is done in HMP/command-line.
Markus Armbruster - Feb. 8, 2013, 7:34 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 08 Feb 2013 19:58:42 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Fri,  8 Feb 2013 17:17:10 +0100
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> commit 8be7e7e4 and commit ec7b2ccb messed up the ordering of error
>> >> message and the helpful explanation that should follow it, like this:
>> >> 
>> >>     $ qemu-system-x86_64 --nodefaults -S --vnc :0 --chardev null,id=,
>> >>     Identifiers consist of letters, digits, '-', '.', '_', starting
>> >> with a letter.
>> >>     qemu-system-x86_64: -chardev null,id=,: Parameter 'id' expects
>> >> an identifier
>> >> 
>> >>     $ qemu-system-x86_64 --nodefaults -S --vnc :0 --machine
>> >> kvm_shadow_mem=dunno
>> >>     You may use k, M, G or T suffixes for kilobytes, megabytes,
>> >> gigabytes and terabytes.
>> >>     qemu-system-x86_64: -machine kvm_shadow_mem=dunno: Parameter
>> >> kvm_shadow_mem' expects a size
>> >> 
>> >> Pity.  Disable them for now.
>> >
>> > Oops, sorry. I think I'd prefer to drop them, but as this fixes the problem:
>> >
>> > Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
>> >
>> > Also, the real problem here is that general functions like
>> > parse_option_size()
>> > should never assume/try to guess in which context they're running. So, the
>> > best solution here is either to have a general enough error message that's
>> > not tied to any context, or let up layers (say vl.c) concatenate the
>> > context-dependent part of the error message.
>> 
>> I'm open to suggestions on how to better code the pattern "report an
>> error (a short string without newlines) together with some explanation
>> or hint (possibly longer string, newlines okay).
>
> The real problem here is that the k, M, G suffixes, for example, are not
> good to be reported by QMP. So maybe we should refactor the code in a way
> that we separate what's done in QMP from what is done in HMP/command-line.

Isn't it separated already?  parse_option_size() is used when parsing
key=value,...  Such strings should not exist in QMP.  If they do, it's a
design bug.
Luiz Capitulino - Feb. 13, 2013, 7:20 p.m.
On Fri, 08 Feb 2013 20:34:20 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> > The real problem here is that the k, M, G suffixes, for example, are not
> > good to be reported by QMP. So maybe we should refactor the code in a way
> > that we separate what's done in QMP from what is done in HMP/command-line.
> 
> Isn't it separated already?  parse_option_size() is used when parsing
> key=value,...  Such strings should not exist in QMP.  If they do, it's a
> design bug.

No and no. Such strings don't exist in QMP as far as can tell (see bugs
below though), but parse_option_size() is theoretically "present" in a
possible QMP call stack:

qemu_opts_from_dict_1()
  qemu_opt_set_err()
    opt_set()
      qemu_opt_paser()
        parse_option_size()

I can't tell if this will ever happen because qemu_opts_from_dict_1()
restricts the call to qemu_opt_set_err() to certain values, but the
fact that it's not clear is an indication that a better separation is
necessary.

Now, I think I've found at least two bugs. The first one is the
netdev_add doc in the schema, which states that we do accept key=value
strings. The problem is here is that that's about the C API, on the
wire we act as before (ie. accepting each key as a separate argument).
The qapi-schame.json is more or less format-independent, so I'm not
exactly sure what's the best way to describe commands using QemuOpts
the way QMP uses it.

The second bug is that I entirely ignored how set_option_paramater()
handles errors when doing parse_option_size() conversion to Error **
and also when converting bdrv_img_create(). The end result is that
we can report an error twice, once with error_set() and later with
qerror_report() (or vice-versa). Shouldn't hurt on QMP as it knows
how to deal with this, on HMP and command-line we get complementary
error messages if we're lucky.

I'm very surprised with my mistakes on the second bug (although some
of the mess with fprintf() was already there), but I honestly think we
should defer this to 1.5 (and I can do it myself next week).
Markus Armbruster - Feb. 14, 2013, 9:45 a.m.
[Note cc: +Laszlo, +Anthony, -qemu-trivial]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 08 Feb 2013 20:34:20 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> > The real problem here is that the k, M, G suffixes, for example, are not
>> > good to be reported by QMP. So maybe we should refactor the code in a way
>> > that we separate what's done in QMP from what is done in HMP/command-line.
>> 
>> Isn't it separated already?  parse_option_size() is used when parsing
>> key=value,...  Such strings should not exist in QMP.  If they do, it's a
>> design bug.
>
> No and no. Such strings don't exist in QMP as far as can tell (see bugs
> below though), but parse_option_size() is theoretically "present" in a
> possible QMP call stack:
>
> qemu_opts_from_dict_1()
>   qemu_opt_set_err()
>     opt_set()
>       qemu_opt_paser()
>         parse_option_size()
>
> I can't tell if this will ever happen because qemu_opts_from_dict_1()
> restricts the call to qemu_opt_set_err() to certain values, but the
> fact that it's not clear is an indication that a better separation is
> necessary.

Permit me two detours.

One, qemu_opt_set_err() is a confusing name.  I doesn't set an error.
It attempts to set an option value, using the conventional Error **
parameter for error reporting.  Its buddy qemu_opt_set() does the same,
except it reports errors to the user and returns only success/failure.

Two, qemu_opts_from_qdict() should be taken out and shot (I may say
that, because I created it).  I created it because I had to go from
QDicts I get from QMP to internal APIs that want QemuOpts, specifically
qdev_device_add().

qdev_device_add() takes QemuOpts because that's the only tool we had at
the time for passing dictionaries around.  Made enough sense then:
command line gives us QemuOpts already, so why not just pass them on.

Still made sense for the human monitor command: give it an argument just
like -device's option argument, parse it the same way, done.

It became awkward with QMP.  Perhaps I should've switched
qdev_device_add() to QDict then, so the QMP command handler can use it
directly.  Instead, I simply converted from QDict to QemuOpts.

Now, to build hmp_device_add() on top of qmp_device_add(), we'd have to
convert first from QemuOpts to QDict and then back to QemuOpts.  Blech.
Instead, we made do_device_add() go straight to qdev_device_add().  Same
for hmp_netdev_add(): it goes straight to netdev_add().

QemuOpts is a bad fit for general interfaces, because it's really
designed for accumulating command line options for later use.  Features
useful there get in the way, e.g. how QemuOptsList serves both as schema
and as the single list of QemuOpts.  Features not needed there are
missing, e.g. signed and floating-point numbers.

End of detours, back to your questions.

I guess weird things can happen with qemu_opts_from_qdict(), at least in
theory.

For each (key, value) in the QDict, qemu_opts_from_qdict() converts
value to string according to its qtype_code.  The string then gets
parsed according to key's QemuOptType.  Yes, that means you can pass a
QString to QEMU_OPT_SIZE option, and get the suffixes interpreted.

However, we've only used qemu_opts_from_qdict() with QemuOptsLists that
have empty desc[]!  Specifically: qemu_netdev_opts and qemu_device_opts.
Without desc, qemu_opt_parse() does nothing, and QemuOpts holds just
string values.  Actual parsing left to the consumer.

Two consumers: qdev_device_add() and netdev_add().

netdev_add() uses QAPI wizardry to create the appropriate object from
the QemuOpts.  The parsing is done by visitors.  Things get foggy for me
from there on, but it looks like one of them, opts_type_size(), can
parse size suffixes, which is inappropriate for QMP.  A quick test
confirms that this is indeed possible:

    {"execute": "netdev_add","arguments": {"type":"tap","id":"net.1",
    "sndbuf": "8k" }}

Sets NetdevTapOptions member sndbuf to 8192.

To sum up, we go from QDict delivered by the JSON parser via QemuOpts to
QAPI object, with a few cock-ups along the way.  Ugh.

By the way, the JSON schema reads

    { 'command': 'netdev_add',
      'data': {'type': 'str', 'id': 'str', '*props': '**'},
      'gen': 'no' }

I'll be hanged if I know what '**' means.

qdev_device_add() uses a few well-known options itself, and they're all
strings.  The others it passes on to qdev_prop_parse().  This permits
some weirdness like passing PCI device's addr property as number in QMP,
even though it's nominally a string of the form SLOT[.FN].

No JSON schema, because device_add hasn't been qapified (Laszlo's
working on it now).

> Now, I think I've found at least two bugs. The first one is the
> netdev_add doc in the schema, which states that we do accept key=value
> strings. The problem is here is that that's about the C API, on the
> wire we act as before (ie. accepting each key as a separate argument).
> The qapi-schame.json is more or less format-independent, so I'm not
> exactly sure what's the best way to describe commands using QemuOpts
> the way QMP uses it.

Netdev could be done without this key=value business in the schema.  We
have just a few backends, and they're well-known, so we could just
collect them all in a union, like Gerd did for chardev backends.

Devices are hairier.  For a union approach, we'd have to include a
schema for every device model.  Dunno.

> The second bug is that I entirely ignored how set_option_paramater()
> handles errors when doing parse_option_size() conversion to Error **
> and also when converting bdrv_img_create(). The end result is that
> we can report an error twice, once with error_set() and later with
> qerror_report() (or vice-versa). Shouldn't hurt on QMP as it knows
> how to deal with this, on HMP and command-line we get complementary
> error messages if we're lucky.

Example?  Fixable?

> I'm very surprised with my mistakes on the second bug (although some
> of the mess with fprintf() was already there), but I honestly think we
> should defer this to 1.5 (and I can do it myself next week).

Stick a fork in 1.4, it's done.
Luiz Capitulino - Feb. 14, 2013, 12:05 p.m.
On Thu, 14 Feb 2013 10:45:22 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> [Note cc: +Laszlo, +Anthony, -qemu-trivial]
> 
> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Fri, 08 Feb 2013 20:34:20 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> > The real problem here is that the k, M, G suffixes, for example, are not
> >> > good to be reported by QMP. So maybe we should refactor the code in a way
> >> > that we separate what's done in QMP from what is done in HMP/command-line.
> >> 
> >> Isn't it separated already?  parse_option_size() is used when parsing
> >> key=value,...  Such strings should not exist in QMP.  If they do, it's a
> >> design bug.
> >
> > No and no. Such strings don't exist in QMP as far as can tell (see bugs
> > below though), but parse_option_size() is theoretically "present" in a
> > possible QMP call stack:
> >
> > qemu_opts_from_dict_1()
> >   qemu_opt_set_err()
> >     opt_set()
> >       qemu_opt_paser()
> >         parse_option_size()
> >
> > I can't tell if this will ever happen because qemu_opts_from_dict_1()
> > restricts the call to qemu_opt_set_err() to certain values, but the
> > fact that it's not clear is an indication that a better separation is
> > necessary.
> 
> Permit me two detours.
> 
> One, qemu_opt_set_err() is a confusing name.  I doesn't set an error.

It takes an Error ** object and it was introduced to avoid having
to convert qemu_opt_set() to take an Error ** object, as this would
multiply the work required to get netdev_add converted to the qapi.

Now, I pass the bikeshed :)

> Now, to build hmp_device_add() on top of qmp_device_add(), we'd have to
> convert first from QemuOpts to QDict and then back to QemuOpts.  Blech.
> Instead, we made do_device_add() go straight to qdev_device_add().  Same
> for hmp_netdev_add(): it goes straight to netdev_add().

Yes, the idea was to have netdev_add() and add frontends to hmp
and qmp. More on this below.

> netdev_add() uses QAPI wizardry to create the appropriate object from
> the QemuOpts.  The parsing is done by visitors.  Things get foggy for me
> from there on, but it looks like one of them, opts_type_size(), can
> parse size suffixes, which is inappropriate for QMP.  A quick test
> confirms that this is indeed possible:
> 
>     {"execute": "netdev_add","arguments": {"type":"tap","id":"net.1",
>     "sndbuf": "8k" }}
> 
> Sets NetdevTapOptions member sndbuf to 8192.

Well, I've just tested this with commit 783e9b4, which is before
netdev_add conversion to the qapi, and the command above just works.

Didn't check if sndbuf is actually set to 8192, but this shows that
we've always accepted such a command.

> To sum up, we go from QDict delivered by the JSON parser via QemuOpts to
> QAPI object, with a few cock-ups along the way.  Ugh.
> 
> By the way, the JSON schema reads
> 
>     { 'command': 'netdev_add',
>       'data': {'type': 'str', 'id': 'str', '*props': '**'},
>       'gen': 'no' }
> 
> I'll be hanged if I know what '**' means.

For QMP on the wire it means that the command accepts a bunch of
arguments not specified in the schema.

Yes, it's a dirty trick. Long story short: we decide to maintain
QemuOpts usage in both HMP and QMP to speed up netdev_add conversion.

> qdev_device_add() uses a few well-known options itself, and they're all
> strings.  The others it passes on to qdev_prop_parse().  This permits
> some weirdness like passing PCI device's addr property as number in QMP,
> even though it's nominally a string of the form SLOT[.FN].
> 
> No JSON schema, because device_add hasn't been qapified (Laszlo's
> working on it now).
> 
> > Now, I think I've found at least two bugs. The first one is the
> > netdev_add doc in the schema, which states that we do accept key=value
> > strings. The problem is here is that that's about the C API, on the
> > wire we act as before (ie. accepting each key as a separate argument).
> > The qapi-schame.json is more or less format-independent, so I'm not
> > exactly sure what's the best way to describe commands using QemuOpts
> > the way QMP uses it.
> 
> Netdev could be done without this key=value business in the schema.  We
> have just a few backends, and they're well-known, so we could just
> collect them all in a union, like Gerd did for chardev backends.

I honestly don't know if this is a good idea, but should be possible
to change it if you're willing to.

> Devices are hairier.  For a union approach, we'd have to include a
> schema for every device model.  Dunno.

IMHO, right now going fast is more important than doing things
with perfection (unless you can do perfection in no time, of course),
because the qapi conversions already took a lot more than expected
and it's delaying very good stuff (like the new qmp server, and dropping
the *.hx files and old QMP code).

So, I wouldn't bother doing old crap commands perfect. Specially when
replacements are on the way.

> > The second bug is that I entirely ignored how set_option_paramater()
> > handles errors when doing parse_option_size() conversion to Error **
> > and also when converting bdrv_img_create(). The end result is that
> > we can report an error twice, once with error_set() and later with
> > qerror_report() (or vice-versa). Shouldn't hurt on QMP as it knows
> > how to deal with this, on HMP and command-line we get complementary
> > error messages if we're lucky.
> 
> Example?  Fixable?

Of course it's fixable, everything is fixable :)

qmp_drive_mirror()
  bdrv_img()
    set_option_paramater()
    
> > I'm very surprised with my mistakes on the second bug (although some
> > of the mess with fprintf() was already there), but I honestly think we
> > should defer this to 1.5 (and I can do it myself next week).
> 
> Stick a fork in 1.4, it's done.

No, I honestly think that at this point in time we should be fixing bugs
with proven end-user impact and serious regressions.

I consider bikeshedding and fixing small glitches present in more than
one release to be an abuse for a hard-freeze.
Markus Armbruster - Feb. 14, 2013, 1:31 p.m.
[Some quoted material restored]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 14 Feb 2013 10:45:22 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> [Note cc: +Laszlo, +Anthony, -qemu-trivial]
>> 
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Fri, 08 Feb 2013 20:34:20 +0100
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> > The real problem here is that the k, M, G suffixes, for example, are not
>> >> > good to be reported by QMP. So maybe we should refactor the code in a way
>> >> > that we separate what's done in QMP from what is done in
>> >> > HMP/command-line.
>> >> 
>> >> Isn't it separated already?  parse_option_size() is used when parsing
>> >> key=value,...  Such strings should not exist in QMP.  If they do, it's a
>> >> design bug.
>> >
>> > No and no. Such strings don't exist in QMP as far as can tell (see bugs
>> > below though), but parse_option_size() is theoretically "present" in a
>> > possible QMP call stack:
>> >
>> > qemu_opts_from_dict_1()
>> >   qemu_opt_set_err()
>> >     opt_set()
>> >       qemu_opt_paser()
>> >         parse_option_size()
>> >
>> > I can't tell if this will ever happen because qemu_opts_from_dict_1()
>> > restricts the call to qemu_opt_set_err() to certain values, but the
>> > fact that it's not clear is an indication that a better separation is
>> > necessary.
>> 
>> Permit me two detours.
>> 
>> One, qemu_opt_set_err() is a confusing name.  I doesn't set an error.
>
> It takes an Error ** object and it was introduced to avoid having
> to convert qemu_opt_set() to take an Error ** object, as this would
> multiply the work required to get netdev_add converted to the qapi.
>
> Now, I pass the bikeshed :)

I get why it's there, I just find its name confusing.

> [...]
>> Now, to build hmp_device_add() on top of qmp_device_add(), we'd have to
>> convert first from QemuOpts to QDict and then back to QemuOpts.  Blech.
>> Instead, we made do_device_add() go straight to qdev_device_add().  Same
>> for hmp_netdev_add(): it goes straight to netdev_add().
>
> Yes, the idea was to have netdev_add() and add frontends to hmp
> and qmp. More on this below.
>
> [...]
>> I guess weird things can happen with qemu_opts_from_qdict(), at least in
>> theory.
>> 
>> For each (key, value) in the QDict, qemu_opts_from_qdict() converts
>> value to string according to its qtype_code.  The string then gets
>> parsed according to key's QemuOptType.  Yes, that means you can pass a
>> QString to QEMU_OPT_SIZE option, and get the suffixes interpreted.
>> 
>> However, we've only used qemu_opts_from_qdict() with QemuOptsLists that
>> have empty desc[]!  Specifically: qemu_netdev_opts and qemu_device_opts.
>> Without desc, qemu_opt_parse() does nothing, and QemuOpts holds just
>> string values.  Actual parsing left to the consumer.
>> 
>> Two consumers: qdev_device_add() and netdev_add().
>> 
>> netdev_add() uses QAPI wizardry to create the appropriate object from
>> the QemuOpts.  The parsing is done by visitors.  Things get foggy for me
>> from there on, but it looks like one of them, opts_type_size(), can
>> parse size suffixes, which is inappropriate for QMP.  A quick test
>> confirms that this is indeed possible:
>> 
>>     {"execute": "netdev_add","arguments": {"type":"tap","id":"net.1",
>>     "sndbuf": "8k" }}
>> 
>> Sets NetdevTapOptions member sndbuf to 8192.
>
> Well, I've just tested this with commit 783e9b4, which is before
> netdev_add conversion to the qapi, and the command above just works.
>
> Didn't check if sndbuf is actually set to 8192, but this shows that
> we've always accepted such a command.

Plausible.  Nevertheless, we really shouldn't.

>> To sum up, we go from QDict delivered by the JSON parser via QemuOpts to
>> QAPI object, with a few cock-ups along the way.  Ugh.
>> 
>> By the way, the JSON schema reads
>> 
>>     { 'command': 'netdev_add',
>>       'data': {'type': 'str', 'id': 'str', '*props': '**'},
>>       'gen': 'no' }
>> 
>> I'll be hanged if I know what '**' means.
>
> For QMP on the wire it means that the command accepts a bunch of
> arguments not specified in the schema.

Thanks!  Is the schema language documented anywhere?

> Yes, it's a dirty trick. Long story short: we decide to maintain
> QemuOpts usage in both HMP and QMP to speed up netdev_add conversion.

I don't think '**' is as dirty as you make it sound.  It's simply a way
to relax the rigidity of the schema.  I got no problem with that.

>> qdev_device_add() uses a few well-known options itself, and they're all
>> strings.  The others it passes on to qdev_prop_parse().  This permits
>> some weirdness like passing PCI device's addr property as number in QMP,
>> even though it's nominally a string of the form SLOT[.FN].
>> 
>> No JSON schema, because device_add hasn't been qapified (Laszlo's
>> working on it now).
>> 
>> > Now, I think I've found at least two bugs. The first one is the
>> > netdev_add doc in the schema, which states that we do accept key=value
>> > strings. The problem is here is that that's about the C API, on the
>> > wire we act as before (ie. accepting each key as a separate argument).
>> > The qapi-schame.json is more or less format-independent, so I'm not
>> > exactly sure what's the best way to describe commands using QemuOpts
>> > the way QMP uses it.
>> 
>> Netdev could be done without this key=value business in the schema.  We
>> have just a few backends, and they're well-known, so we could just
>> collect them all in a union, like Gerd did for chardev backends.
>
> I honestly don't know if this is a good idea, but should be possible
> to change it if you're willing to.

chardev-add: the schema defines an object type for each backend
(ChardevFile, ChardevSocket, ...), and collects them together in
discriminated union ChardevBackend.  chardev_add takes that union.
Thus, the schema accurately describes chardev-add's arguments, and the
generated marshaller takes care of parsing chardev-add arguments into
the appropriate objects.

netdev_add: the schema defines an object type for each backend
(NetdevNoneOptions, NetdevUserOptions, ...).  netdev_add doesn't use
them, but takes arbitrary parameters instead.  The connection is made in
code, which stuffs the parameters in a QemuOpts (losing all JSON type
information), then takes them out again to stuff them into the
appropriate object type.  A job the marshaller should be able to do for
us.

For me, the way chardev-add works makes a whole lot more sense, and I
think we should clean up netdev_add to work the same way.
Unfortunately, I can't commit to this cleanup job right now.

>> Devices are hairier.  For a union approach, we'd have to include a
>> schema for every device model.  Dunno.
>
> IMHO, right now going fast is more important than doing things
> with perfection (unless you can do perfection in no time, of course),
> because the qapi conversions already took a lot more than expected
> and it's delaying very good stuff (like the new qmp server, and dropping
> the *.hx files and old QMP code).
>
> So, I wouldn't bother doing old crap commands perfect. Specially when
> replacements are on the way.

I'm not asking for perfection.  You wondered whether we can hit certain
errors with qemu_opts_from_qdict(), and I showed you how we can, and the
unintended misfeatures that make it possible.

As far as I can tell, we can hit them only with netdev_add, not with
device_add.  Happy to explain in more detail.

>> > The second bug is that I entirely ignored how set_option_paramater()
>> > handles errors when doing parse_option_size() conversion to Error **
>> > and also when converting bdrv_img_create(). The end result is that
>> > we can report an error twice, once with error_set() and later with
>> > qerror_report() (or vice-versa). Shouldn't hurt on QMP as it knows
>> > how to deal with this, on HMP and command-line we get complementary
>> > error messages if we're lucky.
>> 
>> Example?  Fixable?
>
> Of course it's fixable, everything is fixable :)
>
> qmp_drive_mirror()
>   bdrv_img()
>     set_option_paramater()
>     
>> > I'm very surprised with my mistakes on the second bug (although some
>> > of the mess with fprintf() was already there), but I honestly think we
>> > should defer this to 1.5 (and I can do it myself next week).
>> 
>> Stick a fork in 1.4, it's done.
>
> No, I honestly think that at this point in time we should be fixing bugs
> with proven end-user impact and serious regressions.

Note even that, it's *done*.  Finished.  Fertig.  Finito.  Game over;
insert coin to play again ;-P

> I consider bikeshedding and fixing small glitches present in more than
> one release to be an abuse for a hard-freeze.

Misunderstanding?
Luiz Capitulino - Feb. 14, 2013, 4:36 p.m.
On Thu, 14 Feb 2013 14:31:50 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> [Some quoted material restored]
> 
> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Thu, 14 Feb 2013 10:45:22 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> [Note cc: +Laszlo, +Anthony, -qemu-trivial]
> >> 
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > On Fri, 08 Feb 2013 20:34:20 +0100
> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >
> >> >> > The real problem here is that the k, M, G suffixes, for example, are not
> >> >> > good to be reported by QMP. So maybe we should refactor the code in a way
> >> >> > that we separate what's done in QMP from what is done in
> >> >> > HMP/command-line.
> >> >> 
> >> >> Isn't it separated already?  parse_option_size() is used when parsing
> >> >> key=value,...  Such strings should not exist in QMP.  If they do, it's a
> >> >> design bug.
> >> >
> >> > No and no. Such strings don't exist in QMP as far as can tell (see bugs
> >> > below though), but parse_option_size() is theoretically "present" in a
> >> > possible QMP call stack:
> >> >
> >> > qemu_opts_from_dict_1()
> >> >   qemu_opt_set_err()
> >> >     opt_set()
> >> >       qemu_opt_paser()
> >> >         parse_option_size()
> >> >
> >> > I can't tell if this will ever happen because qemu_opts_from_dict_1()
> >> > restricts the call to qemu_opt_set_err() to certain values, but the
> >> > fact that it's not clear is an indication that a better separation is
> >> > necessary.
> >> 
> >> Permit me two detours.
> >> 
> >> One, qemu_opt_set_err() is a confusing name.  I doesn't set an error.
> >
> > It takes an Error ** object and it was introduced to avoid having
> > to convert qemu_opt_set() to take an Error ** object, as this would
> > multiply the work required to get netdev_add converted to the qapi.
> >
> > Now, I pass the bikeshed :)
> 
> I get why it's there, I just find its name confusing.
> 
> > [...]
> >> Now, to build hmp_device_add() on top of qmp_device_add(), we'd have to
> >> convert first from QemuOpts to QDict and then back to QemuOpts.  Blech.
> >> Instead, we made do_device_add() go straight to qdev_device_add().  Same
> >> for hmp_netdev_add(): it goes straight to netdev_add().
> >
> > Yes, the idea was to have netdev_add() and add frontends to hmp
> > and qmp. More on this below.
> >
> > [...]
> >> I guess weird things can happen with qemu_opts_from_qdict(), at least in
> >> theory.
> >> 
> >> For each (key, value) in the QDict, qemu_opts_from_qdict() converts
> >> value to string according to its qtype_code.  The string then gets
> >> parsed according to key's QemuOptType.  Yes, that means you can pass a
> >> QString to QEMU_OPT_SIZE option, and get the suffixes interpreted.
> >> 
> >> However, we've only used qemu_opts_from_qdict() with QemuOptsLists that
> >> have empty desc[]!  Specifically: qemu_netdev_opts and qemu_device_opts.
> >> Without desc, qemu_opt_parse() does nothing, and QemuOpts holds just
> >> string values.  Actual parsing left to the consumer.
> >> 
> >> Two consumers: qdev_device_add() and netdev_add().
> >> 
> >> netdev_add() uses QAPI wizardry to create the appropriate object from
> >> the QemuOpts.  The parsing is done by visitors.  Things get foggy for me
> >> from there on, but it looks like one of them, opts_type_size(), can
> >> parse size suffixes, which is inappropriate for QMP.  A quick test
> >> confirms that this is indeed possible:
> >> 
> >>     {"execute": "netdev_add","arguments": {"type":"tap","id":"net.1",
> >>     "sndbuf": "8k" }}
> >> 
> >> Sets NetdevTapOptions member sndbuf to 8192.
> >
> > Well, I've just tested this with commit 783e9b4, which is before
> > netdev_add conversion to the qapi, and the command above just works.
> >
> > Didn't check if sndbuf is actually set to 8192, but this shows that
> > we've always accepted such a command.
> 
> Plausible.  Nevertheless, we really shouldn't.

Agreed. I would be willing to break compatibility to fix the suffix
part of the problem, but there's another issue: sndbuf shouldn't be
a string in QMP, and that's unfixable.

> >> To sum up, we go from QDict delivered by the JSON parser via QemuOpts to
> >> QAPI object, with a few cock-ups along the way.  Ugh.
> >> 
> >> By the way, the JSON schema reads
> >> 
> >>     { 'command': 'netdev_add',
> >>       'data': {'type': 'str', 'id': 'str', '*props': '**'},
> >>       'gen': 'no' }
> >> 
> >> I'll be hanged if I know what '**' means.
> >
> > For QMP on the wire it means that the command accepts a bunch of
> > arguments not specified in the schema.
> 
> Thanks!  Is the schema language documented anywhere?

Unfortunately not.

> > Yes, it's a dirty trick. Long story short: we decide to maintain
> > QemuOpts usage in both HMP and QMP to speed up netdev_add conversion.
> 
> I don't think '**' is as dirty as you make it sound.  It's simply a way
> to relax the rigidity of the schema.  I got no problem with that.

Problem is, I don't know how to make it better if we want to. I think
we could use it for the old commands that depend on QemuOpts so that
we don't make conversions too hard, but we shouldn't use it for new
commands.

> >> qdev_device_add() uses a few well-known options itself, and they're all
> >> strings.  The others it passes on to qdev_prop_parse().  This permits
> >> some weirdness like passing PCI device's addr property as number in QMP,
> >> even though it's nominally a string of the form SLOT[.FN].
> >> 
> >> No JSON schema, because device_add hasn't been qapified (Laszlo's
> >> working on it now).
> >> 
> >> > Now, I think I've found at least two bugs. The first one is the
> >> > netdev_add doc in the schema, which states that we do accept key=value
> >> > strings. The problem is here is that that's about the C API, on the
> >> > wire we act as before (ie. accepting each key as a separate argument).
> >> > The qapi-schame.json is more or less format-independent, so I'm not
> >> > exactly sure what's the best way to describe commands using QemuOpts
> >> > the way QMP uses it.
> >> 
> >> Netdev could be done without this key=value business in the schema.  We
> >> have just a few backends, and they're well-known, so we could just
> >> collect them all in a union, like Gerd did for chardev backends.
> >
> > I honestly don't know if this is a good idea, but should be possible
> > to change it if you're willing to.
> 
> chardev-add: the schema defines an object type for each backend
> (ChardevFile, ChardevSocket, ...), and collects them together in
> discriminated union ChardevBackend.  chardev_add takes that union.
> Thus, the schema accurately describes chardev-add's arguments, and the
> generated marshaller takes care of parsing chardev-add arguments into
> the appropriate objects.

Yes, it's a nice solution. I don't know why we didn't have that idea
when discussing netdev_add. Maybe we were biased by the old
implementation.

> netdev_add: the schema defines an object type for each backend
> (NetdevNoneOptions, NetdevUserOptions, ...).  netdev_add doesn't use
> them, but takes arbitrary parameters instead.  The connection is made in
> code, which stuffs the parameters in a QemuOpts (losing all JSON type
> information), then takes them out again to stuff them into the
> appropriate object type.  A job the marshaller should be able to do for
> us.
> 
> For me, the way chardev-add works makes a whole lot more sense, and I
> think we should clean up netdev_add to work the same way.
> Unfortunately, I can't commit to this cleanup job right now.

Agreed, and I think we won't break compatibility by doing this
improvement.

But you don't have to do it right now, having this for 1.5 would be nice.

> >> Devices are hairier.  For a union approach, we'd have to include a
> >> schema for every device model.  Dunno.
> >
> > IMHO, right now going fast is more important than doing things
> > with perfection (unless you can do perfection in no time, of course),
> > because the qapi conversions already took a lot more than expected
> > and it's delaying very good stuff (like the new qmp server, and dropping
> > the *.hx files and old QMP code).
> >
> > So, I wouldn't bother doing old crap commands perfect. Specially when
> > replacements are on the way.
> 
> I'm not asking for perfection.  You wondered whether we can hit certain
> errors with qemu_opts_from_qdict(), and I showed you how we can, and the
> unintended misfeatures that make it possible.
> 
> As far as I can tell, we can hit them only with netdev_add, not with
> device_add.  Happy to explain in more detail.

What would be nice is to have a list of bugs to fix if you're not
planning to fix them yourself. The problem I mention below is more
likely to be triggered with drive-mirror, I can fix that.

> >> > The second bug is that I entirely ignored how set_option_paramater()
> >> > handles errors when doing parse_option_size() conversion to Error **
> >> > and also when converting bdrv_img_create(). The end result is that
> >> > we can report an error twice, once with error_set() and later with
> >> > qerror_report() (or vice-versa). Shouldn't hurt on QMP as it knows
> >> > how to deal with this, on HMP and command-line we get complementary
> >> > error messages if we're lucky.
> >> 
> >> Example?  Fixable?
> >
> > Of course it's fixable, everything is fixable :)
> >
> > qmp_drive_mirror()
> >   bdrv_img()
> >     set_option_paramater()
> >     
> >> > I'm very surprised with my mistakes on the second bug (although some
> >> > of the mess with fprintf() was already there), but I honestly think we
> >> > should defer this to 1.5 (and I can do it myself next week).
> >> 
> >> Stick a fork in 1.4, it's done.
> >
> > No, I honestly think that at this point in time we should be fixing bugs
> > with proven end-user impact and serious regressions.
> 
> Note even that, it's *done*.  Finished.  Fertig.  Finito.  Game over;
> insert coin to play again ;-P
> 
> > I consider bikeshedding and fixing small glitches present in more than
> > one release to be an abuse for a hard-freeze.
> 
> Misunderstanding?

Regarding 1.4, yes. I thought you meant we should fix it for 1.4 and
be done with it. Sorry for that.
Laszlo Ersek - Feb. 15, 2013, 12:20 a.m.
Hi,

sorry for the late answer. I can only address the netdev_add /
opts-visitor stuff now.

On 02/14/13 17:36, Luiz Capitulino wrote:
> On Thu, 14 Feb 2013 14:31:50 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>>> On Thu, 14 Feb 2013 10:45:22 +0100
>>> Markus Armbruster <armbru@redhat.com> wrote:

>>>> netdev_add() uses QAPI wizardry to create the appropriate object from
>>>> the QemuOpts.  The parsing is done by visitors.  Things get foggy for me
>>>> from there on, but it looks like one of them, opts_type_size(), can
>>>> parse size suffixes, which is inappropriate for QMP.  A quick test
>>>> confirms that this is indeed possible:
>>>>
>>>>     {"execute": "netdev_add","arguments": {"type":"tap","id":"net.1",
>>>>     "sndbuf": "8k" }}
>>>>
>>>> Sets NetdevTapOptions member sndbuf to 8192.
>>>
>>> Well, I've just tested this with commit 783e9b4, which is before
>>> netdev_add conversion to the qapi, and the command above just works.
>>>
>>> Didn't check if sndbuf is actually set to 8192, but this shows that
>>> we've always accepted such a command.
>>
>> Plausible.  Nevertheless, we really shouldn't.
> 
> Agreed. I would be willing to break compatibility to fix the suffix
> part of the problem, but there's another issue: sndbuf shouldn't be
> a string in QMP, and that's unfixable.

The main purpose / requirement / guideline of the netdev_add &
opts-visitor conversion was that the exact same command lines had to
keep working. The primary source of input was the command line, ie.
QemuOpts. The qapi-schema was absolutely bastardized for the task, with
the single goal that the QemuOpts stuff *already gotten from the user*
would be auto-parsed into C structs -- structs that were generated from
the json. So, no QMP callers had been in anyone's mind as a priority;
the qapi/visitor etc. scaffolding was retrofitted to QemuOpts.

Please read the message on the main commit of the series:

  http://git.qemu.org/?p=qemu.git;a=commitdiff;h=eb7ee2cb

plus here's the blurb:

  http://lists.nongnu.org/archive/html/qemu-devel/2012-07/msg02102.html


>>>> Netdev could be done without this key=value business in the schema.  We
>>>> have just a few backends, and they're well-known, so we could just
>>>> collect them all in a union, like Gerd did for chardev backends.

The -netdev option centers on [type=]discriminator, and it had to work
as transparently as possible. I can't recall all the details any longer
(luckily!), but I remember sweating bullets wrapping the visitor API
around QemuOpts.

>>> I honestly don't know if this is a good idea, but should be possible
>>> to change it if you're willing to.
>>
>> chardev-add: the schema defines an object type for each backend
>> (ChardevFile, ChardevSocket, ...), and collects them together in
>> discriminated union ChardevBackend.  chardev_add takes that union.
>> Thus, the schema accurately describes chardev-add's arguments, and the
>> generated marshaller takes care of parsing chardev-add arguments into
>> the appropriate objects.
> 
> Yes, it's a nice solution. I don't know why we didn't have that idea
> when discussing netdev_add. Maybe we were biased by the old
> implementation.
> 
>> netdev_add: the schema defines an object type for each backend
>> (NetdevNoneOptions, NetdevUserOptions, ...).  netdev_add doesn't use
>> them, but takes arbitrary parameters instead.  The connection is made in
>> code, which stuffs the parameters in a QemuOpts (losing all JSON type
>> information), then takes them out again to stuff them into the
>> appropriate object type.  A job the marshaller should be able to do for
>> us.
>>
>> For me, the way chardev-add works makes a whole lot more sense, and I
>> think we should clean up netdev_add to work the same way.
>> Unfortunately, I can't commit to this cleanup job right now.
> 
> Agreed, and I think we won't break compatibility by doing this
> improvement.

The most important things to compare are qemu_chr_new_from_opts() and
net_client_init(), if my reading is correct. Each is the corresponding
iteration callback for the list of chardev/netdev list of QemuOpts.

(a) qemu_chr_new_from_opts() dives in, fishes out the discriminator
manually -- "backend" is encoded as a C string literal, and there's a
similar access to "mux" --, and looks up the appropriate open function
based on backend (with linear search & strcmp()).

No matter which open function we choose, each one is chock-full of
qemu_opt_get_XXXX() calls with property names hard-coded as C string
literals. *Killing this* was the exact purpose of opts-visitor. Cf.:

(b) net_client_init() parses the QemuOpts object into a C struct, based
on the schema, validating basic syntax simultaneously. Then
net_client_init1(), rather than a linear, string-based search, uses the
enum value ("kind") as the controlling expression of a switch statement,
and as immediate offset into the array of function pointers.

None of those init functions makes one qemu_opt_get_XXXX() call with a
hard-coded property name; they all use *field names* and access the
pre-parsed structs.


Honestly I don't know wheter opts-visitor was a good idea to begin with.
I was asked to do it, so I tried my best.

What is clear however is that opts-visitor is an utter failure -- people
are not using it; probably because it's awkward or not flexible enough.
If it had lived up to its promise, then we'd be discussing a rebase of
chardev (and maybe even the recent multiqueue patches) *onto* opts-visitor.

(I'm mentioning multiqueue specifically because of the get_fds()
function introduced in commit 264986e2. That commit extends the schema
with a field called "fds" and introduces manually parses it into a list
with get_fds().

Let it suffice to mention that I was working very hard to implement
repeating options in opts-visitor. The parsed output is a standard qapi
list, ie. on the schema level. See again commit eb7ee2cb. In this case
you'd just say "fd=X,fd=Y,fd=Z" rather than the new, custom-format
QemuOpt "fds=X:Y:Z".

I think this nicely underlines the failure of opts-visitor:
- if get_fds() could have been equivalently implemented with a repeating
option in the schema, then opts-visitor failed to get recognition (=
useless work),
- if opts-visitor had turned out inflexible to accomodate this use case,
then it would have failed functionally (= useless work).)

Failure is *not* sweet.

Laszlo
Laszlo Ersek - Feb. 15, 2013, 1:12 a.m.
On 02/15/13 01:20, Laszlo Ersek wrote:
> On 02/14/13 17:36, Luiz Capitulino wrote:
>> On Thu, 14 Feb 2013 14:31:50 +0100
>> Markus Armbruster <armbru@redhat.com> wrote:

>>> chardev-add: the schema defines an object type for each backend
>>> (ChardevFile, ChardevSocket, ...), and collects them together in
>>> discriminated union ChardevBackend.  chardev_add takes that union.
>>> Thus, the schema accurately describes chardev-add's arguments, and the
>>> generated marshaller takes care of parsing chardev-add arguments into
>>> the appropriate objects.
>>
>> Yes, it's a nice solution. I don't know why we didn't have that idea
>> when discussing netdev_add. Maybe we were biased by the old
>> implementation.
>>
>>> netdev_add: the schema defines an object type for each backend
>>> (NetdevNoneOptions, NetdevUserOptions, ...).  netdev_add doesn't use
>>> them, but takes arbitrary parameters instead.  The connection is made in
>>> code, which stuffs the parameters in a QemuOpts (losing all JSON type
>>> information), then takes them out again to stuff them into the
>>> appropriate object type.  A job the marshaller should be able to do for
>>> us.
>>>
>>> For me, the way chardev-add works makes a whole lot more sense, and I
>>> think we should clean up netdev_add to work the same way.

So, regarding netdev_add again,

--[schema dict]--> qmp_netdev_add()                  ---\
--[QemuOpts]-->    net_client_init() == opts-visitor ---\
--[C struct]-->    specific logic

(a) I agree that the intermediary QemuOpts representation is
superfluous, and that we could directly expose the schema over QMP, ie.
go from schema dict right to C struct. However,

(b) opts-visitor's primary responsibility remains mangling one QemuOpts
instance into a C struct. This effectively hamstrings any affected
schema. You *can* choose to expose/reuse that schema over the wire, but
you won't have any freedom massaging the schema later on just for the
QMP caller's sake.

--[schema dict]--> qmp_netdev_add() --[C struct]--> specific logic
--[QemuOpts]-->    opts-visitor     --[C struct]--> specific logic

Obviously, you want to share the [C struct] and the "specific logic"
between the two cases. However the C struct (the schema) is hamstrung by
QemuOpts, thus ultimately the QMP wire format is dictated by the
QemuOpts format that you accept on the command line.

At first sight this might come through as a "semantic match" (same stuff
available on the command line as over QMP), but:
- you can't compose the underlying schema just any way you like,
- what you can't express as a single QemuOpts object (think dictionaries
in dictionaries), you can't allow over QMP.

With chardev_add, IIUC, first you create a logical schema, and expose it
over QMP (all dandy), then hand-craft qemu_opt_get_XXXX() code, with
properties encoded as C string literals, in order to shoehorn the
logical schema into the command line (QemuOpts). I couldn't call this
approach "bad" with a straight face (it clearly works in practice!), but
as I perceived it, opts-visitor had been invented precisely to eliminate
this.

Sorry for ranting...

Laszlo
Markus Armbruster - Feb. 19, 2013, 9:29 a.m.
Laszlo Ersek <lersek@redhat.com> writes:

> On 02/15/13 01:20, Laszlo Ersek wrote:
>> On 02/14/13 17:36, Luiz Capitulino wrote:
>>> On Thu, 14 Feb 2013 14:31:50 +0100
>>> Markus Armbruster <armbru@redhat.com> wrote:
>
>>>> chardev-add: the schema defines an object type for each backend
>>>> (ChardevFile, ChardevSocket, ...), and collects them together in
>>>> discriminated union ChardevBackend.  chardev_add takes that union.
>>>> Thus, the schema accurately describes chardev-add's arguments, and the
>>>> generated marshaller takes care of parsing chardev-add arguments into
>>>> the appropriate objects.
>>>
>>> Yes, it's a nice solution. I don't know why we didn't have that idea
>>> when discussing netdev_add. Maybe we were biased by the old
>>> implementation.
>>>
>>>> netdev_add: the schema defines an object type for each backend
>>>> (NetdevNoneOptions, NetdevUserOptions, ...).  netdev_add doesn't use
>>>> them, but takes arbitrary parameters instead.  The connection is made in
>>>> code, which stuffs the parameters in a QemuOpts (losing all JSON type
>>>> information), then takes them out again to stuff them into the
>>>> appropriate object type.  A job the marshaller should be able to do for
>>>> us.
>>>>
>>>> For me, the way chardev-add works makes a whole lot more sense, and I
>>>> think we should clean up netdev_add to work the same way.
>
> So, regarding netdev_add again,
>
> --[schema dict]--> qmp_netdev_add()                  ---\
> --[QemuOpts]-->    net_client_init() == opts-visitor ---\
> --[C struct]-->    specific logic
>
> (a) I agree that the intermediary QemuOpts representation is
> superfluous, and that we could directly expose the schema over QMP, ie.
> go from schema dict right to C struct. However,
>
> (b) opts-visitor's primary responsibility remains mangling one QemuOpts
> instance into a C struct. This effectively hamstrings any affected
> schema. You *can* choose to expose/reuse that schema over the wire, but
> you won't have any freedom massaging the schema later on just for the
> QMP caller's sake.
>
> --[schema dict]--> qmp_netdev_add() --[C struct]--> specific logic
> --[QemuOpts]-->    opts-visitor     --[C struct]--> specific logic
>
> Obviously, you want to share the [C struct] and the "specific logic"
> between the two cases. However the C struct (the schema) is hamstrung by
> QemuOpts, thus ultimately the QMP wire format is dictated by the
> QemuOpts format that you accept on the command line.
>
> At first sight this might come through as a "semantic match" (same stuff
> available on the command line as over QMP), but:
> - you can't compose the underlying schema just any way you like,
> - what you can't express as a single QemuOpts object (think dictionaries
> in dictionaries), you can't allow over QMP.

Correct in general, but need not be an issue in a specific case.

When it is, I'd suggest to try something like:

* Create a schema appropriate for QMP.  This results in a C data
  structure (generated) and code accepting it.  Let's call the latter
  "the interface".

* Create a schema for an opts-visitor, use it to take apart the option
  string.  This results in another C data structure.

* Write glue code to convert the opts-visitor result into the data
  structure accepted by the interface.

Alternatively, if QemuOpts are sufficiently simple, take them apart the
old-fashioned way, without visitors, then call a suitable interface
shared with the QMP case.  Calling the QMP handler requires building a
QDict, so you might be better off aiming lower.  The obvious option is
filling in the QMP schema's C data structure so you can call "the
interface".

> With chardev_add, IIUC, first you create a logical schema, and expose it
> over QMP (all dandy), then hand-craft qemu_opt_get_XXXX() code, with
> properties encoded as C string literals, in order to shoehorn the
> logical schema into the command line (QemuOpts). I couldn't call this
> approach "bad" with a straight face (it clearly works in practice!), but
> as I perceived it, opts-visitor had been invented precisely to eliminate
> this.

If I understand you correctly, this is exactly what I just described
under "alternatively, if QemuOpts are sufficiently simple".  Except the
options are not really simple.  So maybe we'd be better off doing it the
other way.

> Sorry for ranting...

You call this a rant?  ;)
Markus Armbruster - Feb. 19, 2013, 9:50 a.m.
Laszlo Ersek <lersek@redhat.com> writes:

> Hi,
>
> sorry for the late answer. I can only address the netdev_add /
> opts-visitor stuff now.
>
> On 02/14/13 17:36, Luiz Capitulino wrote:
>> On Thu, 14 Feb 2013 14:31:50 +0100
>> Markus Armbruster <armbru@redhat.com> wrote:
>>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>>>> On Thu, 14 Feb 2013 10:45:22 +0100
>>>> Markus Armbruster <armbru@redhat.com> wrote:
>
>>>>> netdev_add() uses QAPI wizardry to create the appropriate object from
>>>>> the QemuOpts.  The parsing is done by visitors.  Things get foggy for me
>>>>> from there on, but it looks like one of them, opts_type_size(), can
>>>>> parse size suffixes, which is inappropriate for QMP.  A quick test
>>>>> confirms that this is indeed possible:
>>>>>
>>>>>     {"execute": "netdev_add","arguments": {"type":"tap","id":"net.1",
>>>>>     "sndbuf": "8k" }}
>>>>>
>>>>> Sets NetdevTapOptions member sndbuf to 8192.
>>>>
>>>> Well, I've just tested this with commit 783e9b4, which is before
>>>> netdev_add conversion to the qapi, and the command above just works.
>>>>
>>>> Didn't check if sndbuf is actually set to 8192, but this shows that
>>>> we've always accepted such a command.
>>>
>>> Plausible.  Nevertheless, we really shouldn't.
>> 
>> Agreed. I would be willing to break compatibility to fix the suffix
>> part of the problem, but there's another issue: sndbuf shouldn't be
>> a string in QMP, and that's unfixable.
>
> The main purpose / requirement / guideline of the netdev_add &
> opts-visitor conversion was that the exact same command lines had to
> keep working. The primary source of input was the command line, ie.
> QemuOpts. The qapi-schema was absolutely bastardized for the task, with
> the single goal that the QemuOpts stuff *already gotten from the user*
> would be auto-parsed into C structs -- structs that were generated from
> the json. So, no QMP callers had been in anyone's mind as a priority;
> the qapi/visitor etc. scaffolding was retrofitted to QemuOpts.
>
> Please read the message on the main commit of the series:
>
>   http://git.qemu.org/?p=qemu.git;a=commitdiff;h=eb7ee2cb
>
> plus here's the blurb:
>
>   http://lists.nongnu.org/archive/html/qemu-devel/2012-07/msg02102.html

I understand, and it makes sense.  The trouble is it has since bled into
QMP.  I'd like us to clean that up.

>>>>> Netdev could be done without this key=value business in the schema.  We
>>>>> have just a few backends, and they're well-known, so we could just
>>>>> collect them all in a union, like Gerd did for chardev backends.
>
> The -netdev option centers on [type=]discriminator, and it had to work
> as transparently as possible. I can't recall all the details any longer
> (luckily!), but I remember sweating bullets wrapping the visitor API
> around QemuOpts.
>
>>>> I honestly don't know if this is a good idea, but should be possible
>>>> to change it if you're willing to.
>>>
>>> chardev-add: the schema defines an object type for each backend
>>> (ChardevFile, ChardevSocket, ...), and collects them together in
>>> discriminated union ChardevBackend.  chardev_add takes that union.
>>> Thus, the schema accurately describes chardev-add's arguments, and the
>>> generated marshaller takes care of parsing chardev-add arguments into
>>> the appropriate objects.
>> 
>> Yes, it's a nice solution. I don't know why we didn't have that idea
>> when discussing netdev_add. Maybe we were biased by the old
>> implementation.
>> 
>>> netdev_add: the schema defines an object type for each backend
>>> (NetdevNoneOptions, NetdevUserOptions, ...).  netdev_add doesn't use
>>> them, but takes arbitrary parameters instead.  The connection is made in
>>> code, which stuffs the parameters in a QemuOpts (losing all JSON type
>>> information), then takes them out again to stuff them into the
>>> appropriate object type.  A job the marshaller should be able to do for
>>> us.
>>>
>>> For me, the way chardev-add works makes a whole lot more sense, and I
>>> think we should clean up netdev_add to work the same way.
>>> Unfortunately, I can't commit to this cleanup job right now.
>> 
>> Agreed, and I think we won't break compatibility by doing this
>> improvement.
>
> The most important things to compare are qemu_chr_new_from_opts() and
> net_client_init(), if my reading is correct. Each is the corresponding
> iteration callback for the list of chardev/netdev list of QemuOpts.
>
> (a) qemu_chr_new_from_opts() dives in, fishes out the discriminator
> manually -- "backend" is encoded as a C string literal, and there's a
> similar access to "mux" --, and looks up the appropriate open function
> based on backend (with linear search & strcmp()).
>
> No matter which open function we choose, each one is chock-full of
> qemu_opt_get_XXXX() calls with property names hard-coded as C string
> literals. *Killing this* was the exact purpose of opts-visitor. Cf.:
>
> (b) net_client_init() parses the QemuOpts object into a C struct, based
> on the schema, validating basic syntax simultaneously. Then
> net_client_init1(), rather than a linear, string-based search, uses the
> enum value ("kind") as the controlling expression of a switch statement,
> and as immediate offset into the array of function pointers.
>
> None of those init functions makes one qemu_opt_get_XXXX() call with a
> hard-coded property name; they all use *field names* and access the
> pre-parsed structs.

More readable, and the compiler can help more with typos.

> Honestly I don't know wheter opts-visitor was a good idea to begin with.
> I was asked to do it, so I tried my best.
>
> What is clear however is that opts-visitor is an utter failure -- people
> are not using it; probably because it's awkward or not flexible enough.
> If it had lived up to its promise, then we'd be discussing a rebase of
> chardev (and maybe even the recent multiqueue patches) *onto* opts-visitor.

I suspect it's not used more widely because:

* There's just one example (I think) for opts-visitors, and
  understanding how it works is anything but trivial.  Examples for
  doing it the old-fashioned way are all over the place, impossible to
  miss, and easy to copy.

* opts-visitors is overkill for simple cases.  Most cases are simple, or
  at least start simple.

> (I'm mentioning multiqueue specifically because of the get_fds()
> function introduced in commit 264986e2. That commit extends the schema
> with a field called "fds" and introduces manually parses it into a list
> with get_fds().

Clearly inappropriate for QMP.  Should have been caught in review.
Needs to be deprecated and replaced by an appropriate interface.

We suck.

> Let it suffice to mention that I was working very hard to implement
> repeating options in opts-visitor. The parsed output is a standard qapi
> list, ie. on the schema level. See again commit eb7ee2cb. In this case
> you'd just say "fd=X,fd=Y,fd=Z" rather than the new, custom-format
> QemuOpt "fds=X:Y:Z".
>
> I think this nicely underlines the failure of opts-visitor:
> - if get_fds() could have been equivalently implemented with a repeating
> option in the schema, then opts-visitor failed to get recognition (=
> useless work),
> - if opts-visitor had turned out inflexible to accomodate this use case,
> then it would have failed functionally (= useless work).)
>
> Failure is *not* sweet.

I suspect the real failure is in patch review.

We can't expect everyone to know every feature, such as repeating
options.  But we need to catch wheel reinventions in review.
Laszlo Ersek - Feb. 19, 2013, 3:54 p.m.
On 02/19/13 10:50, Markus Armbruster wrote:

> I suspect the real failure is in patch review.
> 
> We can't expect everyone to know every feature, such as repeating
> options.  But we need to catch wheel reinventions in review.

I'd like to agree, but I'm simply unable to review more. On a good day,
if I spend it entirely on review, I can do ~10 patches tops. qemu-devel
gets approx. 200 messages per day (that's my impression anyway).

Laszlo
Laszlo Ersek - Feb. 19, 2013, 3:55 p.m.
On 02/19/13 10:29, Markus Armbruster wrote:

> When it is, I'd suggest to try something like:
> 
> * Create a schema appropriate for QMP.  This results in a C data
>   structure (generated) and code accepting it.  Let's call the latter
>   "the interface".
> 
> * Create a schema for an opts-visitor, use it to take apart the option
>   string.  This results in another C data structure.
> 
> * Write glue code to convert the opts-visitor result into the data
>   structure accepted by the interface.

This is good, I like it.

Thanks,
Laszlo
Markus Armbruster - Feb. 19, 2013, 5:40 p.m.
Laszlo Ersek <lersek@redhat.com> writes:

> On 02/19/13 10:50, Markus Armbruster wrote:
>
>> I suspect the real failure is in patch review.
>> 
>> We can't expect everyone to know every feature, such as repeating
>> options.  But we need to catch wheel reinventions in review.
>
> I'd like to agree, but I'm simply unable to review more. On a good day,
> if I spend it entirely on review, I can do ~10 patches tops. qemu-devel
> gets approx. 200 messages per day (that's my impression anyway).

Note "we need to catch", not "you should've caught".

If everybody took his review duty as seriously as you do, we'd be in
better shape.

Patch

diff --git a/util/qemu-option.c b/util/qemu-option.c
index c12e724..5a1d03c 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -231,8 +231,10 @@  static void parse_option_size(const char *name, const char *value,
             break;
         default:
             error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, "a size");
+#if 0 /* conversion from qerror_report() to error_set() broke this: */
             error_printf_unless_qmp("You may use k, M, G or T suffixes for "
                     "kilobytes, megabytes, gigabytes and terabytes.\n");
+#endif
             return;
         }
     } else {
@@ -771,7 +773,9 @@  QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
     if (id) {
         if (!id_wellformed(id)) {
             error_set(errp,QERR_INVALID_PARAMETER_VALUE, "id", "an identifier");
+#if 0 /* conversion from qerror_report() to error_set() broke this: */
             error_printf_unless_qmp("Identifiers consist of letters, digits, '-', '.', '_', starting with a letter.\n");
+#endif
             return NULL;
         }
         opts = qemu_opts_find(list, id);