[Qemu-devel] [RFC 0/8]: QError v2

[Qemu-devel] [RFC 0/8]: QError v2

[Luiz Capitulino]

 Hi,

 I can't remember seeing updated versions of a RFC series, but this should
prevent Anthony's scripts from merging these patches.

 This new QError version has two major changes: the static error table has
been dropped and I'm using symbolic names instead of error codes.

 Now, a call to:

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

 Would become something like:

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

 Which is basically what Anthony and other people were asking for, the
only difference is that I'm not passing the symbolic name through the
dictionary. The reason is that I have the impression it's less general
(as it becomes mandatory to have a dict) and slightly more complicaded.

 The symbolic name can be freely defined, but we can have the common ones
in qerror.h.

 Hopefully this version addresses the most important issues.

 Luiz.

[Qemu-devel] [PATCH 1/8] 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.143.g8cc62

[Qemu-devel] [PATCH 2/8] QString: Introduce qstring_append_chr()

[Luiz Capitulino]

It appends a C char to a QString.

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

diff --git a/qstring.c b/qstring.c
index 441a9e6..e422bd9 100644
--- a/qstring.c
+++ b/qstring.c
@@ -53,25 +53,39 @@ QString *qstring_from_str(const char *str)
     return qstring;
 }
 
-/* qstring_append(): Append a C string to a QString
- */
-void qstring_append(QString *qstring, const char *str)
+static void capacity_increase(QString *qstring, size_t len)
 {
-    size_t len = strlen(str);
-
     if (qstring->capacity < (qstring->length + len)) {
         qstring->capacity += len;
         qstring->capacity *= 2; /* use exponential growth */
 
         qstring->string = qemu_realloc(qstring->string, qstring->capacity + 1);
     }
+}
+
+/* qstring_append(): Append a C string to a QString
+ */
+void qstring_append(QString *qstring, const char *str)
+{
+    size_t len = strlen(str);
 
+    capacity_increase(qstring, len);
     memcpy(qstring->string + qstring->length, str, len);
     qstring->length += len;
     qstring->string[qstring->length] = 0;
 }
 
 /**
+ * qstring_append_chr(): Append a C char to a QString
+ */
+void qstring_append_chr(QString *qstring, int c)
+{
+    capacity_increase(qstring, 1);
+    qstring->string[qstring->length++] = c;
+    qstring->string[qstring->length] = 0;
+}
+
+/**
  * qobject_to_qstring(): Convert a QObject to a QString
  */
 QString *qobject_to_qstring(const QObject *obj)
diff --git a/qstring.h b/qstring.h
index 65905d4..43581de 100644
--- a/qstring.h
+++ b/qstring.h
@@ -14,6 +14,7 @@ QString *qstring_new(void);
 QString *qstring_from_str(const char *str);
 const char *qstring_get_str(const QString *qstring);
 void qstring_append(QString *qstring, const char *str);
+void qstring_append_chr(QString *qstring, int c);
 QString *qobject_to_qstring(const QObject *obj);
 
 #endif /* QSTRING_H */
-- 
1.6.5.2.143.g8cc62

[Qemu-devel] [PATCH 3/8] Add qstring_append_chr() unit-test

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 check-qstring.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/check-qstring.c b/check-qstring.c
index ea4dfd0..412038a 100644
--- a/check-qstring.c
+++ b/check-qstring.c
@@ -55,6 +55,22 @@ START_TEST(qstring_get_str_test)
 }
 END_TEST
 
+START_TEST(qstring_append_chr_test)
+{
+    int i;
+    QString *qstring;
+    const char *str = "qstring append char unit-test";
+
+    qstring = qstring_new();
+
+    for (i = 0; str[i]; i++)
+        qstring_append_chr(qstring, str[i]);
+
+    fail_unless(strcmp(str, qstring_get_str(qstring)) == 0);
+    QDECREF(qstring);
+}
+END_TEST
+
 START_TEST(qobject_to_qstring_test)
 {
     QString *qstring;
@@ -78,6 +94,7 @@ static Suite *qstring_suite(void)
     tcase_add_test(qstring_public_tcase, qstring_from_str_test);
     tcase_add_test(qstring_public_tcase, qstring_destroy_test);
     tcase_add_test(qstring_public_tcase, qstring_get_str_test);
+    tcase_add_test(qstring_public_tcase, qstring_append_chr_test);
     tcase_add_test(qstring_public_tcase, qobject_to_qstring_test);
 
     return s;
-- 
1.6.5.2.143.g8cc62

[Qemu-devel] [PATCH 4/8] QString: Introduce qstring_append_int()

[Luiz Capitulino]

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

diff --git a/qstring.c b/qstring.c
index e422bd9..ad17769 100644
--- a/qstring.c
+++ b/qstring.c
@@ -75,6 +75,14 @@ void qstring_append(QString *qstring, const char *str)
     qstring->string[qstring->length] = 0;
 }
 
