Re: [Qemu-devel] [PATCH v3 18/18] qapi: Add parameter to visit_end_*

[Eric Blake <eblake@redhat.com>]

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

On 05/02/2016 12:20 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Rather than making the dealloc visitor track of stack of pointers
>> remembered during visit_start_* in order to free them during
>> visit_end_*, it's a lot easier to just make all callers pass the
>> same pointer to visit_end_*.  The generated code has access to the
>> same pointer, while all other users are doing virtual walks and
>> can pass NULL.  The dealloc visitor is then greatly simplified.
>>
>> The clone visitor also gets a minor simplification of not having
>> to track quite as much depth.
> 
> Do this before adding the clone visitor, to reduce churn?

I could. I first thought of it after, and kept it there as a
justification for keeping it (multiple visitors benefit), but even if
rebased earlier, the commit message can still mention that it will make
the clone visitor easier.

>> @@ -59,7 +59,7 @@ struct Visitor
>>      GenericList *(*next_list)(Visitor *v, GenericList *tail, size_t size);
>>
>>      /* Must be set */
>> -    void (*end_list)(Visitor *v);
>> +    void (*end_list)(Visitor *v, void **list);
> 
> Sure you want void ** and not GenericList **?

Yes. There are only two callers: dtc code passes NULL (where the type
doesn't matter), and generated visit_type_FOOList() has to already cast
to GenericList() during visit_start_list(); accepting void** here
instead of GenericList** removes the need for a second instance of that
cast.

> 
>>
>>      /* Must be set by input and dealloc visitors to visit alternates;
>>       * optional for output visitors. */
>> @@ -68,7 +68,7 @@ struct Visitor
>>                              bool promote_int, Error **errp);
>>
>>      /* Optional, needed for dealloc visitor */
>> -    void (*end_alternate)(Visitor *v);
>> +    void (*end_alternate)(Visitor *v, void **obj);
> 
> Sure you want void ** and not GenericAlternate **?

Only one caller: generated code. Same story that we already have to cast
during visit_start_alternate(), so using void** here avoids the need for
a second cast.

Oh, and the clone visitor was easier to write with a single function
that takes void** for all three visit_end() implementations (whereas I'd
have to write three functions identical except for signature otherwise).

>> +++ b/qapi/qapi-clone-visitor.c
>> @@ -29,11 +29,8 @@ static void qapi_clone_start_struct(Visitor *v, const char *name, void **obj,
>>      QapiCloneVisitor *qcv = to_qcv(v);
>>
>>      if (!obj) {
>> -        /* Nothing to allocate on the virtual walk during an
>> -         * alternate, but we still have to push depth.
>> -         * FIXME: passing obj to visit_end_struct would make this easier */
>> +        /* Nothing to allocate on the virtual walk */
>>          assert(qcv->depth);
>> -        qcv->depth++;
>>          return;
>>      }
>>
> 
> Why can we elide qcv->depth++?  Do the assert(qcv->qdepth) still hold?

Yes, the assertion still holds: we can only reach this code underneath
visit_start_alternate(), ...

> 
>> @@ -41,11 +38,13 @@ static void qapi_clone_start_struct(Visitor *v, const char *name, void **obj,
>>      qcv->depth++;
>>  }
>>
>> -static void qapi_clone_end(Visitor *v)
>> +static void qapi_clone_end(Visitor *v, void **obj)
>>  {
>>      QapiCloneVisitor *qcv = to_qcv(v);
>>      assert(qcv->depth);
>> -    qcv->depth--;
>> +    if (obj) {
>> +        qcv->depth--;
>> +    }

...and this is the matching elision of the depth manipulations.

>> +++ b/qapi/qmp-input-visitor.c
>> @@ -145,7 +145,7 @@ static void qmp_input_check_struct(Visitor *v, Error **errp)
>>      }
>>  }
>>
>> -static void qmp_input_pop(Visitor *v)
>> +static void qmp_input_pop(Visitor *v, void **obj)
>>  {
>>      QmpInputVisitor *qiv = to_qiv(v);
>>      StackObject *tos = &qiv->stack[qiv->nb_stack - 1];
> 
> You could assert @obj matches tos->obj.  Same for the other visitors
> that still need a stack.

And brings me back to my question of whether qapi-visit-core.c should
maintain its own stack for asserting these types of sanity checks for
ALL callers, even when the visitor itself doesn't need a stack.

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