| Message ID | 1460131992-32278-6-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@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 <eblake@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--; }
On 04/13/2016 09:53 AM, Markus Armbruster wrote: > Eric Blake <eblake@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 <eblake@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)'. > > > 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.
On 04/13/2016 10:36 AM, Eric Blake wrote: >> 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. Scratch that, there's no error. The outer statement is an 'if', not a 'for', so there's nothing to break out of. The error gets set at most once, regardless of whether there are additional unvisited members.
Eric Blake <eblake@redhat.com> writes: > On 04/13/2016 09:53 AM, Markus Armbruster wrote: >> Eric Blake <eblake@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 <eblake@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.
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 */ const QListEntry *entry; - GHashTable *h; + + GHashTable *h; /* If obj is dict: remaining keys needing visits */ } 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). */ StackObject stack[QIV_STACK_SIZE]; int nb_stack; + + /* True to track whether all keys in QDict have been parsed. */ bool strict; }; @@ -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. */ + 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. */ + 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++;
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 <eblake@redhat.com> --- v14: no change v13: no change v12: new patch --- qapi/qmp-input-visitor.c | 52 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 15 deletions(-)