Re: [Qemu-devel] [PATCH RFC v3 30/32] qapi: New QMP command query-schema for QMP schema introspection

[Markus Armbruster]

Michael Roth <address@hidden> writes:

> Quoting Eric Blake (2015-09-01 14:12:24)
>> On 09/01/2015 12:40 PM, Michael Roth wrote:
>> > Quoting Markus Armbruster (2015-08-04 10:58:14)
>> >> Caution, rough edges.
>> >>
>> >> qapi/introspect.json defines the introspection schema.  It should do
>> >> for uses other than QMP.
>> >> FIXME it's almost entirely devoid of comments.
>> >>
>> >> The introspection schema does not reflect all the rules and
>> >> restrictions that apply to QAPI schemata.  A valid QAPI schema has an
>> >> introspection value conforming to the introspection schema, but the
>> >> converse is not true.
>> >>
>> >> Introspection lowers away a number of schema details:
>> >>
>> >> * The built-in types are declared with their JSON type.
>> >>
>> >>   TODO Should we map all the integer types to just int?
>> > 
>> > I don't think we should. If management chooses to handle them in this
>> > generic fashion that's their perogative/problem, but treating all ints
>> > as a single generic type can lead to unexpected results when those values
>> > get passed on to functions expecting, for instance, uint8_t. So QEMU
>> > should do its part to convey this information somehow.
>> 
>> The argument here is that it is always more conservative to start with
>> less information, where we can later add more information (whether in
>> the form of type constraints such as uint8_t, or in a different form
>> such as min/max values) if they prove useful.  And QMP already
>> guarantees that it gives sane error messages for parameters that are out
>> of range, so the worst behavior a client might see is that a parameter
>> claiming to be 'int' in the introspection gracefully rejects an attempt
>> to use '256' as a value because the underlying type was uint8_t.
>
> That's true for many cases, but a 255 uint8_t value being passed in as
> a int8_t could still cause unexpected results, for instance.

QMP introspection is about the QMP wire ABI.

QMP core translates from the wire to internal C interfaces.  It must
error out unless the C interface can take the value read from the wire.
If the C interface takes int8_t, QMP core must reject 255.  No different
from 3.14, "hello", or { "answer": 42 }.

>> If we advertise full types now, then the choice of integer type becomes
>> ABI (switching from 'int8_t' to 'uint8_t' has observable impact in the
>> introspection) that we are stuck exposing in introspection forever.  And
>> in the past, we have successfully retyped a command with no change to
>> the wire API (see commit 5fba6c0).
>
> Some changes can be deemed 'compatible' on a case-by-case, such as with
> the above, but those changes are were still documented/published through
> the schema.
>
> Why should an introspection interface not convey the same specificity of
> information as our schema? It seems inconsistent.

The QAPI schema defines both the QMP wire ABI and internal C interfaces.
Introspection should expose only the former, not the latter.  Thus,
introspection conveying less information than the full QAPI schema is by
design.

However, there's still a perfectly legitimate question, namely whether
the various integer types should be considered wire ABI or not.

The commit message above asks the question, and the next patch proposes
an answer, namely that they aren't ABI.

> If we do things this way it almost seems like we'd need a disclaimer of
> the form "please reference QAPI schema for details on acceptable
> integer ranges" in the introspection interface documentation. Which
> honestly doesn't seem like that bad an approach if we're up front
> about it and unsure how

We need to point to QAPI schema comments for information on domains
anyway, because the integer types are insufficient for specifying
domains that occur in practice.

Consider a command to create an image, similar to "qemu-img create"
(such a command doesn't currently exist, but it has been discussed).  If
format is qcow2, the cluster size must be a power of two between 2^9 and
2^21, and size must be a multiple of the cluster size.

In QAPI, the size would be in bytes, and the type would be plain 'int'.
The cluster size could either be given in bytes (plain 'int' again), or
in width bits (any integer type would do, value must be between 9 and 21
inclusive).  In both cases, QAPI's types are useless for specifying the
actual domain.

Other schema languages offer richer means for specifying domains.  For
instance, JSON Schema has multipleOf, maximum and exclusiveMaximum,
minimum and exclusiveMinimum[*].  Good enough to specify a cluster size
given in bits (minimum 9, maximum 21).  Still incapable of specifying
the rest.

> Though, if it happens too often, I could see that being a pain for
> management, but the changes are there and in effect as far as QEMU is
> concerned, regardless of whether or not the wire protocol is more
> generic with how it represents types. We should document QAPI: JSON
> RPC is just a transport that could theoretically get swapped
> out for different wire protocol if some need arose in the future.

Yes, QMP is just one possible RPC transport for interfaces defined in
QAPI.

However, QMP introspection isn't about QAPI, it's about the QMP wire
ABI.

Say we add another transport, and that transport is binary instead of
text.  In that transport, the integer encoding (width and signedness) is
part of the wire ABI.  We could then chose a tight coupling to the
internal C interface and pick the same encodings.  Or we could chose a
loser coupling and go with int64_t throughout, just like QMP.  We could
even pick case by case.

My point is: I strongly recommend to distinguish between *encoding* and
*domain*.  Encoding is an artifact of the transport, with the boundary
condition that it needs to be able to hold the complete domain.

In the QMP transport, all integers are encoded the same: decimal ASCII.

>> At any rate, patch 31/32 in this series gives stronger arguments for
>> merging the types for at least the initial implementation; we can always
>> change our minds later and undo the merging, even if it is after 2.5
>> when we change our minds.
>
> 31/32 makes a good point about fixed-width types not being all that
> appropriate for introspection. I suppose you could consider them a QAPI
> shorthand for max/min ranges, but I still think that information should
> be conveyed at least in that basic form.
>
> If we have some concern that this would end up being a mistake in the
> future, we could possibly introduce it as an optional hint for integer
> types. Unfortunately, either way that would complicate the format
> somewhat...

I readily concede that range information could be useful.  Two remarks:

1. Let's start introspection simple, and add power as needed.

2. If a need for range information emerges, I'd rather follow JSON
   Schema's lead and provide for arbitrary ranges, because that way we
   can make them tight: we can specify "9 <= cluster-bits <= 21" instead
   of "cluster-bits is uint8_t, and therefore 0 <= cluster-bits <= 255".

>> But the mere fact that you are questioning the idea means that patch 30
>> and 31 should not be merged (previously, I had argued that separating
>> the patches didn't make sense; I don't know if Markus was planning to
>> merge the two based on my recommendation), if only so that reverting the
>> type merging becomes an easier task if we decide down the road that we
>> don't need the merged types.
>
> I guess another alternative would be to proceed as things are and just
> be clear about the lack of some bits of detail like valid integer
> ranges. Would still make introspection useful for discovering the
> presence and general types of fields, with the schema serving as
> the primary reference for how clients should be implemented.

Sounds good to me.  

Suggest you review the documentation in PATCH v4 once posted to make
sure it's to your taste there.

> Not sure if that sort of separation of scope was the initial plan or
> not.

I separated RFC PATCH 31+32 from RFC PATCH 30 to facilitate discussion.
I envisaged three possible outcomes for each of the two:

1. We're quite sure that's what we want.  Squash the patch into 30.

2. We like it well enough, but we're not quite sure about it.  Keep the
   patch separate for easy revert.

3. Dumb idea.  Drop the patch.

My current opinion is to squash PATCH 31 "qapi-introspect: Map all
integer types to 'int'", but keep PATCH 32 "qapi-introspect: Hide type
names" separate.  I hasten to add that both are still very much open to
debate.


[*] http://json-schema.org/latest/json-schema-validation.html#anchor13