+void qstring_append_int(QString *qstring, int64_t value)
+{
+    char num[32];
+
+    snprintf(num, sizeof(num), "%" PRId64, value);
+    qstring_append(qstring, num);
+}
+
 /**
  * qstring_append_chr(): Append a C char to a QString
  */
diff --git a/qstring.h b/qstring.h
index 43581de..c065331 100644
--- a/qstring.h
+++ b/qstring.h
@@ -1,6 +1,7 @@
 #ifndef QSTRING_H
 #define QSTRING_H
 
+#include <stdint.h>
 #include "qobject.h"
 
 typedef struct QString {
@@ -13,6 +14,7 @@ typedef struct QString {
 QString *qstring_new(void);
 QString *qstring_from_str(const char *str);
 const char *qstring_get_str(const QString *qstring);
+void qstring_append_int(QString *qstring, int64_t value);
 void qstring_append(QString *qstring, const char *str);
 void qstring_append_chr(QString *qstring, int c);
 QString *qobject_to_qstring(const QObject *obj);
-- 
1.6.5.2.143.g8cc62

[Qemu-devel] [PATCH 5/8] Introduce QError

[Luiz Capitulino]

QError is a high-level data type which represents an exception,
it stores the following error information:

- name           A generic error name (eg. "ServiceUnavailable")
- description    A detailed error description, which may contain
                 references to run-time error data
- filename       The file name of where the error occurred
- line number    The exact line number of the error
- run-time data  Run-time error data

The qerror_print() function should be used to properly format and
print the stored information to the right place, that is, to stderr
if the Monitor is not running, or to the Monitor's device otherwise.

The following functions are exported:

- qerror_new(): Create a new QError
- qerror_from_info(): Create a new QError from the specified error
                      information
- qerror_print(): Print the specified QError

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

diff --git a/Makefile b/Makefile
index 6fcbcaa..2dfaebd 100644
--- a/Makefile
+++ b/Makefile
@@ -135,7 +135,7 @@ obj-y += buffered_file.o migration.o migration-tcp.o qemu-sockets.o
 obj-y += qemu-char.o aio.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..7d3137f
--- /dev/null
+++ b/qerror.c
@@ -0,0 +1,235 @@
+/*
+ * 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,
+};
+
+/**
+ * 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:
+ *
+ * - name    generic error name
+ * - file    the file name of where the error occurred
+ * - linenr  the line number of where the error occurred
+ * - desc    detailed description (see below)
+ * - fmt     JSON printf-like format for 'specific data'
+ * - va      va_list of all arguments for 'specific data'
+ *
+ * Note that this is a low-level function, it is supposed to be called
+ * by higher-level functions or macros.
+ *
+ * The 'desc' parameter 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().
+ *
+ * Valid types are:
+ *
+ * s  (string)
+ * d  (integer)
+ *
+ * Example:
+ *
+ * "foo error on device: %(device)s slot: %(slot_nr)d"
+ *
+ * A single percent sign can be printed if followed by a second one,
+ * for example:
+ *
+ * "running out of foo: %(foo)d%%"
+ *
+ * Return strong reference.
+ */
+QError *qerror_from_info(const char *name, const char *file, int linenr,
+                         const char *desc, const char *fmt, va_list *va)
+{
+    QError *qerr;
+
+    qerr = qerror_new();
+    qerr->name = name;
+    qerr->file = file;
+    qerr->linenr = linenr;
+    qerr->desc = desc;
+    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 *qerr)
+{
+    fprintf(stderr, " in '%s' at %s:%d\n", qerr->desc, qerr->file,qerr->linenr);
+    abort();
+}
+
+#define ERROR_PREFIX "\n\nqerror: "
+
+static void type_error(const QError *qerr, int c)
+{
+    fprintf(stderr, ERROR_PREFIX "invalid type '%c'", c);
+    qerror_abort(qerr);
+}
+
+static void key_error(const QError *qerr, const char *key)
+{
+    fprintf(stderr, ERROR_PREFIX "key '%s' not found in QDict ", key);
+    fprintf(stderr, "call at %s:%d\n", qerr->file, qerr->linenr);
+    abort();
+}
+
+static void parse_error(const QError *qerror, int c)
+{
+    fprintf(stderr, ERROR_PREFIX "expected '%c'", c);
+    qerror_abort(qerror);
+}
+
+static const char *append_field(QString *qstring, const QError *qerror,
+                                const char *start)
+{
+    int type;
+    char *name;
+    QDict *qdict;
+    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':
+            qstring_append(qstring, qdict_get_str(qdict, name));
+            break;
+        case 'd':
+            qstring_append_int(qstring, 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;
+    QString *qstring;
+
+    assert(qerror->desc != NULL);
+
+    qstring = qstring_new();
+
+    for (p = qerror->desc; *p != '\0';) {
+        if (*p != '%') {
+            qstring_append_chr(qstring, *p++);
+        } else if (*(p + 1) == '%') {
+            qstring_append_chr(qstring, '%');
+            p += 2;
+        } else {
+            p = append_field(qstring, qerror, p);
+        }
+    }
+
+    qemu_error("%s\n", qstring_get_str(qstring));
+    QDECREF(qstring);
+}
+
+/**
+ * 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..dc39dec
--- /dev/null
+++ b/qerror.h
@@ -0,0 +1,39 @@
+/*
+ * 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"
+
+typedef struct QError {
+    QObject_HEAD;
+    const char *name;   /* generic name */
+    const char *desc;   /* description */
+    int linenr;         /* line number */
+    const char *file;   /* file name */
+    QObject *data;      /* run-time data */
+} QError;
+
+QError *qerror_new(void);
+QError *qerror_from_info(const char *name, const char *file, int linenr,
+                         const char *desc, const char *fmt, va_list *va);
+void qerror_print(const QError *qerror);
+QError *qobject_to_qerror(const QObject *obj);
+
+/*
+ * Common error names
+ */
+#define QERR_DEV_NFOUND "DeviceNotFound"
+#define QERR_SER_UNAV   "ServiceUnavailable"
+
+#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.143.g8cc62

[Qemu-devel] [PATCH 6/8] monitor: QError support

[Luiz Capitulino]

This commit adds QError support in the Monitor.

A QError member is added to the Monitor struct. This new member
stores error information and is also used to check if an error
has occurred when the called handlers returns.

Additionally, a new macro called qemu_error_new() is introduced.
It should be used in pace of qemu_error() to report errors.

When all conversion to qemu_error_new() is done, qemu_error() can
be turned private.

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

1. An error occurs in the handler, it calls qemu_error_new()
2. qemu_error_new() 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

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..0e6e21b 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_full(const char *name, const char *file, int linenr,
+                     const char *desc, const char *fmt, ...)
+{
+    va_list va;
+    QError *qerror;
+
+    assert(qemu_error_sink != NULL);
+
+    va_start(va, fmt);
+    qerror = qerror_from_info(name, file, linenr, desc, 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 96804b4..20ab709 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_full(const char *name, const char *file, int linenr,
+                     const char *desc, const char *fmt, ...)
+                     __attribute__ ((format(printf, 5, 6)));
+
+#define qemu_error_new(name, desc, fmt, ...) \
+    qemu_error_full(name, __FILE__, __LINE__, desc, fmt, ## __VA_ARGS__)
 
 #ifdef _WIN32
 /* Polling handling */
-- 
1.6.5.2.143.g8cc62

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

[Luiz Capitulino]

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

diff --git a/hw/qdev.c b/hw/qdev.c
index c7884d0..db86cb2 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,8 @@ 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_new(QERR_DEV_NFOUND, "device \"%(name)s\" not found",
+                       "{ 'name': %s }", driver);
         return NULL;
     }
     if (info->no_user) {
-- 
1.6.5.2.143.g8cc62

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

[Luiz Capitulino]

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

diff --git a/monitor.c b/monitor.c
index 0e6e21b..423c6b5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1717,10 +1717,11 @@ 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_new(QERR_SER_UNAV,
+                       "Using KVM without synchronous MMU, ballooning disabled",
+                       NULL);
     else if (actual == 0)
-        monitor_printf(mon, "Ballooning not activated in VM\n");
+        qemu_error_new(QERR_SER_UNAV, "Ballooning not activated in VM", NULL);
     else
         *ret_data = QOBJECT(qint_from_int((int)(actual >> 20)));
 }
-- 
1.6.5.2.143.g8cc62

[Qemu-devel] Re: [PATCH 5/8] Introduce QError

[Paolo Bonzini]

On 11/04/2009 09:04 PM, Luiz Capitulino wrote:
> +#define QERR_DEV_NFOUND "DeviceNotFound"
> +#define QERR_SER_UNAV   "ServiceUnavailable"

I don't like packing names this way (what's wrong with DEVICE_NOT_FOUND 
and SERVICE_UNAVAILABLE?...), but otherwise the series looks fine.

Paolo

Re: [Qemu-devel] [RFC 0/8]: QError v2

[Anthony Liguori]

Luiz Capitulino wrote:
>  Hi,
>
>  I can't remember seeing updated versions of a RFC series, but this should
> prevent Anthony's scripts from merging these patches.
>
>  This new QError version has two major changes: the static error table has
> been dropped and I'm using symbolic names instead of error codes.
>
>  Now, a call to:
>
> monitor_printf(mon, "husb: host usb device %d.%d is already open\n",
>                bus_num, addr);
>
>  Would become something like:
>
> qemu_error_new('DeviceAlreadyOpen', "{ 'bus_num': %d, 'addr': %d }",
>                bus_num, addr);
>   

I mostly like this but this is not what the patches do :-)

Here's what I would like to see:

#define QERR_DEVICE_ALREADY_OPEN "{'class': 'DeviceAlreadyOpen', 'data' 
: {'bus_num': %d, 'addr': %d}"

qemu_error_new(QERR_DEVICE_ALREADY_OPEN, bus_num, addr);

That gives us a nice simple interface with full error checking on the 
parameters.

For human readable strings, I'd suggest making a table somewhere else 
that looked like:

QErrorStringTable qerror_descriptions[] = {
{ QERR_DEVICE_ALREADY_OPEN, "This device at %(bus_num)d.%(addr)d is 
already open." },
...
};

There are a number of advantages to an approach like this.  The table 
can be reused by both in the server and by a client.

Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC 0/8]: QError v2

[Luiz Capitulino]

On Wed, 11 Nov 2009 15:20:30 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> Luiz Capitulino wrote:
> >  Hi,
> >
> >  I can't remember seeing updated versions of a RFC series, but this should
> > prevent Anthony's scripts from merging these patches.
> >
> >  This new QError version has two major changes: the static error table has
> > been dropped and I'm using symbolic names instead of error codes.
> >
> >  Now, a call to:
> >
> > monitor_printf(mon, "husb: host usb device %d.%d is already open\n",
> >                bus_num, addr);
> >
> >  Would become something like:
> >
> > qemu_error_new('DeviceAlreadyOpen', "{ 'bus_num': %d, 'addr': %d }",
> >                bus_num, addr);
> >   
> 
> I mostly like this but this is not what the patches do :-)
> 
> Here's what I would like to see:
> 
> #define QERR_DEVICE_ALREADY_OPEN "{'class': 'DeviceAlreadyOpen', 'data' 
> : {'bus_num': %d, 'addr': %d}"
> 
> qemu_error_new(QERR_DEVICE_ALREADY_OPEN, bus_num, addr);
> 
> That gives us a nice simple interface with full error checking on the 
> parameters.
> 
> For human readable strings, I'd suggest making a table somewhere else 
> that looked like:
> 
> QErrorStringTable qerror_descriptions[] = {
> { QERR_DEVICE_ALREADY_OPEN, "This device at %(bus_num)d.%(addr)d is 
> already open." },
> ...
> };
> 
> There are a number of advantages to an approach like this.  The table 
> can be reused by both in the server and by a client.

 I'm ok with this, the only comment I have is that 'class' should
be a member of QErrorStringTable, so that we can use it to lookup the
table.

Re: [Qemu-devel] [RFC 0/8]: QError v2

[Luiz Capitulino]

On Wed, 11 Nov 2009 15:20:30 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> Luiz Capitulino wrote:
> >  Hi,
> >
> >  I can't remember seeing updated versions of a RFC series, but this should
> > prevent Anthony's scripts from merging these patches.
> >
> >  This new QError version has two major changes: the static error table has
> > been dropped and I'm using symbolic names instead of error codes.
> >
> >  Now, a call to:
> >
> > monitor_printf(mon, "husb: host usb device %d.%d is already open\n",
> >                bus_num, addr);
> >
> >  Would become something like:
> >
> > qemu_error_new('DeviceAlreadyOpen', "{ 'bus_num': %d, 'addr': %d }",
> >                bus_num, addr);
> >   
> 
> I mostly like this but this is not what the patches do :-)
> 
> Here's what I would like to see:

 Thinking about this a little more I see some problems with it.

> #define QERR_DEVICE_ALREADY_OPEN "{'class': 'DeviceAlreadyOpen', 'data' 
> : {'bus_num': %d, 'addr': %d}"
> 
> qemu_error_new(QERR_DEVICE_ALREADY_OPEN, bus_num, addr);

 What about DeviceAlreadyOpen errors with a different argument list?

> That gives us a nice simple interface with full error checking on the 
> parameters.

 I've said this is not so simple because people writing those macros
would find out that the 'class' or 'data' _keys_ are missing or incorrect
only at run-time, when the error is triggered.

> For human readable strings, I'd suggest making a table somewhere else 
> that looked like:
> 
> QErrorStringTable qerror_descriptions[] = {
> { QERR_DEVICE_ALREADY_OPEN, "This device at %(bus_num)d.%(addr)d is 
> already open." },
> ...
> };

 How do you suggest we lookup the table? Doing a strcmp() on
QERR_DEVICE_ALREADY_OPEN?

> There are a number of advantages to an approach like this.  The table 
> can be reused by both in the server and by a client.

 My suggestions on both problems makes me willing go back to my initial
series, which had a table indexed by an error number.

Re: [Qemu-devel] [RFC 0/8]: QError v2

[Anthony Liguori]

Luiz Capitulino wrote:
>
>> #define QERR_DEVICE_ALREADY_OPEN "{'class': 'DeviceAlreadyOpen', 'data' 
>> : {'bus_num': %d, 'addr': %d}"
>>
>> qemu_error_new(QERR_DEVICE_ALREADY_OPEN, bus_num, addr);
>>     
>
>  What about DeviceAlreadyOpen errors with a different argument list?
>   

Why would you have this?  That would seem like a problem to me.  I think 
the errors need to be very well structured (just like everything else in 
QMP).

>> That gives us a nice simple interface with full error checking on the 
>> parameters.
>>     
>
>  I've said this is not so simple because people writing those macros
> would find out that the 'class' or 'data' _keys_ are missing or incorrect
> only at run-time, when the error is triggered.
>   

Sure but introducing new types of errors is not the common case.  Using 
existing errors is the common case.

>> For human readable strings, I'd suggest making a table somewhere else 
>> that looked like:
>>
>> QErrorStringTable qerror_descriptions[] = {
>> { QERR_DEVICE_ALREADY_OPEN, "This device at %(bus_num)d.%(addr)d is 
>> already open." },
>> ...
>> };
>>     
>
>  How do you suggest we lookup the table? Doing a strcmp() on
> QERR_DEVICE_ALREADY_OPEN?
>   

We can either change the index on the table to be just the class code or 
find something more clever.

>> There are a number of advantages to an approach like this.  The table 
>> can be reused by both in the server and by a client.
>>     
>
>  My suggestions on both problems makes me willing go back to my initial
> series, which had a table indexed by an error number.
>   

I don't understand why.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC 0/8]: QError v2

[Luiz Capitulino]

On Thu, 12 Nov 2009 08:44:03 -0600
Anthony Liguori <aliguori@linux.vnet.ibm.com> wrote:

> Luiz Capitulino wrote:
> >
> >> #define QERR_DEVICE_ALREADY_OPEN "{'class': 'DeviceAlreadyOpen', 'data' 
> >> : {'bus_num': %d, 'addr': %d}"
> >>
> >> qemu_error_new(QERR_DEVICE_ALREADY_OPEN, bus_num, addr);
> >>     
> >
> >  What about DeviceAlreadyOpen errors with a different argument list?
> >   
> 
> Why would you have this?  That would seem like a problem to me.  I think 
> the errors need to be very well structured (just like everything else in 
> QMP).

 This can happen with errors that carry specific info which are different
among subsystems, eg. USB device info vs. PCI device info.

 We could have 'USBDeviceAlreadyOpen', but then I think the class
attribute will lose generality.

> >> That gives us a nice simple interface with full error checking on the 
> >> parameters.
> >>     
> >
> >  I've said this is not so simple because people writing those macros
> > would find out that the 'class' or 'data' _keys_ are missing or incorrect
> > only at run-time, when the error is triggered.
> >   
> 
> Sure but introducing new types of errors is not the common case.  Using 
> existing errors is the common case.

 Right, although there's a long road until we stabilize.

> >> For human readable strings, I'd suggest making a table somewhere else 
> >> that looked like:
> >>
> >> QErrorStringTable qerror_descriptions[] = {
> >> { QERR_DEVICE_ALREADY_OPEN, "This device at %(bus_num)d.%(addr)d is 
> >> already open." },
> >> ...
> >> };
> >>     
> >
> >  How do you suggest we lookup the table? Doing a strcmp() on
> > QERR_DEVICE_ALREADY_OPEN?
> >   
> 
> We can either change the index on the table to be just the class code or 
> find something more clever.

 I'm working on this and trying to find something.
 
> >> There are a number of advantages to an approach like this.  The table 
> >> can be reused by both in the server and by a client.
> >>     
> >
> >  My suggestions on both problems makes me willing go back to my initial
> > series, which had a table indexed by an error number.
> >   
> 
> I don't understand why.

 I've found other problems with it, let's pretend I didn't mention it. :)

Re: [Qemu-devel] [RFC 0/8]: QError v2

[Anthony Liguori]

Luiz Capitulino wrote:
> On Thu, 12 Nov 2009 08:44:03 -0600
> Anthony Liguori <aliguori@linux.vnet.ibm.com> wrote:
>
>   
>> Luiz Capitulino wrote:
>>     
>>>> #define QERR_DEVICE_ALREADY_OPEN "{'class': 'DeviceAlreadyOpen', 'data' 
>>>> : {'bus_num': %d, 'addr': %d}"
>>>>
>>>> qemu_error_new(QERR_DEVICE_ALREADY_OPEN, bus_num, addr);
>>>>     
>>>>         
>>>  What about DeviceAlreadyOpen errors with a different argument list?
>>>   
>>>       
>> Why would you have this?  That would seem like a problem to me.  I think 
>> the errors need to be very well structured (just like everything else in 
>> QMP).
>>     
>
>  This can happen with errors that carry specific info which are different
> among subsystems, eg. USB device info vs. PCI device info.
>   

This example demonstrates the problem with this.  You haven't included 
enough information in the error to properly distinguish what happened.  
I would suggest changing to:

'data' : { 'device_type' : %s, 'addr': %s }

qemu_error_new(QERR_DEVICE_ALREADY_OPEN, "pci", addr);

Or, if you really wanted to get fancy:

'data' : { 'device_type': %s, 'addr': %p }

qemu_error_new(QERR_DEVICE_ALREADY_OPEN, "pci",
                            qobject_from_jsonf("{'bus': %d, 'slot': %d, 
'function': %d}", bus, dev, fn));

Personally, I'd stick with the first one though.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC 0/8]: QError v2

[Markus Armbruster]

Anthony Liguori <aliguori@linux.vnet.ibm.com> writes:

> Luiz Capitulino wrote:
>>
>>> #define QERR_DEVICE_ALREADY_OPEN "{'class': 'DeviceAlreadyOpen',
>>> data' : {'bus_num': %d, 'addr': %d}"
>>>
>>> qemu_error_new(QERR_DEVICE_ALREADY_OPEN, bus_num, addr);
>>>     
>>
>>  What about DeviceAlreadyOpen errors with a different argument list?
>>   
>
> Why would you have this?  That would seem like a problem to me.  I
> think the errors need to be very well structured (just like everything
> else in QMP).
>
>>> That gives us a nice simple interface with full error checking on
>>> the parameters.
>>>     
>>
>>  I've said this is not so simple because people writing those macros
>> would find out that the 'class' or 'data' _keys_ are missing or incorrect
>> only at run-time, when the error is triggered.
>>   
>
> Sure but introducing new types of errors is not the common case.
> Using existing errors is the common case.

Careful.  (Valid) reuse of existing errors may be more common, but
proper creation of new errors is important.  If you make that too hard
compared to reuse, you risk abuse of existing errors for cases where
they don't quite fit.

[...]