On 03/03/2017 06:32 AM, Markus Armbruster wrote: > Fix the design flaw demonstrated in the previous commit: new method > check_list() lets input visitors report that unvisited input remains > for a list, exactly like check_struct() lets them report that > unvisited input remains for a struct or union. > > Implement the method for the qobject input visitor (straightforward), > and the string input visitor (less so, due to the magic list syntax > there). The opts visitor's list magic is even more impenetrable, and > all I can do there today is a stub with a FIXME comment. No worse > than before. > > Signed-off-by: Markus Armbruster <arm...@redhat.com> > ---
Didn't I already review this one? Ah, there's my R-b: https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg07614.html But since it disappeared again, I had another look. > +++ b/qapi/qobject-input-visitor.c > @@ -51,7 +51,8 @@ static QObjectInputVisitor *to_qiv(Visitor *v) > return container_of(v, QObjectInputVisitor, visitor); > } > > -static const char *full_name(QObjectInputVisitor *qiv, const char *name) > +static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name, > + int n) > { > StackObject *so; > char buf[32]; > @@ -63,8 +64,10 @@ static const char *full_name(QObjectInputVisitor *qiv, > const char *name) > } > > QSLIST_FOREACH(so , &qiv->stack, node) { > - if (qobject_type(so->obj) == QTYPE_QDICT) { > - g_string_prepend(qiv->errname, name); > + if (n) { > + n--; > + } else if (qobject_type(so->obj) == QTYPE_QDICT) { > + g_string_prepend(qiv->errname, name ?: "<anonymous>"); > g_string_prepend_c(qiv->errname, '.'); > } else { > snprintf(buf, sizeof(buf), "[%u]", so->index); > @@ -72,18 +75,24 @@ static const char *full_name(QObjectInputVisitor *qiv, > const char *name) > } > name = so->name; > } > + assert(!n); If I'm reading this right, your use of n-- in the loop followed by the post-condition is to assert that QSLIST_FOREACH() iterated n times, but lets see what callers pass for n: > > if (name) { > g_string_prepend(qiv->errname, name); > } else if (qiv->errname->str[0] == '.') { > g_string_erase(qiv->errname, 0, 1); > - } else { > + } else if (!qiv->errname->str[0]) { > return "<anonymous>"; > } > > return qiv->errname->str; > } > > +static const char *full_name(QObjectInputVisitor *qiv, const char *name) > +{ > + return full_name_nth(qiv, name, 0); > +} One caller passes 0, > +static void qobject_input_check_list(Visitor *v, Error **errp) > +{ > + QObjectInputVisitor *qiv = to_qiv(v); > + StackObject *tos = QSLIST_FIRST(&qiv->stack); > + > + assert(tos && tos->obj && qobject_type(tos->obj) == QTYPE_QLIST); > + > + if (tos->entry) { > + error_setg(errp, "Only %u list elements expected in %s", > + tos->index + 1, full_name_nth(qiv, NULL, 1)); the other passes 1. No other calls. Did we really need an integer, where we use n--, or would a bool have done as well? At any rate, since I've already reviewed it once, you can add R-b, but we may want a followup to make it less confusing. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
