Re: [Qemu-devel] [PATCH 2/2] qapi: Improve qobject input visitor error reporting

[Eric Blake]

On 02/21/2017 05:00 AM, Markus Armbruster wrote:
> Error messages refer to nodes of the QObject being visited by name.
> Trouble is the names are sometimes less than helpful:
> 
> * The name of the root QObject is whatever @name argument got passed
>   to the visitor, except NULL gets mapped to "null".  We commonly pass
>   NULL.  Not good.
> 
>   Avoiding errors "at the root" mitigates.  For instance,
>   visit_start_struct() can only fail when the visited object is not a
>   dictionary, and we commonly ensure it is beforehand.
> 
> * The name of a QDict's member is the member key.  Good enough only
>   when this happens to be unique.
> 
> * The name of a QList's member is "null".  Not good.

At least we didn't crash.  But indeed it was not nice.

> 
> Improve error messages by referring to nodes by path instead, as
> follows:
> 
> * The path of the root QObject is whatever @name argument got passed
>   to the visitor, except NULL gets mapped to "<anonymous>".
> 
> * The path of a root QDict's member is the member key.
> 
> * The path of a root QList's member is "[%zu]", where %zu is the list
>   index, starting at zero.
> 
> * The path of a non-root QDict's member is the path of the QDict
>   concatenated with "." and the member key.
> 
> * The path of a non-root QList's member is the path of the QList
>   concatenated with "[%zu]", where %zu is the list index.

Nice!  We've mentioned this idea in the past, but I'm glad to see actual
code for it.

> 
> For example, the incorrect QMP command
> 
>     { "execute": "blockdev-add", "arguments": { "node-name": "foo", "driver": 
> "raw", "file": {"driver": "file" } } }
> 
> now fails with
> 
>     {"error": {"class": "GenericError", "desc": "Parameter 'file.filename' is 
> missing"}}
> 
> instead of
> 
>     {"error": {"class": "GenericError", "desc": "Parameter 'filename' is 
> missing"}}
> 
> and
> 
>     { "execute": "input-send-event", "arguments": { "device": "bar", 
> "events": [ [] ] } }
> 
> now fails with
> 
>     {"error": {"class": "GenericError", "desc": "Invalid parameter type for 
> 'events[0]', expected: object"}}
> 
> instead of
> 
>     {"error": {"class": "GenericError", "desc": "Invalid parameter type for 
> 'null', expected: QDict"}}

And that right there justifies this patch!

> 
> The qobject output visitor doesn't have this problem because it should
> not fail.  Same for dealloc and clone visitors.
> 
> The string visitors don't have this problem because they visit just
> one value, whose name needs to be passed to the visitor as @name.  The
> string output visitor shouldn't fail anyway.
> 
> The options visitor uses QemuOpts names.  Their name space is flat, so
> the use of QDict member keys as names is fine.  NULL names used with
> roots and lists could conceivably result in bad error messages.  Left
> for another day.
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  include/qapi/visitor.h       |   6 ---
>  qapi/qobject-input-visitor.c | 123 
> +++++++++++++++++++++++++++++++------------
>  2 files changed, 89 insertions(+), 40 deletions(-)
> 
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 9bb6cba..7c91a50 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -66,12 +66,6 @@
>   * object, @name is the key associated with the value; and when
>   * visiting a member of a list, @name is NULL.
>   *
> - * FIXME: Clients must pass NULL for @name when visiting a member of a
> - * list, but this leads to poor error messages; it might be nicer to
> - * require a non-NULL name such as "key.0" for '{ "key": [ "value" ]
> - * }' if an error is encountered on "value" (or to have the visitor
> - * core auto-generate the nicer name).

Aha - I _knew_ we'd talked about it before :)

Nice change.
Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature