Re: [Qemu-devel] [PATCH 7/9] qmp: expose a command to query capabilities of config parser

[Anthony Liguori <anthony@codemonkey.ws>]

On 03/19/2012 03:31 PM, Eric Blake wrote:
> On 03/19/2012 02:19 PM, Anthony Liguori wrote:
>>>> +# @size: an integer followed by either 'K', 'M', 'G', or 'T'.
>>>
>>> Not quite - I found three different parsers, but none of them match this
>>> description.
>>
>> Heh, I was going by the documentation in the code:
>>
>>      QEMU_OPT_SIZE,        /* size, accepts (K)ilo, (M)ega, (G)iga,
>> (T)era postfix */
>>
>>
>>> That is, qemu-option.c:parse_option_size() supports 'k'
>>> and 'b', as well as omitting a suffix, but does not support 'm'.
>>
>> TBH, I'd prefer to not document 'k' and 'b' and explicitly document that
>> the suffix is optional and bytes is implied (which I thought was implied
>> in my documentation, but I can see that it may not be).
>>
>> I'd rather we support officially support one way of doing things and if
>> we support more, do it under the mantra of being liberal in what we accept.
>
> Okay, I can live with documenting less than we accept, but we definitely
> need to mention that the suffix is optional, and that when omitted, the
> unit is bytes (as originally written, I read it that the suffix was
> mandatory, and that there was no way to request bytes).  I'm not sure
> whether documenting an explicit 'b' for bytes helps or hurts, and I'm
> not sure whether adding support for 'p' and 'e' as documented suffixes
> makes sense.

I'm not really concerned about 'p' and 'e' for now.  'T' still seems novel 
enough to me :-)

>>> Then
>>> there's cmd.c:cvtnum(), which is case-insensitive, and adds 'p' and 'e',
>>> but omits 'b'.  Then there's the 'o' type in monitor.c that defers to
>>> cutils.c:strtosz(), which defaults to bytes but is case-insensitive and
>>> supports 'b' but not 'p'.  Why are we using three different parsers,
>>> anyway?
>>
>> Yes, I am aware of that, and yes, I'm going to fix it.  One bit at a
>> time though.
>
> Fair enough :)  It's just that I recently fixed a bug in libvirt where
> it was using a human interface with no suffix and getting 'M' behavior,
> where an explicit 'B' behavior fixed things to use bytes, and I was
> quite put off by the inconsistencies and lack of documentation when I
> finally discovered that a 'B' suffix did what I wanted.

Yup, and hopefully qapi-schema.json is fixing the documentation problem here. 
For what it's worth, my aim is to refactor the QemuOpts definitions to be part 
of qapi-schema.json too such that they carry the same level of documentation 
(and self consistency).

This is the advantage of having a single centralized schema.  I think it has 
really helped make sure everything is consistently documented and specified. 
Having disjoint definitions and descriptions has led to a lot of inconsistency 
over time.

> I'm glad that
> QMP defaults to 'B', not 'M'.  (Libvirt had its own fair share of
> inconsistent scaling routines, where some interfaces default to 'k'
> instead of bytes, and I recently consolidated libvirt to use a single
> scaling routing for much the same reasons.)

You can thank Xen for the kb default :-)  That goes back to the libvir days.

Regards,

Anthony Liguori