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

[Michael Roth]

Quoting Markus Armbruster (2015-09-02 03:56:41)
> 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.

That's a good point, it's not feasible to convey all the
constraints through this sort of interface, so I suppose we
have to draw the line somewhere. Generic integer types is a
fairly logical/convenient stopping point.

> 
> > 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.

So I may have missed the point somewhat then: it's not a single
introspection structure for all conceivable wire protocols,
but instead each exposed wire protocol would have it's own
schema visitor implementation to convey how fields are encoded.

So in this case we're essentially documenting how
QMP{Input,Output}Visitor expect/encode the individual parameters,
and anything beyond that happens at the QAPI/C level. QMP*Visitor
does in fact map fixed-width integers to more generic int64_t/QInt,
then on to JSON primitives, so it makes sense to describe them as
such.

I think we need to be careful that these descriptions are not
interpreted by clients as an alternative to the more-specific
constraints in the QAPI schema though. 'query-schema' seems
a bit misleading in that regard, it appears to be more like
'query-schema-encoding' in function. But not sure it's worth
renaming or anything so long as the documentation is clear.

> 
> 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".

Both sound reasonable now that I have a better idea of what's going
on. Range isn't necessary to convey the encoding, so it's okay if
that's not a part of the initial implementation. If it starts to seem
useful then we can add that later. If we think this might be the case
we should maybe take some precautions to allow for extensibility, but
it seems like SchemaInfoBuiltin could be extended in that regard, so
we should be okay.

> 
> >> 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.

Thanks, will do.

> 
> > 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.

Makes sense. Not sure what to think of 32 atm, but 31 seems reasonable
to me, whether that be approach 1) or 2) I don't have a strong
opinion on anymore.

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