Laszlo Ersek <ler...@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 <arm...@redhat.com> wrote: >>> Luiz Capitulino <lcapitul...@redhat.com> writes: >>>> On Thu, 14 Feb 2013 10:45:22 +0100 >>>> Markus Armbruster <arm...@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.