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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 02/26/2017 03:43 PM, Markus Armbruster wrote:
>> Error messages refer to nodes of the QObject being visited by name.
>> Trouble is the names are sometimes less than helpful:
>> 
>
>> 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.
>
> %zu here, but...
>
>
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  include/qapi/visitor.h       |   6 ---
>>  qapi/qobject-input-visitor.c | 121 
>> +++++++++++++++++++++++++++++++------------
>>  2 files changed, 87 insertions(+), 40 deletions(-)
>> 
>
>>  
>> -typedef struct StackObject
>> -{
>> -    QObject *obj; /* Object being visited */
>> +typedef struct StackObject {
>> +    const char *name;            /* Name of @obj in its parent, if any */
>> +    QObject *obj;                /* QDict or QList being visited */
>>      void *qapi; /* sanity check that caller uses same pointer */
>>  
>> -    GHashTable *h;           /* If obj is dict: unvisited keys */
>> -    const QListEntry *entry; /* If obj is list: unvisited tail */
>> +    GHashTable *h;              /* If @obj is QDict: unvisited keys */
>> +    const QListEntry *entry;    /* If @obj is QList: unvisited tail */
>> +    unsigned index;             /* If @obj is QList: list index of @entry */
>
>> +    QSLIST_FOREACH(so , &qiv->stack, node) {
>> +        if (qobject_type(so->obj) == QTYPE_QDICT) {
>> +            g_string_prepend(qiv->errname, name);
>> +            g_string_prepend_c(qiv->errname, '.');
>> +        } else {
>> +            snprintf(buf, sizeof(buf), "[%u]", so->index);
>
> ...%u here.  Minor inconsistency in the commit message, but I'm okay
> mere 'unsigned' in the code, as we'd probably have other problems long
> before hitting a list with 2 billion entries, even on platforms where
> size_t is 64-bit.

Good catch.  I'll touch up the commit message.

> Reviewed-by: Eric Blake <address@hidden>

Thanks!