Eric Blake <ebl...@redhat.com> writes: > On 04/13/2016 09:53 AM, Markus Armbruster wrote: >> Eric Blake <ebl...@redhat.com> writes: >> >>> Management of the top of stack was a bit verbose; creating a >>> temporary variable and adding some comments makes the existing >>> code more legible before the next few patches improve things. >>> No semantic changes other than asserting that we are always >>> visiting a QObject, and not a NULL value. >>> >>> Signed-off-by: Eric Blake <ebl...@redhat.com> >>> >>> --- > >> >> The mixture of block comments and comments to the right is a bit >> awkward. What about: >> >> typedef struct StackObject { >> QObject *obj; /* Object being visited */ >> >> GHashTable *h; /* if obj is dict: unvisited keys */ >> const QListEntry *entry; /* if obj is list: unvisited tail */ >> } StackObject; >> > > Works for me. > >>> >>> struct QmpInputVisitor >>> { >>> Visitor visitor; >>> + >>> + /* Stack of objects being visited. stack[0] is root of visit, >>> + * stack[1] and below correspond to visit_start_struct (nested >>> + * QDict) and visit_start_list (nested QList). */ >> >> I guess what you want to say is stack[1..] record the nesting of >> start_struct() ... end_struct() and start_list() ... end_list() pairs. >> >> Comment gets rewritten in PATCH 17, no need to worry too much about it. >> >>> StackObject stack[QIV_STACK_SIZE]; >>> int nb_stack; >>> + >>> + /* True to track whether all keys in QDict have been parsed. */ >>> bool strict; >> >> I think @strict switches on rejection of unexpected dictionary keys. >> See qmp_input_pop() below. >> >> I dislike the fact that we have two input visitors, and the one with the >> obvious name ignores certain errors. I don't doubt that it has its >> uses, but reporting errors should be the default, and ignoring them >> should be a conscious decision. Anyway, not this patch's problem. > > Dan also has a pending patch that reworks it to add yet another > parameter (the ability to take input in string format and auto-convert > it to the correct type). In that one, he exposes a third method for > choosing which visitor you get, and which then under the hood call a > helper with two boolean flags. Maybe it's time to just convert all > clients to always passing the parameters they want, along with auditing > whether ignoring extra input is a sane option for that client - but as > you say, it's fine for a separate patch. > >>> + >>> + /* If we have a name, and we're in a dictionary, then return that >>> + * value. */ >> >> Can we be in a dictionary and not have a name? > > The converse happens: we can certainly have a name and not be in a > dictionary, for a top-level visit. But it has weird semantics until I > clean it up later in 17/19. For this patch, it was just code motion and > documentation (the 'if (name && qobject_type...)' condition here is the > same pre- and post-patch), where I was just getting rid of a dead 'if > (qobj)'.
So "in dictionary" implies "have a name". Why do we bother to test @name then? >> >> >> static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp) >> { >> assert(qiv->nb_stack > 0); >> >> if (qiv->strict) { >> GHashTable * const top_ht = qiv->stack[qiv->nb_stack - 1].h; >> if (top_ht) { >> GHashTableIter iter; >> const char *key; >> >> g_hash_table_iter_init(&iter, top_ht); >> if (g_hash_table_iter_next(&iter, (void **)&key, NULL)) { >> error_setg(errp, QERR_QMP_EXTRA_MEMBER, key); >> >> This looks wrong. If we have more than one extra members, the second >> call error_setg() will fail an assertion in error_setv(), unless errp is >> null. > > Whoops - looks like f96493b1 is broken for missing a 'break' statement. > I'll send that as a separate for-2.6 cleanup that we should pull sooner > rather than later. False alarm, as you noticed.