On Thu, 14 Feb 2013 14:31:50 +0100 Markus Armbruster <arm...@redhat.com> wrote:
> [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. 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.