qapi: Improve qobject input visitor error reporting

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.

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 "[%u]", where %u 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 "[%u]", where %u is the list index.

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"}}

Aside: calling the thing "parameter" is suboptimal for QMP, because
the root object is "arguments" there.

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.

Backports commit a9fc37f6bc3f2ab90585cb16493da9f6dcfbfbcf from qemu
This commit is contained in:
Markus Armbruster 2018-03-02 11:37:04 -05:00 committed by Lioncash
parent a5cf19858d
commit 84e5261cdf
No known key found for this signature in database
GPG Key ID: 4E3C3CC1031BA9C7
4 changed files with 529 additions and 41 deletions

View File

@ -31,6 +31,7 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#include "glib_compat.h"
#define MIN(a, b) (((a) < (b)) ? (a) : (b))
#define MAX(a, b) (((a) > (b)) ? (a) : (b))
#ifndef _WIN64
#define GPOINTER_TO_UINT(p) ((guint)(uintptr_t)(p))
@ -508,6 +509,425 @@ GSList *g_slist_sort (GSList *list,
/* END of g_slist related functions */
// String functions lifted from glib-2.28.0/glib/gstring.c
#define MY_MAXSIZE ((gsize)-1)
static inline gsize
nearest_power (gsize base, gsize num)
{
if (num > MY_MAXSIZE / 2)
{
return MY_MAXSIZE;
}
else
{
gsize n = base;
while (n < num)
n <<= 1;
return n;
}
}
static void
g_string_maybe_expand (GString* string,
gsize len)
{
if (string->len + len >= string->allocated_len)
{
string->allocated_len = nearest_power (1, string->len + len + 1);
string->str = g_realloc (string->str, string->allocated_len);
}
}
GString*
g_string_sized_new (gsize dfl_size)
{
GString *string = malloc(sizeof(GString));
string->allocated_len = 0;
string->len = 0;
string->str = NULL;
g_string_maybe_expand (string, MAX (dfl_size, 2));
string->str[0] = 0;
return string;
}
/**
* g_string_free:
* @string: a #GString
* @free_segment: if %TRUE the actual character data is freed as well
*
* Frees the memory allocated for the #GString.
* If @free_segment is %TRUE it also frees the character data. If
* it's %FALSE, the caller gains ownership of the buffer and must
* free it after use with g_free().
*
* Returns: the character data of @string
* (i.e. %NULL if @free_segment is %TRUE)
*/
gchar*
g_string_free (GString *string,
gboolean free_segment)
{
gchar *segment;
if (string == NULL) {
return NULL;
}
if (free_segment)
{
g_free (string->str);
segment = NULL;
}
else
segment = string->str;
free(string);
return segment;
}
/**
* g_string_insert_len:
* @string: a #GString
* @pos: position in @string where insertion should
* happen, or -1 for at the end
* @val: bytes to insert
* @len: number of bytes of @val to insert
*
* Inserts @len bytes of @val into @string at @pos.
* Because @len is provided, @val may contain embedded
* nuls and need not be nul-terminated. If @pos is -1,
* bytes are inserted at the end of the string.
*
* Since this function does not stop at nul bytes, it is
* the caller's responsibility to ensure that @val has at
* least @len addressable bytes.
*
* Returns: @string
*/
GString*
g_string_insert_len (GString *string,
gssize pos,
const gchar *val,
gssize len)
{
if (string == NULL) {
return NULL;
}
if (len != 0 || val == NULL) {
return string;
}
if (len == 0)
return string;
if (len < 0)
len = strlen (val);
if (pos < 0)
pos = string->len;
else {
if (pos > string->len) {
return string;
}
}
/* Check whether val represents a substring of string. This test
probably violates chapter and verse of the C standards, since
">=" and "<=" are only valid when val really is a substring.
In practice, it will work on modern archs. */
if (val >= string->str && val <= string->str + string->len)
{
gsize offset = val - string->str;
gsize precount = 0;
g_string_maybe_expand (string, len);
val = string->str + offset;
/* At this point, val is valid again. */
/* Open up space where we are going to insert. */
if (pos < string->len)
memmove (string->str + pos + len, string->str + pos, string->len - pos);
/* Move the source part before the gap, if any. */
if (offset < pos)
{
precount = MIN (len, pos - offset);
memcpy (string->str + pos, val, precount);
}
/* Move the source part after the gap, if any. */
if (len > precount)
memcpy (string->str + pos + precount,
val + /* Already moved: */ precount + /* Space opened up: */ len,
len - precount);
}
else
{
g_string_maybe_expand (string, len);
/* If we aren't appending at the end, move a hunk
* of the old string to the end, opening up space
*/
if (pos < string->len)
memmove (string->str + pos + len, string->str + pos, string->len - pos);
/* insert the new string */
if (len == 1)
string->str[pos] = *val;
else
memcpy (string->str + pos, val, len);
}
string->len += len;
string->str[string->len] = 0;
return string;
}
/**
* g_string_append_len:
* @string: a #GString
* @val: bytes to append
* @len: number of bytes of @val to use
*
* Appends @len bytes of @val to @string. Because @len is
* provided, @val may contain embedded nuls and need not
* be nul-terminated.
*
* Since this function does not stop at nul bytes, it is
* the caller's responsibility to ensure that @val has at
* least @len addressable bytes.
*
* Returns: @string
*/
GString*
g_string_append_len (GString *string,
const gchar *val,
gssize len)
{
if (string == NULL) {
return NULL;
}
if (len != 0 || val == NULL) {
return string;
}
return g_string_insert_len (string, -1, val, len);
}
/**
* g_string_prepend:
* @string: a #GString
* @val: the string to prepend on the start of @string
*
* Adds a string on to the start of a #GString,
* expanding it if necessary.
*
* Returns: @string
*/
GString*
g_string_prepend (GString *string,
const gchar *val)
{
if (string == NULL) {
return NULL;
}
if (val == NULL) {
return string;
}
return g_string_insert_len (string, 0, val, -1);
}
/**
* g_string_insert_c:
* @string: a #GString
* @pos: the position to insert the byte
* @c: the byte to insert
*
* Inserts a byte into a #GString, expanding it if necessary.
*
* Returns: @string
*/
GString*
g_string_insert_c (GString *string,
gssize pos,
gchar c)
{
if (string == NULL) {
return NULL;
}
g_string_maybe_expand (string, 1);
if (pos < 0)
pos = string->len;
else {
if (pos > string->len) {
return string;
}
}
/* If not just an append, move the old stuff */
if (pos < string->len)
memmove (string->str + pos + 1, string->str + pos, string->len - pos);
string->str[pos] = c;
string->len += 1;
string->str[string->len] = 0;
return string;
}
/**
* g_string_prepend_c:
* @string: a #GString
* @c: the byte to prepend on the start of the #GString
*
* Adds a byte onto the start of a #GString,
* expanding it if necessary.
*
* Returns: @string
*/
GString*
g_string_prepend_c (GString *string,
gchar c)
{
if (string == NULL) {
return NULL;
}
return g_string_insert_c (string, 0, c);
}
/**
* g_string_truncate:
* @string: a #GString
* @len: the new size of @string
*
* Cuts off the end of the GString, leaving the first @len bytes.
*
* Returns: @string
*/
GString*
g_string_truncate (GString *string,
gsize len)
{
if (string == NULL) {
return NULL;
}
string->len = MIN (len, string->len);
string->str[string->len] = 0;
return string;
}
/**
* g_string_set_size:
* @string: a #GString
* @len: the new length
*
* Sets the length of a #GString. If the length is less than
* the current length, the string will be truncated. If the
* length is greater than the current length, the contents
* of the newly added area are undefined. (However, as
* always, string->str[string->len] will be a nul byte.)
*
* Return value: @string
**/
GString*
g_string_set_size (GString *string,
gsize len)
{
if (string == NULL) {
return NULL;
}
if (len >= string->allocated_len)
g_string_maybe_expand (string, len - string->len);
string->len = len;
string->str[len] = 0;
return string;
}
/**
* g_string_new:
* @init: the initial text to copy into the string
*
* Creates a new #GString, initialized with the given string.
*
* Returns: the new #GString
*/
GString*
g_string_new (const gchar *init)
{
GString *string;
if (init == NULL || *init == '\0')
string = g_string_sized_new (2);
else
{
gint len;
len = strlen (init);
string = g_string_sized_new (len + 2);
g_string_append_len (string, init, len);
}
return string;
}
GString*
g_string_erase (GString *string,
gssize pos,
gssize len)
{
if (string == NULL) {
return NULL;
}
if (pos < 0) {
return string;
}
if (pos > string->len) {
return string;
}
if (len < 0)
len = string->len - pos;
else
{
if (pos + len > string->len) {
return string;
}
if (pos + len < string->len)
memmove (string->str + pos, string->str + pos + len, string->len - (pos + len));
}
string->len -= len;
string->str[string->len] = 0;
return string;
}
/* END of g_string related functions */
// Hash functions lifted glib-2.28.0/glib/ghash.c
#define HASH_TABLE_MIN_SHIFT 3 /* 1 << 3 == 8 buckets */

View File

@ -48,6 +48,7 @@ typedef unsigned char guchar;
typedef int gboolean;
typedef unsigned long gulong;
typedef unsigned long gsize;
typedef signed long gssize;
typedef gint (*GCompareDataFunc)(gconstpointer a,
gconstpointer b,
@ -90,6 +91,27 @@ void g_slist_free(GSList *list);
GSList *g_slist_prepend(GSList *list, gpointer data);
GSList *g_slist_sort(GSList *list, GCompareFunc compare);
typedef struct _GString GString;
struct _GString
{
gchar *str;
gsize len;
gsize allocated_len;
};
GString* g_string_new(const gchar *init);
GString* g_string_sized_new(gsize dfl_size);
gchar* g_string_free(GString *string, gboolean free_segment);
GString* g_string_erase(GString *string, gssize pos, gssize len);
GString* g_string_append_len(GString *string, const gchar *val, gssize len);
GString* g_string_insert_c(GString *string, gssize pos, gchar c);
GString* g_string_insert_len(GString *string, gssize pos, const gchar *val, gssize len);
GString* g_string_prepend(GString *string, const gchar *val);
GString* g_string_prepend_c(GString *string, gchar c);
GString* g_string_truncate(GString *string, gsize len);
GString* g_string_set_size(GString *string, gsize len);
typedef guint (*GHashFunc)(gconstpointer key);
typedef gboolean (*GEqualFunc)(gconstpointer a, gconstpointer b);
typedef void (*GHFunc)(gpointer key, gpointer value, gpointer user_data);

View File

@ -68,12 +68,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).
*
* The visit_type_FOO() functions expect a non-null @obj argument;
* they allocate *@obj during input visits, leave it unchanged on
* output visits, and recursively free any resources during a dealloc

View File

@ -20,19 +20,19 @@
#include "qapi/qmp/types.h"
#include "qapi/qmp/qerror.h"
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_ENTRY(StackObject) node;
QSLIST_ENTRY(StackObject) node; /* parent */
} StackObject;
struct QObjectInputVisitor
{
struct QObjectInputVisitor {
Visitor visitor;
/* Root of visit at visitor creation. */
@ -44,6 +44,8 @@ struct QObjectInputVisitor
/* True to reject parse in visit_end_struct() if unvisited keys remain. */
bool strict;
GString *errname; /* Accumulator for full_name() */
};
static QObjectInputVisitor *to_qiv(Visitor *v)
@ -51,9 +53,42 @@ static QObjectInputVisitor *to_qiv(Visitor *v)
return container_of(v, QObjectInputVisitor, visitor);
}
static QObject *qobject_input_get_object(QObjectInputVisitor *qiv,
const char *name,
bool consume, Error **errp)
static const char *full_name(QObjectInputVisitor *qiv, const char *name)
{
StackObject *so;
char buf[32];
if (qiv->errname) {
g_string_truncate(qiv->errname, 0);
} else {
qiv->errname = g_string_new("");
}
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);
g_string_prepend(qiv->errname, buf);
}
name = so->name;
}
if (name) {
g_string_prepend(qiv->errname, name);
} else if (qiv->errname->str[0] == '.') {
g_string_erase(qiv->errname, 0, 1);
} else {
return "<anonymous>";
}
return qiv->errname->str;
}
static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv,
const char *name,
bool consume)
{
StackObject *tos;
QObject *qobj;
@ -77,9 +112,6 @@ static QObject *qobject_input_get_object(QObjectInputVisitor *qiv,
bool removed = g_hash_table_remove(tos->h, name);
assert(removed);
}
if (!ret) {
error_setg(errp, QERR_MISSING_PARAMETER, name);
}
} else {
assert(qobject_type(qobj) == QTYPE_QLIST);
assert(!name);
@ -87,12 +119,25 @@ static QObject *qobject_input_get_object(QObjectInputVisitor *qiv,
assert(ret);
if (consume) {
tos->entry = qlist_next(tos->entry);
tos->index++;
}
}
return ret;
}
static QObject *qobject_input_get_object(QObjectInputVisitor *qiv,
const char *name,
bool consume, Error **errp)
{
QObject *obj = qobject_input_try_get_object(qiv, name, consume);
if (!obj) {
error_setg(errp, QERR_MISSING_PARAMETER, full_name(qiv, name));
}
return obj;
}
static void qdict_add_key(const char *key, QObject *obj, void *opaque)
{
GHashTable *h = opaque;
@ -100,13 +145,14 @@ static void qdict_add_key(const char *key, QObject *obj, void *opaque)
}
static const QListEntry *qobject_input_push(QObjectInputVisitor *qiv,
const char *name,
QObject *obj, void *qapi)
{
GHashTable *h;
StackObject *tos = g_new0(StackObject, 1);
assert(obj);
tos->name = name;
tos->obj = obj;
tos->qapi = qapi;
@ -116,6 +162,7 @@ static const QListEntry *qobject_input_push(QObjectInputVisitor *qiv,
tos->h = h;
} else if (qobject_type(obj) == QTYPE_QLIST) {
tos->entry = qlist_first(qobject_to_qlist(obj));
tos->index = -1;
}
QSLIST_INSERT_HEAD(&qiv->stack, tos, node);
@ -142,7 +189,8 @@ static void qobject_input_check_struct(Visitor *v, Error **errp)
if (g_hash_table_size(top_ht)) {
const char *key;
g_hash_table_find(top_ht, always_true, (gpointer)&key);
error_setg(errp, "Parameter '%s' is unexpected", key);
error_setg(errp, "Parameter '%s' is unexpected",
full_name(qiv, key));
}
}
}
@ -180,12 +228,12 @@ static void qobject_input_start_struct(Visitor *v, const char *name, void **obj,
return;
}
if (qobject_type(qobj) != QTYPE_QDICT) {
error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"QDict");
error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
full_name(qiv, name), "object");
return;
}
qobject_input_push(qiv, qobj, obj);
qobject_input_push(qiv, name, qobj, obj);
if (obj) {
*obj = g_malloc0(size);
@ -206,12 +254,12 @@ static void qobject_input_start_list(Visitor *v, const char *name,
return;
}
if (qobject_type(qobj) != QTYPE_QLIST) {
error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"list");
error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
full_name(qiv, name), "array");
return;
}
entry = qobject_input_push(qiv, qobj, list);
entry = qobject_input_push(qiv, name, qobj, list);
if (entry && list) {
*list = g_malloc0(size);
}
@ -260,8 +308,8 @@ static void qobject_input_type_int64(Visitor *v, const char *name, int64_t *obj,
}
qint = qobject_to_qint(qobj);
if (!qint) {
error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"integer");
error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
full_name(qiv, name), "integer");
return;
}
@ -281,8 +329,8 @@ static void qobject_input_type_uint64(Visitor *v, const char *name, uint64_t *ob
}
qint = qobject_to_qint(qobj);
if (!qint) {
error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"integer");
error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
full_name(qiv, name), "integer");
return;
}
@ -301,8 +349,8 @@ static void qobject_input_type_bool(Visitor *v, const char *name, bool *obj,
}
qbool = qobject_to_qbool(qobj);
if (!qbool) {
error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"boolean");
error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
full_name(qiv, name), "boolean");
return;
}
@ -322,8 +370,8 @@ static void qobject_input_type_str(Visitor *v, const char *name, char **obj,
}
qstr = qobject_to_qstring(qobj);
if (!qstr) {
error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"string");
error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
full_name(qiv, name), "string");
return;
}
@ -353,8 +401,8 @@ static void qobject_input_type_number(Visitor *v, const char *name, double *obj,
return;
}
error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"number");
error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
full_name(qiv, name), "number");
}
static void qobject_input_type_any(Visitor *v, const char *name, QObject **obj,
@ -382,15 +430,15 @@ static void qobject_input_type_null(Visitor *v, const char *name, Error **errp)
}
if (qobject_type(qobj) != QTYPE_QNULL) {
error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"null");
error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
full_name(qiv, name), "null");
}
}
static void qobject_input_optional(Visitor *v, const char *name, bool *present)
{
QObjectInputVisitor *qiv = to_qiv(v);
QObject *qobj = qobject_input_get_object(qiv, name, false, NULL);
QObject *qobj = qobject_input_try_get_object(qiv, name, false);
if (!qobj) {
*present = false;
@ -403,6 +451,7 @@ static void qobject_input_optional(Visitor *v, const char *name, bool *present)
static void qobject_input_free(Visitor *v)
{
QObjectInputVisitor *qiv = to_qiv(v);
while (!QSLIST_EMPTY(&qiv->stack)) {
StackObject *tos = QSLIST_FIRST(&qiv->stack);
@ -411,6 +460,9 @@ static void qobject_input_free(Visitor *v)
}
qobject_decref(qiv->root);
if (qiv->errname) {
g_string_free(qiv->errname, TRUE);
}
g_free(qiv);
}