[Qemu-devel] [RFC 0/7] QError v1

[Qemu-devel] [RFC 0/7] QError v1

[Luiz Capitulino]

 Hi there,

 This is a new version of the QError framework.

 I've been through the last QError thread and tried to collect the best
suggestions. The important changes are:

- Dropped the .user_print function
- Added Paolo's suggestion on having a printf-like format for 'desc'
- Line number and filename are also stored in the QError object
- Uses QJSon

 Now, a call like:

monitor_printf(mon, "husb: host usb device %d.%d is already open\n",
                      bus_num, addr);

 Would become:

qemu_error_structed("{ 'bus_num': %d, 'addr': %d }", bus_num, addr);

 Its error table entry should be:

{
    .code = QERR_USB_EOPEN,
    .desc = "husb: host usb device %(bus_num)d.%(addr)d is already open",
}

 Finally, on the wire we'd have:

{ "error": { "code": 45,
             "data": { "bus_num": 1, "addr": 0xff } } }

 As the previous version, qemu_error_structed() builds on top of Gerd's
QemuErrorSink API.

 Besides introducing QError, this series also has simple usage examples for
qdev_device_add() and do_info_balloon().

 A last note: this series is on top of the (to be merged) QJSon module,
that's why it's a RFC and.. I didn't test it too much. :)

 Thanks.

[Qemu-devel] [PATCH 1/7] QJSon: Introduce qobject_from_json_va()

[Luiz Capitulino]

Simple wrapper to parse_json() that accepts a va_list, will be
used by QError.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qjson.c |    5 +++++
 qjson.h |    2 ++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qjson.c b/qjson.c
index 5f92996..02fcd83 100644
--- a/qjson.c
+++ b/qjson.c
@@ -723,3 +723,8 @@ QObject *qobject_from_jsonf(const char *string, size_t *length, ...)
 
     return obj;
 }
+
+QObject *qobject_from_json_va(const char *string, va_list *ap)
+{
+    return parse_json(string, NULL, ap);
+}
diff --git a/qjson.h b/qjson.h
index 0c94954..da0d653 100644
--- a/qjson.h
+++ b/qjson.h
@@ -14,10 +14,12 @@
 #ifndef QJSON_H
 #define QJSON_H
 
+#include <stdarg.h>
 #include "qobject.h"
 
 QObject *qobject_from_json(const char *string, size_t *length);
 QObject *qobject_from_jsonf(const char *string, size_t *length, ...)
     __attribute__((__format__ (__printf__, 1, 3)));
+QObject *qobject_from_json_va(const char *string, va_list *ap);
 
 #endif /* QJSON_H */
-- 
1.6.5.2.74.g610f9

[Qemu-devel] [PATCH 2/7] Introduce QError

[Luiz Capitulino]

QError is a high-level data type that can be used to store the
following error information:

o Error data: Any kind of data generated at error time can be stored
              (if turned into a QObject)
o Description: A string description, which may contain error data
o Error location: file name and line number of where the error was
                  triggered

Before using it functions usually have to register a new error type,
this is done by adding a new entry in the qerror_type[] table with
the following error information:

1. Code, which should be added to enum QErrorCode first
2. Description, which is a printf-like string

QError exports the following functions:

- qerror_new(): Create a new 'empty' QError
- qerror_from_va(): Create a new QError from the specified va_list
- qerror_print(): Print the specified QError

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 Makefile  |    2 +-
 qerror.c  |  240 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qerror.h  |   48 ++++++++++++
 qobject.h |    1 +
 4 files changed, 290 insertions(+), 1 deletions(-)
 create mode 100644 qerror.c
 create mode 100644 qerror.h

diff --git a/Makefile b/Makefile
index 813bd0a..325e583 100644
--- a/Makefile
+++ b/Makefile
@@ -125,7 +125,7 @@ obj-y += net.o net-queue.o
 obj-y += qemu-char.o aio.o net-checksum.o savevm.o
 obj-y += msmouse.o ps2.o
 obj-y += qdev.o qdev-properties.o
-obj-y += qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o qjson.o
+obj-y += qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o qjson.o qerror.o
 obj-y += qemu-config.o
 
 obj-$(CONFIG_BRLAPI) += baum.o
