[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 25/28] qapi: Make input visitors detect unvis
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v4 25/28] qapi: Make input visitors detect unvisited list tails |
Date: |
Fri, 3 Mar 2017 13:15:05 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 |
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 <address@hidden>
> ---
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
- Re: [Qemu-devel] [PATCH v4 06/28] qapi-introspect: Mangle --prefix argument properly for C, (continued)
- [Qemu-devel] [PATCH v4 04/28] qmp: Dumb down how we run QMP command registration, Markus Armbruster, 2017/03/03
- [Qemu-devel] [PATCH v4 12/28] qapi: Clean up after commit 3d344c2, Markus Armbruster, 2017/03/03
- [Qemu-devel] [PATCH v4 15/28] qapi: Drop string input visitor method optional(), Markus Armbruster, 2017/03/03
- [Qemu-devel] [PATCH v4 27/28] qapi: Fix object input visit beyond end of list, Markus Armbruster, 2017/03/03
- [Qemu-devel] [PATCH v4 17/28] qom: Make object_property_set_qobject()'s input visitor strict, Markus Armbruster, 2017/03/03
- [Qemu-devel] [PATCH v4 26/28] tests: Cover input visit beyond end of list, Markus Armbruster, 2017/03/03
- [Qemu-devel] [PATCH v4 23/28] tests: Cover partial input visit of list, Markus Armbruster, 2017/03/03
- [Qemu-devel] [PATCH v4 25/28] qapi: Make input visitors detect unvisited list tails, Markus Armbruster, 2017/03/03
- Re: [Qemu-devel] [PATCH v4 25/28] qapi: Make input visitors detect unvisited list tails,
Eric Blake <=
[Qemu-devel] [PATCH v4 24/28] test-qobject-input-visitor: Cover missing nested struct member, Markus Armbruster, 2017/03/03
[Qemu-devel] [PATCH v4 28/28] qapi: Improve qobject visitor documentation, Markus Armbruster, 2017/03/03
[Qemu-devel] [PATCH v4 21/28] test-string-input-visitor: Tear down existing test automatically, Markus Armbruster, 2017/03/03
[Qemu-devel] [PATCH v4 19/28] qapi: Drop unused non-strict qobject input visitor, Markus Armbruster, 2017/03/03
[Qemu-devel] [PATCH v4 02/28] libqtest: Work around a "QMP wants a newline" bug, Markus Armbruster, 2017/03/03
[Qemu-devel] [PATCH v4 22/28] test-string-input-visitor: Improve list coverage, Markus Armbruster, 2017/03/03