qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH 7/9] qmp: expose a command to query capabilities of config parser
Date: Mon, 19 Mar 2012 15:41:31 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.2) Gecko/20120216 Thunderbird/10.0.2

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]