diff --git a/qerror.c b/qerror.c
new file mode 100644
index 0000000..0359d65
--- /dev/null
+++ b/qerror.c
@@ -0,0 +1,240 @@
+/*
+ * QError: QEMU Error data-type.
+ *
+ * Copyright (C) 2009 Red Hat Inc.
+ *
+ * Authors:
+ *  Luiz Capitulino <lcapitulino@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ */
+#include "qint.h"
+#include "qjson.h"
+#include "qerror.h"
+#include "qstring.h"
+#include "sysemu.h"
+#include "qemu-common.h"
+
+static void qerror_destroy_obj(QObject *obj);
+
+static const QType qerror_type = {
+    .code = QTYPE_QERROR,
+    .destroy = qerror_destroy_obj,
+};
+
+/**
+ * The 'desc' member is a printf-like string, the format of the format
+ * string is:
+ *
+ * %(KEY)TYPE
+ *
+ * Where KEY is a QDict key and TYPE is the type of its value, KEY and
+ * its value must be passed to qerror_from_info().
+ *
+ * Example:
+ *
+ * "foo error on device: %(device)s slot: %(slot_nr)s"
+ *
+ * A single percent sign can be printed if followed by a second one,
+ * for example:
+ *
+ * "running out of foo: %(foo)d%%"
+ *
+ * Valid types are:
+ *
+ * s    (string)
+ * d    (integer)
+ */
+static QErrorTable qerror_table[] = {
+    {
+        .code = QERR_UNKNOWN,
+        .desc = "unknown error",
+    },
+};
+
+/**
+ * qerror_new(): Create a new QError
+ *
+ * Return strong reference.
+ */
+QError *qerror_new(void)
+{
+    QError *qerr;
+
+    qerr = qemu_mallocz(sizeof(*qerr));
+    QOBJECT_INIT(qerr, &qerror_type);
+
+    return qerr;
+}
+
+/**
+ * qerror_from_info(): Create a new QError from error information
+ *
+ * The information consists of:
+ *
+ * - code: error code
+ * - file: the file of where the error happend
+ * - linenr: the line number of where the error happend
+ * - fmt: JSON printf-like format
+ * - va: va_list of all arguments
+ *
+ * Return strong reference.
+ */
+QError *qerror_from_info(QErrorCode code, const char *file, int linenr,
+                         const char *fmt, va_list *va)
+{
+    QError *qerr;
+
+    assert((code > 0) && (code < QERR_MAX));
+
+    qerr = qerror_new();
+    qerr->entry = &qerror_table[code];
+    qerr->file = file;
+    qerr->linenr = linenr;
+    if (fmt) {
+        qerr->data = qobject_from_json_va(fmt, va);
+        assert(qerr->data != NULL);
+    }
+
+    return qerr;
+}
+
+static char *get_substr(const char *start, const char *end)
+{
+    char *str;
+    size_t length;
+
+    length = end - start + 1;
+    str = qemu_malloc(length + 1);
+    memcpy(str, start, length);
+    str[length] = '\0';
+
+    return str;
+}
+
+static void qerror_abort(const QError *qerror)
+{
+    fprintf(stderr, " in '%s'\n", qerror->entry->desc);
+    abort();
+}
+
+#define ERROR_PREFIX "\n\nqerror: "
+
+static void type_error(const QError *qerror, int c)
+{
+    fprintf(stderr, ERROR_PREFIX "invalid type '%c'", c);
+    qerror_abort(qerror);
+}
+
+static void key_error(const QError *qerror, const char *key)
+{
+    fprintf(stderr, ERROR_PREFIX "key '%s' not found in QDict ", key);
+    fprintf(stderr, "check call at %s:%d\n", qerror->file, qerror->linenr);
+    abort();
+}
+
+static void parse_error(const QError *qerror, int c)
+{
+    fprintf(stderr, ERROR_PREFIX "expected '%c'", c);
+    qerror_abort(qerror);
+}
+
+static const char *print_field(const QError *qerror, const char *start)
+{
+    int type;
+    QDict *qdict;
+    char *name;
+    const char *end;
+
+    if (*start != '%')
+        parse_error(qerror, '%');
+    start++;
+    if (*start != '(')
+        parse_error(qerror, '(');
+    start++;
+
+    end = strchr(start, ')');
+    if (!end)
+        parse_error(qerror, ')');
+
+    name = get_substr(start, end - 1);
+    qdict = qobject_to_qdict(qerror->data);
+
+    if (!qdict_haskey(qdict, name)) {
+        key_error(qerror, name);
+    }
+
+    type = *++end;
+    switch (type) {
+        case 's':
+            qemu_error("%s", qdict_get_str(qdict, name));
+            break;
+        case 'd':
+            qemu_error("%" PRId64, qdict_get_int(qdict, name));
+            break;
+        default:
+            type_error(qerror, type);
+    }
+
+    qemu_free(name);
+    return ++end;
+}
+
+/**
+ * qerror_print(): Print QError data
+ *
+ * This function will print the member 'desc' of the specified QError object,
+ * it uses qemu_error() for this, so that the output is routed to the right
+ * place (ie. stderr ou Monitor's device).
+ */
+void qerror_print(const QError *qerror)
+{
+    const char *p;
+
+    for (p = qerror->entry->desc; *p != '\0'; p++) {
+        if (*p == '%') {
+            if (*(p + 1) == '%') {
+                p++;
+            } else {
+                p = print_field(qerror, p);
+                if (*p == '\0') {
+                    break;
+                }
+            }
+        }
+
+        /*
+         * FIXME: This is inefficient, qemu_error() likes strings
+         */
+        qemu_error("%c", *p);
+    }
+
+    qemu_error("\n");
+}
+
+/**
+ * qobject_to_qerror(): Convert a QObject into a QError
+ */
+QError *qobject_to_qerror(const QObject *obj)
+{
+    if (qobject_type(obj) != QTYPE_QERROR) {
+        return NULL;
+    }
+
+    return container_of(obj, QError, base);
+}
+
+/**
+ * qerror_destroy_obj(): Free all memory allocated by a QError
+ */
+static void qerror_destroy_obj(QObject *obj)
+{
+    QError *qerr;
+
+    assert(obj != NULL);
+    qerr = qobject_to_qerror(obj);
+
+    qobject_decref(qerr->data);
+    qemu_free(qerr);
+}
diff --git a/qerror.h b/qerror.h
new file mode 100644
index 0000000..2fd0d58
--- /dev/null
+++ b/qerror.h
@@ -0,0 +1,48 @@
+/*
+ * QError header file.
+ *
+ * Copyright (C) 2009 Red Hat Inc.
+ *
+ * Authors:
+ *  Luiz Capitulino <lcapitulino@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ */
+#ifndef QERROR_H
+#define QERROR_H
+
+#include <stdarg.h>
+#include "qobject.h"
+
+/*
+ * IMPORTANT: errors numbers must not change after they have been
+ * added here.
+ */
+typedef enum QErrorCode {
+    QERR_UNKNOWN,
+    QERR_MAX,
+} QErrorCode;
+
+struct QError;
+
+typedef struct QErrorTable {
+    QErrorCode code;
+    const char *desc;
+} QErrorTable;
+
+typedef struct QError {
+    QObject_HEAD;
+    int linenr;         /* error line number */
+    const char *file;   /* error file */
+    QObject *data;      /* error specific data */
+    const QErrorTable *entry;
+} QError;
+
+QError *qerror_new(void);
+QError *qerror_from_info(QErrorCode code, const char *file, int linenr,
+                         const char *fmt, va_list *va);
+void qerror_print(const QError *qerror);
+QError *qobject_to_qerror(const QObject *obj);
+
+#endif /* QERROR_H */
diff --git a/qobject.h b/qobject.h
index 167b607..838dddc 100644
--- a/qobject.h
+++ b/qobject.h
@@ -43,6 +43,7 @@ typedef enum {
     QTYPE_QLIST,
     QTYPE_QFLOAT,
     QTYPE_QBOOL,
+    QTYPE_QERROR,
 } qtype_code;
 
 struct QObject;
-- 
1.6.5.2.74.g610f9

[Qemu-devel] Re: [PATCH 2/7] Introduce QError

[Anthony Liguori]

> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  Makefile  |    2 +-
>  qerror.c  |  240 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qerror.h  |   48 ++++++++++++
>  qobject.h |    1 +
>  4 files changed, 290 insertions(+), 1 deletions(-)
>  create mode 100644 qerror.c
>  create mode 100644 qerror.h
>
> diff --git a/Makefile b/Makefile
> index 813bd0a..325e583 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -125,7 +125,7 @@ obj-y += net.o net-queue.o
>  obj-y += qemu-char.o aio.o net-checksum.o savevm.o
>  obj-y += msmouse.o ps2.o
>  obj-y += qdev.o qdev-properties.o
> -obj-y += qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o qjson.o
> +obj-y += qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o qjson.o qerror.o
>  obj-y += qemu-config.o
>
>  obj-$(CONFIG_BRLAPI) += baum.o
> diff --git a/qerror.c b/qerror.c
> new file mode 100644
> index 0000000..0359d65
> --- /dev/null
> +++ b/qerror.c
> @@ -0,0 +1,240 @@
> +/*
> + * QError: QEMU Error data-type.
> + *
> + * Copyright (C) 2009 Red Hat Inc.
> + *
> + * Authors:
> + *  Luiz Capitulino <lcapitulino@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + */
> +#include "qint.h"
> +#include "qjson.h"
> +#include "qerror.h"
> +#include "qstring.h"
> +#include "sysemu.h"
> +#include "qemu-common.h"
> +
> +static void qerror_destroy_obj(QObject *obj);
> +
> +static const QType qerror_type = {
> +    .code = QTYPE_QERROR,
> +    .destroy = qerror_destroy_obj,
> +};
> +
> +/**
> + * The 'desc' member is a printf-like string, the format of the format
> + * string is:
> + *
> + * %(KEY)TYPE
> + *
> + * Where KEY is a QDict key and TYPE is the type of its value, KEY and
> + * its value must be passed to qerror_from_info().
> + *
> + * Example:
> + *
> + * "foo error on device: %(device)s slot: %(slot_nr)s"
> + *
> + * A single percent sign can be printed if followed by a second one,
> + * for example:
> + *
> + * "running out of foo: %(foo)d%%"
> + *
> + * Valid types are:
> + *
> + * s    (string)
> + * d    (integer)
> + */
> +static QErrorTable qerror_table[] = {
> +    {
> +        .code = QERR_UNKNOWN,
> +        .desc = "unknown error",
> +    },
> +};
> +
> +/**
> + * qerror_new(): Create a new QError
> + *
> + * Return strong reference.
> + */
> +QError *qerror_new(void)
> +{
> +    QError *qerr;
> +
> +    qerr = qemu_mallocz(sizeof(*qerr));
> +    QOBJECT_INIT(qerr, &qerror_type);
> +
> +    return qerr;
> +}
> +
> +/**
> + * qerror_from_info(): Create a new QError from error information
> + *
> + * The information consists of:
> + *
> + * - code: error code
> + * - file: the file of where the error happend
> + * - linenr: the line number of where the error happend
> + * - fmt: JSON printf-like format
> + * - va: va_list of all arguments
> + *
> + * Return strong reference.
> + */
> +QError *qerror_from_info(QErrorCode code, const char *file, int linenr,
> +                         const char *fmt, va_list *va)
> +{
>   

va_list doesn't need to be a pointer.

Shouldn't this take a ... too?

> diff --git a/qerror.h b/qerror.h
> new file mode 100644
> index 0000000..2fd0d58
> --- /dev/null
> +++ b/qerror.h
> @@ -0,0 +1,48 @@
> +/*
> + * QError header file.
> + *
> + * Copyright (C) 2009 Red Hat Inc.
> + *
> + * Authors:
> + *  Luiz Capitulino <lcapitulino@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + */
> +#ifndef QERROR_H
> +#define QERROR_H
> +
> +#include <stdarg.h>
> +#include "qobject.h"
> +
> +/*
> + * IMPORTANT: errors numbers must not change after they have been
> + * added here.
> + */
> +typedef enum QErrorCode {
> +    QERR_UNKNOWN,
> +    QERR_MAX,
> +} QErrorCode;
> +
> +struct QError;
> +
> +typedef struct QErrorTable {
> +    QErrorCode code;
> +    const char *desc;
> +} QErrorTable;
> +
> +typedef struct QError {
> +    QObject_HEAD;
> +    int linenr;         /* error line number */
> +    const char *file;   /* error file */
> +    QObject *data;      /* error specific data */
> +    const QErrorTable *entry;
> +} QError;
> +
> +QError *qerror_new(void);
> +QError *qerror_from_info(QErrorCode code, const char *file, int linenr,
> +                         const char *fmt, va_list *va);
>   

I don't know how I feel about the linenr/file bit.  It seems prone to 
misused when used in a public interface (vs. automatically via a macro).

-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 2/7] Introduce QError

[Luiz Capitulino]

