Re: [Qemu-devel] [PATCH v14 07/19] qmp-input: Refactor when list is advanced

[Eric Blake <eblake@redhat.com>]

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

On 04/13/2016 11:38 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Refactor the code to advance the QListEntry pointer at the point
>> where visit_type_FOO() is called, rather than visit_next_list().
>> This will allow a future patch to move the visit of the list head
>> into visit_start_list(), and get rid of the 'first' flag.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> After having read the patch, I think the change is from this sequence of
> states
> 
>            start_list next_list type_ELT ... next_list type_ELT end_list
>     entry  NULL       1st elt   1st elt      last elt  last elt gone
> 
> where type_ELT() returns (entry ? entry : 1st elt) and next_list() steps
> entry
> 
> to this one
> 
>            start_list next_list type_ELT ... next_list type_ELT end_list
>     entry  1st elt    1nd elt   2nd elt      last elt  NULL     gone
> 
> where type_ELT() steps entry and returns the old entry, and next_list()
> leaves entry alone.  Correct?

Yes. I think I know what diagram I'll be adding to my commit message :)

> 
> Now have a look at the typical generated visit:

Which gets greatly simplified later in 18/19 (ie. this patch is doing
some of the refactoring to make 18/19 easier).

> 
>     void visit_type_FOOList(Visitor *v, const char *name, FOOList **obj, Error **errp)
>     {
>         Error *err = NULL;
>         FOOList *tail;
>         size_t size = sizeof(**obj);
> 
>         visit_start_list(v, name, (GenericList **)obj, size, &err);
>         if (err) {
>             goto out;
>         }
>         for (tail = *obj;
>              tail;
>              tail = (FOOList *)visit_next_list(v, (GenericList *)tail, size)) {
>             visit_type_FOO(v, NULL, &tail->value, &err);
>             if (err) {
>                 break;
>             }
>         }
>         if (visit_end_list(v) && err) {
>             qapi_free_FOOList(*obj);
>             *obj = NULL;
>         }
>     out:
>         error_propagate(errp, err);
>     }
> 
> While the loop variable is still stepped in the for's third expression
> as it should, the actual stepping below the hood now happens in the loop
> body.  I find this mildly confusing.

There are TWO lists being stepped.

The generated code is stepping through the GenericList *.  The
under-the-hood code is stepping through QList input.  My refactoring
here is to advance through the under-the-hood QList at the point where
we CONSUME data (similar to how we remove things from QDict at the point
where we CONSUME it) - during visit_type_FOO, and doesn't touch the
GenericList* visits.  Yes, that means the two are slightly out of sync,
until 18/19 fixes the GenericList* visit to be sane (none of this magic
visit the head of the list twice during visit_next_list).

> 
> Perhaps a more natural loop would be
> 
>     void visit_type_FOOList(Visitor *v, const char *name, FOOList **obj, Error **errp)
>     {
>         Error *err = NULL;
>         GenericList *tail;
>         size_t size = sizeof(**obj);
> 
>         visit_start_list(v, name, (GenericList **)obj, size, &err);
>         if (err) {
>             goto out;
>         }
>         while ((tail = visit_next_list(v, tail, size))) {

tail has to be set to something before this while loop will work.

>             visit_type_FOO(v, NULL, &((FOOList *)tail)->value, &err);
>             if (err) {
>                 break;
>             }
>         }

And that's kind of the whole point of 18/19.

>         if (visit_end_list(v) && err) {
>             qapi_free_FOOList(*obj);
>             *obj = NULL;
>         }
>     out:
>         error_propagate(errp, err);
>     }
> 
> Might also permit de-duplicating the g_malloc0(size), since we now call
> next_list() *before* each iteration instead of *between* iterations.

One other wrinkle to think about in both this patch and 18/19: We have
the annoying difference in representation that an empty GenericList* is
the NULL pointer, while an empty QList is an actual object.

