Re: [Qemu-devel] [PATCH v6 05/23] qmp: Fix reference-counting of qnull on empty output visit

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4256 bytes --]

On 12/03/2015 10:50 AM, Markus Armbruster wrote:

>>> So, how does this contraption work?
>>>
>>> A visitor cab encounter NULL only when it visits pointers (d'oh!).
>>> Searching qapi-visit-core.c for **obj finds start_struct(),
>>> start_implicit_struct(), type_str(), type_any().
>>>
>>> As far as I can tell, start_implicit_struct() is for unboxed structs, so
>>> NULL must not happen there.
>>
>> You are correct that start_implicit_struct is for unboxed substructs
>> (the branch of a flat union).  And we should never get here with it
>> being NULL (although until commit 20/23 of this series, it was because
>> we are abusing the 'data' member of the union during visit_start_union()).
> 
> We should spell out "*obj isn't null" in visit_start_implicit_struct()'s
> contract.  I'd assert it for good measure.

I've spent the better part of my afternoon playing with asserts to see
what works and what doesn't.  So far, I've learned:

We have input visitors that allow visit_start_struct(v, NULL, ...) (see
vl.c's use of opts-visitor).  This is done when we want to parse from
some other format into what qapi would accept by hierarchical
visit_type_FOO(), but don't actually have a qapi type to parse into.

We have output visitors that allow visit_start_struct(v, NULL, ...)
(most uses of visit_start outside of generated qapi-visit).  This is
done when we want to create certain output but don't have a qapi type,
so we instead are visiting types by hand.

We are inconsistent on how we pass things.  I think that ALL callers of
visit_start_struct() should pass one of two sets:
 valid type name, sizeof(that type), non-NULL obj (where input visitors
will do *obj = g_new0(size) if no other error is present; and where
output visitors should have an already-complete *obj)
or:
 NULL type name, size 0, NULL obj
But we aren't there yet - several clients have to be tweaked.

I'm also considering changing the signature of visit_start_struct() to
bundle the type name next to the type size (the current code inserts the
dictionary key name in between the two, making life a bit more awkward).

>>> qmp_output_type_any() crashes on NULL.  Can this happen?
>>
>> Again, if the QObject is trying to represent NULL, it does so with
>> qnull() (a non-null QObject), so we should never pass in NULL.  We
>> aren't using 'any' very heavily, so I doubt we have any broken clients.

And I argued in the other thread that the spapr prop_get_fdt() abuse
should probably explicitly use visit_type_any() with qnull() as its
subject, instead of relying on no visit at all, if it turns out that we
like 'null' for missing fdt as distinct from an fdt that is present but
empty.

> 
> A visit_type_FOO() visits a variable whose type is the C representation
> of a FOOish QAPI type, and its obj parameter is a pointer to the C
> representation.

Or NULL when there is no qapi type, and we are just using the
visit_type_* to parse or output things manually without the intermediate
qapi representation.


> For now, I think we should simply spell out the restrictions in
> visit_type_any()'s contract.  We should revisit it (haha) when we
> rethink null in QAPI.

Yes, this thread has given me ideas on how to improve my 7/23 patch
documentation of the visitor interfaces.

> Again, let's simply spell out the mess in the contract for now.

Or since we're (soon) at the start of 2.6 development, add some asserts,
with the ability to fix bugs or revert the asserts closer to that
release as needed.


> Taken together, prop_get_fdt() returns null | T.
> 
> Note that the value's structure is *dynamic*.  The best a statically
> defined QAPI schema can do to describe it is 'any'.

Or possibly an alternate.

> Back to the QMP output visitor: is asking the QMP output visitor for the
> visit's value when you haven't visited anything a sane thing to do?  I
> doubt it.  I think we should go back to the initial behavior, and find
> and fix the code that misuses visitors that way.

Sounds like adding assertions and fixing up fallout is worth attempting,
then.


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]