On 04/13/2016 11:38 AM, Markus Armbruster wrote: > Eric Blake <ebl...@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 <ebl...@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
signature.asc
Description: OpenPGP digital signature