>> @@ -77,7 +78,12 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
>        /* If we are in the middle of a list, then return the next element
>>       * of the list.  */
>>      if (tos->entry) {
> 
> Before the patch, the condition matches the comment: tos->entry implies
> this is a list and we're not at its head.
> 
> After the patch, the condition only implies it's a list, but ...
> 
>>          assert(qobject_type(qobj) == QTYPE_QLIST);
>> -        return qlist_entry_obj(tos->entry);
>> +        assert(!tos->first);
> 
> ... you assert that "not at its head" can't happen.  Why is that the
> case?

Because the only way to consume an element is by calling
visit_type_FOO(), but until 18/19 reorders the generated loop, we don't
call visit_type_FOO() until AFTER our first visit_next_list().  The only
time "at its head" can happen is BEFORE the first visit_next_list().

> 
>> +        qobj = qlist_entry_obj(tos->entry);
>> +        if (consume) {
>> +            tos->entry = qlist_next(tos->entry);
>> +        }
>> +        return qobj;
> 
> The remainder of the hunk adds the qlist_next() call moved from
> qmp_input_next_list(), as advertized by the commit message.  Good.
> 
>>      }
>>
>>      /* Otherwise, we are at the root of the visit or the start of a
>> @@ -91,7 +97,8 @@ static void qdict_add_key(const char *key, QObject *obj, void *opaque)
>>      g_hash_table_insert(h, (gpointer) key, NULL);
>>  }
>>
>> -static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj, Error **errp)
>> +static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj,
>> +                           const QListEntry *entry, Error **errp)
>>  {
>>      GHashTable *h;
>>      StackObject *tos = &qiv->stack[qiv->nb_stack];
>> @@ -103,7 +110,8 @@ static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj, Error **errp)
>>      }
>>
>>      tos->obj = obj;
>> -    tos->entry = NULL;
>> +    tos->entry = entry;
>> +    tos->first = true;
>>      tos->h = NULL;
>>
>>      if (qiv->strict && qobject_type(obj) == QTYPE_QDICT) {
> 
> Here, you change the initial state.  Not explicitly advertized in the
> commit message, unlike the change to qlist_next().  I guess that's okay,
> but perhaps my ramblings give you ideas on improving the commit message.

Sure.

> 
> I wonder whether we really need the new argument.  Could we do something
> like
> 
>        if (qobject_type(obj) == QTYPE_QLIST) {
>            tos->entry = qlist_first(qobject_to_qlist(obj));
>            tos->first = true;
>        }
> 

I'll try it.  But I seem to recall the reason I did it this way was
because...

> ?
> 
>> @@ -153,7 +161,7 @@ static void qmp_input_start_struct(Visitor *v, const char *name, void **obj,
>>          return;
>>      }
>>
>> -    qmp_input_push(qiv, qobj, &err);
>> +    qmp_input_push(qiv, qobj, NULL, &err);
> 
> Since @qobj is a dictionary, the StackObject's entry will be unused, so
> pass null.  Okay.
> 
>>      if (err) {
>>          error_propagate(errp, err);
>>          return;
>> @@ -175,6 +183,7 @@ static void qmp_input_start_list(Visitor *v, const char *name, Error **errp)
>>  {
>>      QmpInputVisitor *qiv = to_qiv(v);
>>      QObject *qobj = qmp_input_get_object(qiv, name, true);
>> +    const QListEntry *entry;
>>
>>      if (!qobj || qobject_type(qobj) != QTYPE_QLIST) {
>>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>> @@ -182,7 +191,8 @@ static void qmp_input_start_list(Visitor *v, const char *name, Error **errp)
>>          return;
>>      }
>>
>> -    qmp_input_push(qiv, qobj, errp);
>> +    entry = qlist_first(qobject_to_qlist(qobj));
>> +    qmp_input_push(qiv, qobj, entry, errp);
> 
> @qobj is a list, the StackObject's entry must be initialized to its
> first member, so pass that.

...in 18/19, the semantics of visit_start_list() change such that we
have to know whether we are visiting an empty QList (we set **obj
differently for an empty vs. non-empty list) - so it was either grab
entry locally and pass it in as a parameter, or let qmp_input_push()
populate entry and then read it after the fact.

>> @@ -382,7 +384,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
>>      v->visitor.type_any = qmp_input_type_any;
>>      v->visitor.optional = qmp_input_optional;
>>
>> -    qmp_input_push(v, obj, NULL);
>> +    qmp_input_push(v, obj, NULL, NULL);
> 
> @obj is the root of the visit.  Can it be a list?
> 
> If yes, then StackObject's entry is not its first element.  Why is that
> correct?

Because when the root of the visit is a QList, you STILL have to call
visit_start_list() before visiting list element members.  It is
visit_start_list() that sets up entry in preparation for visit_next_list().

The root of the QMP input visitor is currently a bit odd; we have weird
semantics where a dictionary as root behaves differently depending on
whether name is NULL or non-NULL.  See 17/19, where I clean that up
later in the series, at which point I get rid of the qmp_input_push()
during visitor initialization.

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