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>
>
> ---
> v14: no change
> v13: no change
> v12: new patch
> ---
> qapi/qmp-input-visitor.c | 52
> ++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 37 insertions(+), 15 deletions(-)
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 77cce8b..7ba6d3d 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -25,16 +25,26 @@
>
> typedef struct StackObject
> {
> - QObject *obj;
> + QObject *obj; /* Object being visited */
> +
> + /* If obj is list: NULL if list is at head, otherwise tail of list
> + * still needing visits */
"still needing visits?"
> const QListEntry *entry;
> - GHashTable *h;
> +
> + GHashTable *h; /* If obj is dict: remaining keys needing visits */
Ah, now I get it. Swap the two?
> } StackObject;
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;
>
> 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.
> };
>
> @@ -47,19 +57,29 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
> const char *name,
> bool consume)
> {
> - QObject *qobj = qiv->stack[qiv->nb_stack - 1].obj;
> + StackObject *tos = &qiv->stack[qiv->nb_stack - 1];
> + QObject *qobj = tos->obj;
>
> - if (qobj) {
> - if (name && qobject_type(qobj) == QTYPE_QDICT) {
> - if (qiv->stack[qiv->nb_stack - 1].h && consume) {
> - g_hash_table_remove(qiv->stack[qiv->nb_stack - 1].h, name);
> - }
> - return qdict_get(qobject_to_qdict(qobj), name);
> - } else if (qiv->stack[qiv->nb_stack - 1].entry) {
> - return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry);
> + assert(qobj);
> +
> + /* 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?
Either one...
> + if (name && qobject_type(qobj) == QTYPE_QDICT) {
> + if (tos->h && consume) {
> + g_hash_table_remove(tos->h, name);
> }
> + return qdict_get(qobject_to_qdict(qobj), name);
> }
>
> + /* If we are in the middle of a list, then return the next element
> + * of the list. */
... or two spaces between end of sentence and */. I like the
old-fashioned double space between sentences myself, but not before */.
Regardless of what I like, we should try to be consistent.
I got used to winged comments, where this issue is moot.
> + if (tos->entry) {
> + assert(qobject_type(qobj) == QTYPE_QLIST);
> + return qlist_entry_obj(tos->entry);
> + }
> +
> + /* Otherwise, we are at the root of the visit or the start of a
> + * list, and return the object as-is. */
> return qobj;
> }
>
> @@ -72,20 +92,22 @@ static void qdict_add_key(const char *key, QObject *obj,
> void *opaque)
> static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj, Error **errp)
> {
> GHashTable *h;
> + StackObject *tos = &qiv->stack[qiv->nb_stack];
>
> + assert(obj);
> if (qiv->nb_stack >= QIV_STACK_SIZE) {
> error_setg(errp, "An internal buffer overran");
> return;
> }
>
> - qiv->stack[qiv->nb_stack].obj = obj;
> - qiv->stack[qiv->nb_stack].entry = NULL;
> - qiv->stack[qiv->nb_stack].h = NULL;
> + tos->obj = obj;
> + tos->entry = NULL;
> + tos->h = NULL;
>
> if (qiv->strict && qobject_type(obj) == QTYPE_QDICT) {
> h = g_hash_table_new(g_str_hash, g_str_equal);
> qdict_iter(qobject_to_qdict(obj), qdict_add_key, h);
> - qiv->stack[qiv->nb_stack].h = h;
> + tos->h = h;
> }
>
> qiv->nb_stack++;
}
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.
}
g_hash_table_unref(top_ht);
}
}
qiv->nb_stack--;
}
