Laszlo Ersek <ler...@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 <arm...@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? ;)