[Some quoted material restored] Luiz Capitulino <lcapitul...@redhat.com> writes:
> On Thu, 14 Feb 2013 10:45:22 +0100 > Markus Armbruster <arm...@redhat.com> wrote: > >> [Note cc: +Laszlo, +Anthony, -qemu-trivial] >> >> Luiz Capitulino <lcapitul...@redhat.com> writes: >> >> > On Fri, 08 Feb 2013 20:34:20 +0100 >> > Markus Armbruster <arm...@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?