| Message ID | 1360340232-4670-5-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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);
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).
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.
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.
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).
[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.
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.
[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?
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.
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
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
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? ;)
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.
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
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
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.
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);
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(+)