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

[Michael Roth]

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.

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

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

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.

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

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

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

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>