On Thu, 29 Oct 2009 15:14:12 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  Makefile  |    2 +-
> >  qerror.c  |  240 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  qerror.h  |   48 ++++++++++++
> >  qobject.h |    1 +
> >  4 files changed, 290 insertions(+), 1 deletions(-)
> >  create mode 100644 qerror.c
> >  create mode 100644 qerror.h
> >
> > diff --git a/Makefile b/Makefile
> > index 813bd0a..325e583 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -125,7 +125,7 @@ obj-y += net.o net-queue.o
> >  obj-y += qemu-char.o aio.o net-checksum.o savevm.o
> >  obj-y += msmouse.o ps2.o
> >  obj-y += qdev.o qdev-properties.o
> > -obj-y += qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o qjson.o
> > +obj-y += qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o qjson.o qerror.o
> >  obj-y += qemu-config.o
> >
> >  obj-$(CONFIG_BRLAPI) += baum.o
> > diff --git a/qerror.c b/qerror.c
> > new file mode 100644
> > index 0000000..0359d65
> > --- /dev/null
> > +++ b/qerror.c
> > @@ -0,0 +1,240 @@
> > +/*
> > + * QError: QEMU Error data-type.
> > + *
> > + * Copyright (C) 2009 Red Hat Inc.
> > + *
> > + * Authors:
> > + *  Luiz Capitulino <lcapitulino@redhat.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2.  See
> > + * the COPYING file in the top-level directory.
> > + */
> > +#include "qint.h"
> > +#include "qjson.h"
> > +#include "qerror.h"
> > +#include "qstring.h"
> > +#include "sysemu.h"
> > +#include "qemu-common.h"
> > +
> > +static void qerror_destroy_obj(QObject *obj);
> > +
> > +static const QType qerror_type = {
> > +    .code = QTYPE_QERROR,
> > +    .destroy = qerror_destroy_obj,
> > +};
> > +
> > +/**
> > + * The 'desc' member is a printf-like string, the format of the format
> > + * string is:
> > + *
> > + * %(KEY)TYPE
> > + *
> > + * Where KEY is a QDict key and TYPE is the type of its value, KEY and
> > + * its value must be passed to qerror_from_info().
> > + *
> > + * Example:
> > + *
> > + * "foo error on device: %(device)s slot: %(slot_nr)s"
> > + *
> > + * A single percent sign can be printed if followed by a second one,
> > + * for example:
> > + *
> > + * "running out of foo: %(foo)d%%"
> > + *
> > + * Valid types are:
> > + *
> > + * s    (string)
> > + * d    (integer)
> > + */
> > +static QErrorTable qerror_table[] = {
> > +    {
> > +        .code = QERR_UNKNOWN,
> > +        .desc = "unknown error",
> > +    },
> > +};
> > +
> > +/**
> > + * qerror_new(): Create a new QError
> > + *
> > + * Return strong reference.
> > + */
> > +QError *qerror_new(void)
> > +{
> > +    QError *qerr;
> > +
> > +    qerr = qemu_mallocz(sizeof(*qerr));
> > +    QOBJECT_INIT(qerr, &qerror_type);
> > +
> > +    return qerr;
> > +}
> > +
> > +/**
> > + * qerror_from_info(): Create a new QError from error information
> > + *
> > + * The information consists of:
> > + *
> > + * - code: error code
> > + * - file: the file of where the error happend
> > + * - linenr: the line number of where the error happend
> > + * - fmt: JSON printf-like format
> > + * - va: va_list of all arguments
> > + *
> > + * Return strong reference.
> > + */
> > +QError *qerror_from_info(QErrorCode code, const char *file, int linenr,
> > +                         const char *fmt, va_list *va)
> > +{
> >   
> 
> va_list doesn't need to be a pointer.

 Ok, but if this is going to be a public interface, I think it's
better to va_copy() before passing it to qobject_from_json_va() then.

> Shouldn't this take a ... too?

 The problem is that its caller (qemu_error_structed()) also takes
a ..., I don't know how to pass the arguments to a second function with
variadic arguments w/o passing the va_list.

> > diff --git a/qerror.h b/qerror.h
> > new file mode 100644
> > index 0000000..2fd0d58
> > --- /dev/null
> > +++ b/qerror.h
> > @@ -0,0 +1,48 @@
> > +/*
> > + * QError header file.
> > + *
> > + * Copyright (C) 2009 Red Hat Inc.
> > + *
> > + * Authors:
> > + *  Luiz Capitulino <lcapitulino@redhat.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2.  See
> > + * the COPYING file in the top-level directory.
> > + */
> > +#ifndef QERROR_H
> > +#define QERROR_H
> > +
> > +#include <stdarg.h>
> > +#include "qobject.h"
> > +
> > +/*
> > + * IMPORTANT: errors numbers must not change after they have been
> > + * added here.
> > + */
> > +typedef enum QErrorCode {
> > +    QERR_UNKNOWN,
> > +    QERR_MAX,
> > +} QErrorCode;
> > +
> > +struct QError;
> > +
> > +typedef struct QErrorTable {
> > +    QErrorCode code;
> > +    const char *desc;
> > +} QErrorTable;
> > +
> > +typedef struct QError {
> > +    QObject_HEAD;
> > +    int linenr;         /* error line number */
> > +    const char *file;   /* error file */
> > +    QObject *data;      /* error specific data */
> > +    const QErrorTable *entry;
> > +} QError;
> > +
> > +QError *qerror_new(void);
> > +QError *qerror_from_info(QErrorCode code, const char *file, int linenr,
> > +                         const char *fmt, va_list *va);
> >   
> 
> I don't know how I feel about the linenr/file bit.  It seems prone to 
> misused when used in a public interface (vs. automatically via a macro).

 This is what happens in practice, qemu_error_structed() is a macro,
it's where the __LINE__/__FILE__ macros are actually used.

 Although there is no problems in being part of a public interface,
qerror_from_info() is a low-level function, it's supposed to be
part of a higher level API.

 The only way to make this the way you suggest, would be to
export qemu_error_structed() from QError someway. But I'd have to
think how to do this, as we have to deal with the Monitor's bits
and the QemuErrorSink API.

 By the way, what contributes to make this worse is the
all-in-a-file Monitor module.. We also have cyclic dependencies
there. I plan to split it, but probably not for 0.12.

[Qemu-devel] Re: [PATCH 2/7] Introduce QError

[Paolo Bonzini]

On 10/29/2009 09:48 PM, Luiz Capitulino wrote:
> >  va_list doesn't need to be a pointer.
>
>   Ok, but if this is going to be a public interface, I think it's
> better to va_copy() before passing it to qobject_from_json_va() then.

It is standard to pass a va_list by value without doing va_copy in the 
caller.

>> >  Shouldn't this take a ... too?
>   The problem is that its caller (qemu_error_structed()) also takes
> a ..., I don't know how to pass the arguments to a second function with
> variadic arguments w/o passing the va_list.

Yeah, this is fine.

Paolo

Re: [Qemu-devel] Re: [PATCH 2/7] Introduce QError

[Jamie Lokier]

Paolo Bonzini wrote:
> On 10/29/2009 09:48 PM, Luiz Capitulino wrote:
> >>  va_list doesn't need to be a pointer.
> >
> >  Ok, but if this is going to be a public interface, I think it's
> >better to va_copy() before passing it to qobject_from_json_va() then.
> 
> It is standard to pass a va_list by value without doing va_copy in the 
> caller.

That's right, just like:

    va_list ap;
    va_start(ap, format);
    vfprintf(stderr, format, ap);   /* <- passed by value */
    va_end(ap);

You only need va_copy() is you're going to use some more it after
passing it to the function.

There should be one va_end() for every va_start() plus every va_copy().

-- Jamie

[Qemu-devel] Re: [PATCH 2/7] Introduce QError

[Anthony Liguori]

Paolo Bonzini wrote:
> On 10/29/2009 09:48 PM, Luiz Capitulino wrote:
>> >  va_list doesn't need to be a pointer.
>>
>>   Ok, but if this is going to be a public interface, I think it's
>> better to va_copy() before passing it to qobject_from_json_va() then.
>
> It is standard to pass a va_list by value without doing va_copy in the 
> caller.
>
>>> >  Shouldn't this take a ... too?
>>   The problem is that its caller (qemu_error_structed()) also takes
>> a ..., I don't know how to pass the arguments to a second function with
>> variadic arguments w/o passing the va_list.
>
> Yeah, this is fine.

Usually you provide two versions of variadic functions.  One that takes 
an ... and one that takes a va_list.

-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 2/7] Introduce QError

[Luiz Capitulino]

On Fri, 30 Oct 2009 08:05:57 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> Paolo Bonzini wrote:
> > On 10/29/2009 09:48 PM, Luiz Capitulino wrote:
> >> >  va_list doesn't need to be a pointer.
> >>
> >>   Ok, but if this is going to be a public interface, I think it's
> >> better to va_copy() before passing it to qobject_from_json_va() then.
> >
> > It is standard to pass a va_list by value without doing va_copy in the 
> > caller.
> >
> >>> >  Shouldn't this take a ... too?
> >>   The problem is that its caller (qemu_error_structed()) also takes
> >> a ..., I don't know how to pass the arguments to a second function with
> >> variadic arguments w/o passing the va_list.
> >
> > Yeah, this is fine.
> 
> Usually you provide two versions of variadic functions.  One that takes 
> an ... and one that takes a va_list.

 This is what is going here, qemu_error_structed() take a ... and
qobject_from_json_va() takes a va_list.

[Qemu-devel] [PATCH 3/7] monitor: QError support

[Luiz Capitulino]

This commit paves the way for QError support in the Monitor,
it adds a QError member to the Monitor struct and functions
to check and print it.

Additionally, it introduces qemu_error_structed() which should
be used by monitor handlers which report errors.

This new function has to be used in place of qemu_error(), which
will become private when all the conversion is done.

Basically, the Monitor's error flow is something like this:

1. An error happens in the handler, it calls qemu_error_structed()
2. qemu_error_structed() builds a new QError object and stores it in
   the Monitor struct
3. The handler returns
4. Top level Monitor code checks the Monitor struct and calls
   qerror_print() to print the error

When in protocol mode, step 4 will use the specified QError to
build a protocol error response, intead of calling qerror_print().

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |   43 ++++++++++++++++++++++++++++++++++++++++++-
 sysemu.h  |    7 +++++++
 2 files changed, 49 insertions(+), 1 deletions(-)

diff --git a/monitor.c b/monitor.c
index 109ff5c..4d56b8e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -49,6 +49,7 @@
 #include "qlist.h"
 #include "qdict.h"
 #include "qstring.h"
+#include "qerror.h"
 
 //#define DEBUG
 //#define DEBUG_COMPLETION
@@ -103,6 +104,7 @@ struct Monitor {
     CPUState *mon_cpu;
     BlockDriverCompletionFunc *password_completion_cb;
     void *password_opaque;
+    QError *error;
     QLIST_HEAD(,mon_fd_t) fds;
     QLIST_ENTRY(Monitor) entry;
 };
@@ -3146,6 +3148,18 @@ fail:
     return NULL;
 }
 
