Re: [Qemu-devel] [PATCH for-1.4 v2 4/6] qemu-option: Disable two helpful messages that got broken recently
Laszlo Ersek 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.
Re: [Qemu-devel] [PATCH for-1.4 v2 4/6] qemu-option: Disable two helpful messages that got broken recently
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
Re: [Qemu-devel] [PATCH for-1.4 v2 4/6] qemu-option: Disable two helpful messages that got broken recently
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
Re: [Qemu-devel] [PATCH for-1.4 v2 4/6] qemu-option: Disable two helpful messages that got broken recently
Laszlo Ersek 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 wrote:
>>> Luiz Capitulino writes:
On Thu, 14 Feb 2013 10:45:22 +0100
Markus Armbruster 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_() 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 tha
Re: [Qemu-devel] [PATCH for-1.4 v2 4/6] qemu-option: Disable two helpful messages that got broken recently
Laszlo Ersek 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 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_() 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? ;)
Re: [Qemu-devel] [PATCH for-1.4 v2 4/6] qemu-option: Disable two helpful messages that got broken recently
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 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_() 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
Re: [Qemu-devel] [PATCH for-1.4 v2 4/6] qemu-option: Disable two helpful messages that got broken recently
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 wrote:
>> Luiz Capitulino writes:
>>> On Thu, 14 Feb 2013 10:45:22 +0100
>>> Markus Armbruster 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_() 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_() call with a
hard-coded property na
Re: [Qemu-devel] [PATCH for-1.4 v2 4/6] qemu-option: Disable two helpful messages that got broken recently
On Thu, 14 Feb 2013 14:31:50 +0100
Markus Armbruster wrote:
> [Some quoted material restored]
>
> Luiz Capitulino writes:
>
> > On Thu, 14 Feb 2013 10:45:22 +0100
> > Markus Armbruster wrote:
> >
> >> [Note cc: +Laszlo, +Anthony, -qemu-trivial]
> >>
> >> Luiz Capitulino writes:
> >>
> >> > On Fri, 08 Feb 2013 20:34:20 +0100
> >> > Markus Armbruster 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 ol
Re: [Qemu-devel] [PATCH for-1.4 v2 4/6] qemu-option: Disable two helpful messages that got broken recently
[Some quoted material restored]
Luiz Capitulino writes:
> On Thu, 14 Feb 2013 10:45:22 +0100
> Markus Armbruster wrote:
>
>> [Note cc: +Laszlo, +Anthony, -qemu-trivial]
>>
>> Luiz Capitulino writes:
>>
>> > On Fri, 08 Feb 2013 20:34:20 +0100
>> > Markus Armbruster 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 A
Re: [Qemu-devel] [PATCH for-1.4 v2 4/6] qemu-option: Disable two helpful messages that got broken recently
On Thu, 14 Feb 2013 10:45:22 +0100
Markus Armbruster wrote:
> [Note cc: +Laszlo, +Anthony, -qemu-trivial]
>
> Luiz Capitulino writes:
>
> > On Fri, 08 Feb 2013 20:34:20 +0100
> > Markus Armbruster 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
Re: [Qemu-devel] [PATCH for-1.4 v2 4/6] qemu-option: Disable two helpful messages that got broken recently
[Note cc: +Laszlo, +Anthony, -qemu-trivial]
Luiz Capitulino writes:
> On Fri, 08 Feb 2013 20:34:20 +0100
> Markus Armbruster 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 c
Re: [Qemu-devel] [PATCH for-1.4 v2 4/6] qemu-option: Disable two helpful messages that got broken recently
On Fri, 08 Feb 2013 20:34:20 +0100 Markus Armbruster 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).
Re: [Qemu-devel] [PATCH for-1.4 v2 4/6] qemu-option: Disable two helpful messages that got broken recently
Luiz Capitulino writes: > On Fri, 08 Feb 2013 19:58:42 +0100 > Markus Armbruster wrote: > >> Luiz Capitulino writes: >> >> > On Fri, 8 Feb 2013 17:17:10 +0100 >> > Markus Armbruster 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 >> > >> > 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.
Re: [Qemu-devel] [PATCH for-1.4 v2 4/6] qemu-option: Disable two helpful messages that got broken recently
Luiz Capitulino writes: > On Fri, 8 Feb 2013 17:17:10 +0100 > Markus Armbruster 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 > > 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).
Re: [Qemu-devel] [PATCH for-1.4 v2 4/6] qemu-option: Disable two helpful messages that got broken recently
On Fri, 08 Feb 2013 19:58:42 +0100 Markus Armbruster wrote: > Luiz Capitulino writes: > > > On Fri, 8 Feb 2013 17:17:10 +0100 > > Markus Armbruster 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 > > > > 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.
Re: [Qemu-devel] [PATCH for-1.4 v2 4/6] qemu-option: Disable two helpful messages that got broken recently
On Fri, 8 Feb 2013 17:17:10 +0100
Markus Armbruster 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
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
> ---
> 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);
