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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 05/02/2016 12:20 PM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> 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.

Let's do it early.

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

Fewer casts is good, but symmetry is also good.

>>>
>>>      /* 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).

Okay, I can buy this one.

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

Okay, I'm confused.

Consider BlockdevRef, defined as

    { 'alternate': 'BlockdevRef',
      'data': { 'definition': 'BlockdevOptions',
                'reference': 'str' } }

where BlockdevOptions is a (flat) union.  Let's clone a BlockdevRef
holding a str.  Sequence of calls:

    qapi_BlockdevRef_clone(src)
        qapi_clone_visitor_new(src)
        // qcv->depth is now 0
        visit_type_BlockdevRef(v, NULL, &dst, &error_abort)
            visit_start_alternate(v, NULL, &dst,
                                  sizeof(BlockdevRef), true, &error_abort)
                qapi_clone_start_alternate(v, NULL, &dst,
                                  sizeof(BlockdevRef), true, &error_abort)
                    qapi_clone_start_struct(v, NULL, &dst,
                                  sizeof(BlockdevRef), &error_abort)
                        // does not increment qcv->depth
        visit_type_str(v, NULL, &dst->u.references, &error_abort)
            qapi_clone_type_str(v, NULL, &dst->u.references, &error_abort)
                assert(qcv->depth)  // why does this hold?

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