+static inline int monitor_has_error(const Monitor *mon)
+{
+    return mon->error != NULL;
+}
+
+static void monitor_print_error(Monitor *mon)
+{
+    qerror_print(mon->error);
+    QDECREF(mon->error);
+    mon->error = NULL;
+}
+
 static void monitor_handle_command(Monitor *mon, const char *cmdline)
 {
     QDict *qdict;
@@ -3171,7 +3185,10 @@ static void monitor_handle_command(Monitor *mon, const char *cmdline)
         cmd->mhandler.cmd(mon, qdict);
     }
 
-   qemu_errors_to_previous();
+    if (monitor_has_error(mon))
+        monitor_print_error(mon);
+
+    qemu_errors_to_previous();
 
 out:
     QDECREF(qdict);
@@ -3622,3 +3639,27 @@ void qemu_error(const char *fmt, ...)
         break;
     }
 }
+
+void qemu_error_qerror(QErrorCode code, const char *file, int linenr,
+                       const char *fmt, ...)
+{
+    va_list va;
+    QError *qerror;
+
+    assert(qemu_error_sink != NULL);
+
+    va_start(va, fmt);
+    qerror = qerror_from_info(code, file, linenr, fmt, &va);
+    va_end(va);
+
+    switch (qemu_error_sink->dest) {
+    case ERR_SINK_FILE:
+        qerror_print(qerror);
+        QDECREF(qerror);
+        break;
+    case ERR_SINK_MONITOR:
+        assert(qemu_error_sink->mon->error == NULL);
+        qemu_error_sink->mon->error = qerror;
+        break;
+    }
+}
diff --git a/sysemu.h b/sysemu.h
index 17af024..440a47c 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -7,6 +7,7 @@
 #include "qemu-queue.h"
 #include "qemu-timer.h"
 #include "qdict.h"
+#include "qerror.h"
 
 #ifdef _WIN32
 #include <windows.h>
@@ -71,6 +72,12 @@ void qemu_errors_to_file(FILE *fp);
 void qemu_errors_to_mon(Monitor *mon);
 void qemu_errors_to_previous(void);
 void qemu_error(const char *fmt, ...) __attribute__ ((format(printf, 1, 2)));
+void qemu_error_qerror(QErrorCode code, const char *file, int linenr,
+                       const char *fmt, ...)
+                      __attribute__ ((format(printf, 4, 5)));
+
+#define qemu_error_structed(code, fmt, ...) \
+    qemu_error_qerror(code, __FILE__, __LINE__, fmt, ## __VA_ARGS__)
 
 #ifdef _WIN32
 /* Polling handling */
-- 
1.6.5.2.74.g610f9

[Qemu-devel] [PATCH 4/7] QError: Add QERR_DEV_NFOUND

[Luiz Capitulino]

A generic error to be triggered when a device is not found.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    1 +
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 0359d65..69eabfb 100644
--- a/qerror.c
+++ b/qerror.c
@@ -51,6 +51,10 @@ static QErrorTable qerror_table[] = {
         .code = QERR_UNKNOWN,
         .desc = "unknown error",
     },
+    {
+        .code = QERR_DEV_NFOUND,
+        .desc = "device \"%(name)s\" not found",
+    },
 };
 
 /**
diff --git a/qerror.h b/qerror.h
index 2fd0d58..7f45360 100644
--- a/qerror.h
+++ b/qerror.h
@@ -21,6 +21,7 @@
  */
 typedef enum QErrorCode {
     QERR_UNKNOWN,
+    QERR_DEV_NFOUND,
     QERR_MAX,
 } QErrorCode;
 
-- 
1.6.5.2.74.g610f9

[Qemu-devel] [PATCH 5/7] qdev: Use QError for not found error

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/qdev.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 373ddfc..1cb9c35 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -29,6 +29,7 @@
 #include "qdev.h"
 #include "sysemu.h"
 #include "monitor.h"
+#include "qerror.h"
 
 static int qdev_hotplug = 0;
 
@@ -176,8 +177,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     /* find driver */
     info = qdev_find_info(NULL, driver);
     if (!info) {
-        qemu_error("Device \"%s\" not found.  Try -device '?' for a list.\n",
-                   driver);
+        qemu_error_structed(QERR_DEV_NFOUND, "{ 'name': %s }", driver);
         return NULL;
     }
     if (info->no_user) {
-- 
1.6.5.2.74.g610f9

[Qemu-devel] [PATCH 6/7] QError: Add do_info_balloon() errors

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qerror.c |    8 ++++++++
 qerror.h |    2 ++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 69eabfb..ad8d4e0 100644
--- a/qerror.c
+++ b/qerror.c
@@ -55,6 +55,14 @@ static QErrorTable qerror_table[] = {
         .code = QERR_DEV_NFOUND,
         .desc = "device \"%(name)s\" not found",
     },
+    {
+        .code = QERR_BAL_MMU,
+        .desc = "Using KVM without synchronous MMU, ballooning disabled",
+    },
+    {
+        .code = QERR_BAL_DIS,
+        .desc = "Ballooning not activated in VM",
+    },
 };
 
 /**
diff --git a/qerror.h b/qerror.h
index 7f45360..6a4c8a9 100644
--- a/qerror.h
+++ b/qerror.h
@@ -22,6 +22,8 @@
 typedef enum QErrorCode {
     QERR_UNKNOWN,
     QERR_DEV_NFOUND,
+    QERR_BAL_MMU,
+    QERR_BAL_DIS,
     QERR_MAX,
 } QErrorCode;
 
-- 
1.6.5.2.74.g610f9

[Qemu-devel] [PATCH 7/7] monitor: do_info_balloon(): use QError

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index 4d56b8e..4cec447 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1717,10 +1717,9 @@ static void do_info_balloon(Monitor *mon, QObject **ret_data)
 
     actual = qemu_balloon_status();
     if (kvm_enabled() && !kvm_has_sync_mmu())
-        monitor_printf(mon, "Using KVM without synchronous MMU, "
-                       "ballooning disabled\n");
+        qemu_error_structed(QERR_BAL_MMU, NULL);
     else if (actual == 0)
-        monitor_printf(mon, "Ballooning not activated in VM\n");
+        qemu_error_structed(QERR_BAL_DIS, NULL);
     else
         *ret_data = QOBJECT(qint_from_int((int)(actual >> 20)));
 }
-- 
1.6.5.2.74.g610f9

[Qemu-devel] Re: [RFC 0/7] QError v1

[Paolo Bonzini]

>   A last note: this series is on top of the (to be merged) QJSon module,
> that's why it's a RFC and.. I didn't test it too much. :)

I have just two comments:

1) you do

> -        qemu_error("Device \"%s\" not found.  Try -device '?' for a list.\n",
> -                   driver);
> +        qemu_error_structed(QERR_DEV_NFOUND, "{ 'name': %s }", driver);

why not store the "{ 'name': %s }" in the qerror_table?  I guess you 
plan to have different fields in some cases?

2) as I understood it, the consensus was to store the expanded error 
message (i.e. qerror_print) in the JSON output as well.  That would 
involve returning a QString from qerror_print, I guess.

Paolo

[Qemu-devel] Re: [RFC 0/7] QError v1

[Luiz Capitulino]

On Thu, 29 Oct 2009 23:12:10 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> >   A last note: this series is on top of the (to be merged) QJSon module,
> > that's why it's a RFC and.. I didn't test it too much. :)
> 
> I have just two comments:
> 
> 1) you do
> 
> > -        qemu_error("Device \"%s\" not found.  Try -device '?' for a list.\n",
> > -                   driver);
> > +        qemu_error_structed(QERR_DEV_NFOUND, "{ 'name': %s }", driver);
> 
> why not store the "{ 'name': %s }" in the qerror_table?  I guess you 
> plan to have different fields in some cases?

 The main reason is to have syntax checking, we can declare it in a
macro though, in case of generic errors which are going to be used in
other places.

> 2) as I understood it, the consensus was to store the expanded error 
> message (i.e. qerror_print) in the JSON output as well.  That would 
> involve returning a QString from qerror_print, I guess.

 Really? I thought the consensus was to send only an error code
and error data.

 I'm ok with either way, although I don't think a user targeted string
is going to be that useful.

[Qemu-devel] Re: [RFC 0/7] QError v1

[Gerd Hoffmann]

On 10/30/09 13:28, Luiz Capitulino wrote:
>>> -        qemu_error("Device \"%s\" not found.  Try -device '?' for a list.\n",
>>> -                   driver);
>>> +        qemu_error_structed(QERR_DEV_NFOUND, "{ 'name': %s }", driver);
>>
>> why not store the "{ 'name': %s }" in the qerror_table?  I guess you
>> plan to have different fields in some cases?
>
>   The main reason is to have syntax checking, we can declare it in a
> macro though, in case of generic errors which are going to be used in
> other places.

I still feel the error reporting is too complex.  IMHO there should be 
no need to edit two places for error reporting, which means I'd go the 
opposite direction:  Zap qerror_table[], then have:

qemu_error_structed(QERR_DEV_NFOUND, "device %{name}s not found",
                     "{ 'name': %s }", driver);

Also I think the error codes should be more generic, so you don't need a 
new one for each and every error.  Ideally we'll have a reasonable and 
stable set of error codes after the initial conversion, so you don't 
have to touch the management apps just to add new codes as qemu 
envolves.  The error code must help the management app to decide how to 
deal with the error, but it shouldn't carry details not needed for that.

Picking the balloon errors (other patch in this thread):  You have *two* 
error codes for ballooning not being available.  I think a generic 
"service not available" error code would work for both (and for other 
error cases too) and would be good enougth.  The management app will 
figure it can't balloon down the VM.  It will not know the reason from 
the error code, but does it have to?  I doubt it will react in a 
different way.  And for manual trouble-shooting the text message which 
carries more information gets logged.

cheers,
   Gerd

[Qemu-devel] Re: [RFC 0/7] QError v1

[Anthony Liguori]

Gerd Hoffmann wrote:
> On 10/30/09 13:28, Luiz Capitulino wrote:
>>>> -        qemu_error("Device \"%s\" not found.  Try -device '?' for 
>>>> a list.\n",
>>>> -                   driver);
>>>> +        qemu_error_structed(QERR_DEV_NFOUND, "{ 'name': %s }", 
>>>> driver);
>>>
>>> why not store the "{ 'name': %s }" in the qerror_table?  I guess you
>>> plan to have different fields in some cases?
>>
>>   The main reason is to have syntax checking, we can declare it in a
>> macro though, in case of generic errors which are going to be used in
>> other places.
>
> I still feel the error reporting is too complex.  IMHO there should be 
> no need to edit two places for error reporting, which means I'd go the 
> opposite direction:  Zap qerror_table[], then have:
>
> qemu_error_structed(QERR_DEV_NFOUND, "device %{name}s not found",
>                     "{ 'name': %s }", driver);
>
> Also I think the error codes should be more generic, so you don't need 
> a new one for each and every error.  Ideally we'll have a reasonable 
> and stable set of error codes after the initial conversion, so you 
> don't have to touch the management apps just to add new codes as qemu 
> envolves.  The error code must help the management app to decide how 
> to deal with the error, but it shouldn't carry details not needed for 
> that.

Okay, let's get more clever then and do:

#define QERR_DEV_NFOUND "{ 'code': 404, 'name': %s}"

So we can do:

qemu_error_structured(QERR_DEV_NFOUND, driver);

Such that we still get printf style parameter checking.

>
> Picking the balloon errors (other patch in this thread):  You have 
> *two* error codes for ballooning not being available.  I think a 
> generic "service not available" error code would work for both (and 
> for other error cases too) and would be good enougth.  The management 
> app will figure it can't balloon down the VM.  It will not know the 
> reason from the error code, but does it have to?  I doubt it will 
> react in a different way.  And for manual trouble-shooting the text 
> message which carries more information gets logged.

I think the trouble is Luiz is trying to preserve today's error 
messages.  Honestly, if we need to break those, I don't mind so much 
because I really doubt anyone is depending on the exact text of the 
error messages.

I agree that a bit more generic error messages wouldn't be a bad thing.

-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [RFC 0/7] QError v1

[Paolo Bonzini]

On 10/30/2009 02:09 PM, Anthony Liguori wrote:
>
> Okay, let's get more clever then and do:
>
> #define QERR_DEV_NFOUND "{ 'code': 404, 'name': %s}"
>
> So we can do:
>
> qemu_error_structured(QERR_DEV_NFOUND, driver);
>
> Such that we still get printf style parameter checking.

That's clever indeed. :-)  But no more clever than other things in qemu, 
I think I like it. :-)

Paolo

[Qemu-devel] Re: [RFC 0/7] QError v1

[Luiz Capitulino]

 (Added Daniel Berrange to the CC list)

On Fri, 30 Oct 2009 08:09:02 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> Gerd Hoffmann wrote:
> > On 10/30/09 13:28, Luiz Capitulino wrote:
> >>>> -        qemu_error("Device \"%s\" not found.  Try -device '?' for 
> >>>> a list.\n",
> >>>> -                   driver);
> >>>> +        qemu_error_structed(QERR_DEV_NFOUND, "{ 'name': %s }", 
> >>>> driver);
> >>>
> >>> why not store the "{ 'name': %s }" in the qerror_table?  I guess you
> >>> plan to have different fields in some cases?
> >>
> >>   The main reason is to have syntax checking, we can declare it in a
> >> macro though, in case of generic errors which are going to be used in
> >> other places.
> >
> > I still feel the error reporting is too complex.  IMHO there should be 
> > no need to edit two places for error reporting, which means I'd go the 
> > opposite direction:  Zap qerror_table[], then have:
> >
> > qemu_error_structed(QERR_DEV_NFOUND, "device %{name}s not found",
> >                     "{ 'name': %s }", driver);

 I'm ok with this.

> > Also I think the error codes should be more generic, so you don't need 
> > a new one for each and every error.  Ideally we'll have a reasonable 
> > and stable set of error codes after the initial conversion, so you 
> > don't have to touch the management apps just to add new codes as qemu 
> > envolves.  The error code must help the management app to decide how 
> > to deal with the error, but it shouldn't carry details not needed for 
> > that.
> 
> Okay, let's get more clever then and do:
> 
> #define QERR_DEV_NFOUND "{ 'code': 404, 'name': %s}"
> 
> So we can do:
> 
> qemu_error_structured(QERR_DEV_NFOUND, driver);
> 
> Such that we still get printf style parameter checking.

 Seems ok to me, but to make it harder to reuse existing error
codes we'd have to:

1. Make mandatory the use of a macro in the qemu_error_structed()
   call
2. All macros would have to be defined in qerror.h

 Btw, why are you calling it qemu_error_structured()? It's a long
name, let's call it qemu_erro_structed() or any better name.. I
thought about qemu_error_throw()..

> > Picking the balloon errors (other patch in this thread):  You have 
> > *two* error codes for ballooning not being available.  I think a 
> > generic "service not available" error code would work for both (and 
> > for other error cases too) and would be good enougth.  The management 
> > app will figure it can't balloon down the VM.  It will not know the 
> > reason from the error code, but does it have to?  I doubt it will 
> > react in a different way.  And for manual trouble-shooting the text 
> > message which carries more information gets logged.
> 
> I think the trouble is Luiz is trying to preserve today's error 
> messages.  Honestly, if we need to break those, I don't mind so much 
> because I really doubt anyone is depending on the exact text of the 
> error messages.
> 
> I agree that a bit more generic error messages wouldn't be a bad thing.

 I think we're back to the discussion regarding what information
an error should contain.

 Daniel seems to want as most info as possible:

http://lists.gnu.org/archive/html/qemu-devel/2009-10/msg01640.html

 In case we put only the error code and error data on the wire,
I think error codes should be *unique* and not generic. This way
clients are able to determine the exact error cause.

 If we go with generic errors, then we would have to send the
user string along. This introduces the problem of localization
and can be a bit redundant, as the call above would be emitted
like:

{ "error": { "code": 404,
             "desc": "driver foobar not found",
             "data": { "name": foobar" } } }

[Qemu-devel] Re: [RFC 0/7] QError v1

[Anthony Liguori]

Luiz Capitulino wrote:
>  Seems ok to me, but to make it harder to reuse existing error
> codes we'd have to:
>
> 1. Make mandatory the use of a macro in the qemu_error_structed()
>    call
> 2. All macros would have to be defined in qerror.h
>   

Not really.  All you need is to switch code to something that's more 
easily definable like a string.  So...

#define QERR_DEV_NFOUND "{ 'code': 'DevNotFound', 'name': %s}"

Now you can define this anywhere.

>  Btw, why are you calling it qemu_error_structured()? It's a long
> name, let's call it qemu_erro_structed() or any better name.. I
> thought about qemu_error_throw()..
>   

How about qemu_error_new().  It returns a QObject and I assume we'll 
just return an error object instead of returning a normal result, correct?

>> I agree that a bit more generic error messages wouldn't be a bad thing.
>>     
>
>  I think we're back to the discussion regarding what information
> an error should contain.
>
>  Daniel seems to want as most info as possible:
>
> http://lists.gnu.org/archive/html/qemu-devel/2009-10/msg01640.html
>
>  In case we put only the error code and error data on the wire,
> I think error codes should be *unique* and not generic. This way
> clients are able to determine the exact error cause.
>
>  If we go with generic errors, then we would have to send the
> user string along. This introduces the problem of localization
> and can be a bit redundant, as the call above would be emitted
> like:
>
> { "error": { "code": 404,
>              "desc": "driver foobar not found",
>              "data": { "name": foobar" } } }
>   

I think this is a problem that we don't have to solve yet.  Let's get 
the error format down first and then we can consider how granular the 
errors ought to be.

Changing the types of errors thrown is easy to do without breaking 
compatibility.

-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [RFC 0/7] QError v1

[Luiz Capitulino]

On Fri, 30 Oct 2009 08:59:43 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> Luiz Capitulino wrote:
> >  Seems ok to me, but to make it harder to reuse existing error
> > codes we'd have to:
> >
> > 1. Make mandatory the use of a macro in the qemu_error_structed()
> >    call
> > 2. All macros would have to be defined in qerror.h
> >   
> 
> Not really.  All you need is to switch code to something that's more 
> easily definable like a string.  So...
> 
> #define QERR_DEV_NFOUND "{ 'code': 'DevNotFound', 'name': %s}"
> 
> Now you can define this anywhere.

 In this approach error codes can be freely defined/created by
commands, right? No problem in having generic errors in qerror.h,
do you agree?

 This specific example is generic, IMO.

 Flexible, and easier to use. I'm just wondering if it's a good idea
to have them "spread" over QEMU, as opposite to having then centralized
in qerror.h.

> >  Btw, why are you calling it qemu_error_structured()? It's a long
> > name, let's call it qemu_erro_structed() or any better name.. I
> > thought about qemu_error_throw()..
> >   
> 
> How about qemu_error_new().  It returns a QObject and I assume we'll 
> just return an error object instead of returning a normal result, correct?

 Yes, but it doesn't return the error object as a return value.

 If we're in the monitor, the error object is stored in the Monitor
struct, which will be later checked when the handler returns. If we
were called from the command-line, the error object is printed right away.

 That's way I think qemu_error_throw() makes more sense.

> >> I agree that a bit more generic error messages wouldn't be a bad thing.
> >>     
> >
> >  I think we're back to the discussion regarding what information
> > an error should contain.
> >
> >  Daniel seems to want as most info as possible:
> >
> > http://lists.gnu.org/archive/html/qemu-devel/2009-10/msg01640.html
> >
> >  In case we put only the error code and error data on the wire,
> > I think error codes should be *unique* and not generic. This way
> > clients are able to determine the exact error cause.
> >
> >  If we go with generic errors, then we would have to send the
> > user string along. This introduces the problem of localization
> > and can be a bit redundant, as the call above would be emitted
> > like:
> >
> > { "error": { "code": 404,
> >              "desc": "driver foobar not found",
> >              "data": { "name": foobar" } } }
> >   
> 
> I think this is a problem that we don't have to solve yet.  Let's get 
> the error format down first and then we can consider how granular the 
> errors ought to be.

 I'm trying to avoid making decisions now, and discovering too late
they weren't good enough.

Re: [Qemu-devel] Re: [RFC 0/7] QError v1

[Jamie Lokier]

Anthony Liguori wrote:
> Okay, let's get more clever then and do:
> 
> #define QERR_DEV_NFOUND "{ 'code': 404, 'name': %s}"

By the way, since you've already invented a non-standard JSON
extension, which is the single quotes, why not go a step further and
permit the quotes to be omitted for simple tokens?

#define QERR_DEV_NFOUND "{ code: 404, name: %s}"

Much neater, IMHO.

-- Jamie

Re: [Qemu-devel] Re: [RFC 0/7] QError v1

[Paolo Bonzini]

On 10/30/2009 05:28 PM, Jamie Lokier wrote:
> By the way, since you've already invented a non-standard JSON
> extension, which is the single quotes, why not go a step further and
> permit the quotes to be omitted for simple tokens?
>
> #define QERR_DEV_NFOUND "{ code: 404, name: %s}"
>
> Much neater, IMHO.

While I agree about the neatness, this is quite a slippery slope.  If 
the macro tricks are going to confine the JSON templates in a header 
file, I don't see the need to extend the JSON parser further.

Paolo

Re: [Qemu-devel] Re: [RFC 0/7] QError v1

[Daniel P. Berrange]

On Fri, Oct 30, 2009 at 04:28:49PM +0000, Jamie Lokier wrote:
> Anthony Liguori wrote:
> > Okay, let's get more clever then and do:
> > 
> > #define QERR_DEV_NFOUND "{ 'code': 404, 'name': %s}"
> 
> By the way, since you've already invented a non-standard JSON
> extension, which is the single quotes, why not go a step further and
> permit the quotes to be omitted for simple tokens?

If we're going to use JSON we should be 100% compliant with the JSON
spec, not extend it. By adding custom QEMU extensions, we loose the
ability for programming language to trivially talk to QEMU using a
standard JSON parser, and instead everyone has to write custom client
side code yet again.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] Re: [RFC 0/7] QError v1

[Paolo Bonzini]

On 10/30/2009 06:15 PM, Daniel P. Berrange wrote:
> If we're going to use JSON we should be 100% compliant with the JSON
> spec, not extend it. By adding custom QEMU extensions, we loose the
> ability for programming language to trivially talk to QEMU using a
> standard JSON parser, and instead everyone has to write custom client
> side code yet again.

The single-quoted-string extension is just to be used internally to ease 
writing JSON templates in C.  All emitted text will go through the JSON 
encoder, which will be conservative (no extensions) for the reasons you 
mention.

Paolo

Re: [Qemu-devel] Re: [RFC 0/7] QError v1

[Anthony Liguori]

Paolo Bonzini wrote:
> On 10/30/2009 06:15 PM, Daniel P. Berrange wrote:
>> If we're going to use JSON we should be 100% compliant with the JSON
>> spec, not extend it. By adding custom QEMU extensions, we loose the
>> ability for programming language to trivially talk to QEMU using a
>> standard JSON parser, and instead everyone has to write custom client
>> side code yet again.
>
> The single-quoted-string extension is just to be used internally to 
> ease writing JSON templates in C.  All emitted text will go through 
> the JSON encoder, which will be conservative (no extensions) for the 
> reasons you mention.

Correct.  Furthermore, even our internal extensions should be as minimal 
as possible.  The only reason for allowing single quoted strings is that 
"{"\"key\": 32}" gets out of hand quickly.  It's not so bad in other 
languages that support single quoted strings but for C, it's really a 
necessary extension.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [RFC 0/7] QError v1

[Vincent Hanquez]

On Fri, Oct 30, 2009 at 06:33:15PM +0100, Paolo Bonzini wrote:
> On 10/30/2009 06:15 PM, Daniel P. Berrange wrote:
>> If we're going to use JSON we should be 100% compliant with the JSON
>> spec, not extend it. By adding custom QEMU extensions, we loose the
>> ability for programming language to trivially talk to QEMU using a
>> standard JSON parser, and instead everyone has to write custom client
>> side code yet again.
>
> The single-quoted-string extension is just to be used internally to ease  
> writing JSON templates in C.  All emitted text will go through the JSON  
> encoder, which will be conservative (no extensions) for the reasons you  
> mention.

Are you going to have a mechanism to turn off the extension when the parser is
parsing from external source then ?

because otherwise it means that qemu is accepting a superset of JSON; so how
long before client start by mistake doing the same thing instead of using
conformant JSON ?

I really can't imagine where typing \"..\" instead of '..' for a couple of
strings (10, 30 at most ?) make this single quote string a necessity.

-- 
Vincent

Re: [Qemu-devel] Re: [RFC 0/7] QError v1

[Anthony Liguori]

Jamie Lokier wrote:
> Anthony Liguori wrote:
>   
>> Okay, let's get more clever then and do:
>>
>> #define QERR_DEV_NFOUND "{ 'code': 404, 'name': %s}"
>>     
>
> By the way, since you've already invented a non-standard JSON
> extension, which is the single quotes, why not go a step further and
> permit the quotes to be omitted for simple tokens?
>
> #define QERR_DEV_NFOUND "{ code: 404, name: %s}"
>
> Much neater, IMHO.
>   

Javascript has keywords (like true, false, and null) that could lead to 
confusion using such a syntax.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [RFC 0/7] QError v1

[Jamie Lokier]

Anthony Liguori wrote:
> Jamie Lokier wrote:
> >Anthony Liguori wrote:
> >  
> >>Okay, let's get more clever then and do:
> >>
> >>#define QERR_DEV_NFOUND "{ 'code': 404, 'name': %s}"
> >>    
> >
> >By the way, since you've already invented a non-standard JSON
> >extension, which is the single quotes, why not go a step further and
> >permit the quotes to be omitted for simple tokens?
> >
> >#define QERR_DEV_NFOUND "{ code: 404, name: %s}"
> >
> >Much neater, IMHO.
> >  
> 
> Javascript has keywords (like true, false, and null) that could lead to 
> confusion using such a syntax.

I was thinking only before the colon in a dictionary.  Are the keywords
a problem in those positions, for Qemu?

-- Jamie

Re: [Qemu-devel] Re: [RFC 0/7] QError v1

[Paolo Bonzini]

On 10/30/2009 07:09 PM, Jamie Lokier wrote:
> Anthony Liguori wrote:
>> Jamie Lokier wrote:
>>> Anthony Liguori wrote:
>>>
>>>> Okay, let's get more clever then and do:
>>>>
>>>> #define QERR_DEV_NFOUND "{ 'code': 404, 'name': %s}"
>>>>
>>>
>>> By the way, since you've already invented a non-standard JSON
>>> extension, which is the single quotes, why not go a step further and
>>> permit the quotes to be omitted for simple tokens?
>>>
>>> #define QERR_DEV_NFOUND "{ code: 404, name: %s}"
>>>
>>> Much neater, IMHO.
>>>
>>
>> Javascript has keywords (like true, false, and null) that could lead to
>> confusion using such a syntax.
>
> I was thinking only before the colon in a dictionary.  Are the keywords
> a problem in those positions, for Qemu?

Maybe no, but that would complicate the parser uselessly.  I don't think 
two apostrophes are a huge problem, since 'code' is already a bigger 
improvement over \"code\".

Paolo

Re: [Qemu-devel] Re: [RFC 0/7] QError v1

[Anthony Liguori]

Jamie Lokier wrote:
> Anthony Liguori wrote:
>   
>> Jamie Lokier wrote:
>>     
>>> Anthony Liguori wrote:
>>>  
>>>       
>>>> Okay, let's get more clever then and do:
>>>>
>>>> #define QERR_DEV_NFOUND "{ 'code': 404, 'name': %s}"
>>>>    
>>>>         
>>> By the way, since you've already invented a non-standard JSON
>>> extension, which is the single quotes, why not go a step further and
>>> permit the quotes to be omitted for simple tokens?
>>>
>>> #define QERR_DEV_NFOUND "{ code: 404, name: %s}"
>>>
>>> Much neater, IMHO.
>>>  
>>>       
>> Javascript has keywords (like true, false, and null) that could lead to 
>> confusion using such a syntax.
>>     
>
> I was thinking only before the colon in a dictionary.  Are the keywords
> a problem in those positions, for Qemu?
>   

Eh, that's adding all sorts of nasty context-sensitivity and would abuse 
the lexer keyword tokens.

I really don't think it's justified.  I also think it's ugly :-)

Regards,

Anthony Liguori