[Qemu-devel] [PATCH 00/10]: QError v4

[Qemu-devel] [PATCH 00/10]: QError v4

[Luiz Capitulino]

 Hi,

 This new QError version has some small improvements and is a candidate
for merging, as qjson is already on master.

 Just to remind you that it implements what was suggested by Anthony
in this email:

http://lists.gnu.org/archive/html/qemu-devel/2009-11/msg00601.html

 Basically, the error table is back and qemu_error_new() calls are like
this:

qemu_error_new(QERR_DEVICE_NOT_FOUND, driver);

changelog
---------

v3 -> v4

- Change the license to LGPL
- Add qstring_from_substr()
- qobject_from_ functions are wrappers of qobject_from_jsonv()
- Drop the type specifier from 'desc'
- Minor changes

v0 -> v3

- Big design changes

[Qemu-devel] [PATCH 01/10] QJSON: Introduce qobject_from_jsonv()

[Luiz Capitulino]

It accepts a va_list and will be used by QError. Also simplifies
the code a little, as the other qobject_from_() functions can
use it.

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

diff --git a/qjson.c b/qjson.c
index 7270909..12e6cf0 100644
--- a/qjson.c
+++ b/qjson.c
@@ -34,10 +34,12 @@ static void parse_json(JSONMessageParser *parser, QList *tokens)
     s->result = json_parser_parse(tokens, s->ap);
 }
 
-QObject *qobject_from_json(const char *string)
+QObject *qobject_from_jsonv(const char *string, va_list *ap)
 {
     JSONParsingState state = {};
 
+    state.ap = ap;
+
     json_message_parser_init(&state.parser, parse_json);
     json_message_parser_feed(&state.parser, string, strlen(string));
     json_message_parser_flush(&state.parser);
@@ -46,22 +48,21 @@ QObject *qobject_from_json(const char *string)
     return state.result;
 }
 
+QObject *qobject_from_json(const char *string)
+{
+    return qobject_from_jsonv(string, NULL);
+}
+
 QObject *qobject_from_jsonf(const char *string, ...)
 {
-    JSONParsingState state = {};
+    QObject *obj;
     va_list ap;
 
     va_start(ap, string);
-    state.ap = &ap;
-
-    json_message_parser_init(&state.parser, parse_json);
-    json_message_parser_feed(&state.parser, string, strlen(string));
-    json_message_parser_flush(&state.parser);
-    json_message_parser_destroy(&state.parser);
-
+    obj = qobject_from_jsonv(string, &ap);
     va_end(ap);
 
-    return state.result;
+    return obj;
 }
 
 typedef struct ToJsonIterState
diff --git a/qjson.h b/qjson.h
index 7fce742..7afec2e 100644
--- a/qjson.h
+++ b/qjson.h
@@ -14,12 +14,14 @@
 #ifndef QJSON_H
 #define QJSON_H
 
+#include <stdarg.h>
 #include "qobject.h"
 #include "qstring.h"
 
 QObject *qobject_from_json(const char *string);
 QObject *qobject_from_jsonf(const char *string, ...)
     __attribute__((__format__ (__printf__, 1, 2)));
+QObject *qobject_from_jsonv(const char *string, va_list *ap);
 
 QString *qobject_to_json(const QObject *obj);
 
-- 
1.6.5.2.180.gc5b3e

[Qemu-devel] [PATCH 02/10] 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.180.gc5b3e

[Qemu-devel] [PATCH 03/10] 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.180.gc5b3e

[Qemu-devel] [PATCH 04/10] QString: Introduce qstring_from_substr()

[Luiz Capitulino]

Note that we can now write qstring_from_str() as a wrapper.

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

diff --git a/qstring.c b/qstring.c
index ad17769..740a106 100644
--- a/qstring.c
+++ b/qstring.c
@@ -31,21 +31,21 @@ QString *qstring_new(void)
 }
 
 /**
- * qstring_from_str(): Create a new QString from a regular C string
+ * qstring_from_substr(): Create a new QString from a C string substring
  *
- * Return strong reference.
+ * Return string reference
  */
-QString *qstring_from_str(const char *str)
+QString *qstring_from_substr(const char *str, int start, int end)
 {
     QString *qstring;
 
     qstring = qemu_malloc(sizeof(*qstring));
 
-    qstring->length = strlen(str);
+    qstring->length = end - start + 1;
     qstring->capacity = qstring->length;
 
     qstring->string = qemu_malloc(qstring->capacity + 1);
-    memcpy(qstring->string, str, qstring->length);
+    memcpy(qstring->string, str + start, qstring->length);
     qstring->string[qstring->length] = 0;
 
     QOBJECT_INIT(qstring, &qstring_type);
@@ -53,6 +53,16 @@ QString *qstring_from_str(const char *str)
     return qstring;
 }
 
+/**
+ * qstring_from_str(): Create a new QString from a regular C string
+ *
+ * Return strong reference.
+ */
+QString *qstring_from_str(const char *str)
+{
+    return qstring_from_substr(str, 0, strlen(str) - 1);
+}
+
 static void capacity_increase(QString *qstring, size_t len)
 {
     if (qstring->capacity < (qstring->length + len)) {
diff --git a/qstring.h b/qstring.h
index c065331..6aaa7d5 100644
--- a/qstring.h
+++ b/qstring.h
@@ -13,6 +13,7 @@ typedef struct QString {
 
 QString *qstring_new(void);
 QString *qstring_from_str(const char *str);
+QString *qstring_from_substr(const char *str, int start, int end);
 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);
-- 
1.6.5.2.180.gc5b3e

[Qemu-devel] [PATCH 05/10] utests: 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.180.gc5b3e

[Qemu-devel] [PATCH 06/10] utests: Add qstring_from_substr() unit-test

[Luiz Capitulino]

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

diff --git a/check-qstring.c b/check-qstring.c
index 412038a..c308a63 100644
--- a/check-qstring.c
+++ b/check-qstring.c
@@ -71,6 +71,19 @@ START_TEST(qstring_append_chr_test)
 }
 END_TEST
 
+START_TEST(qstring_from_substr_test)
+{
+    QString *qs;
+
+    qs = qstring_from_substr("virtualization", 3, 9);
+    fail_unless(qs != NULL);
+    fail_unless(strcmp(qstring_get_str(qs), "tualiza") == 0);
+
+    QDECREF(qs);
+}
+END_TEST
+
+
 START_TEST(qobject_to_qstring_test)
 {
     QString *qstring;
@@ -95,6 +108,7 @@ static Suite *qstring_suite(void)
     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, qstring_from_substr_test);
     tcase_add_test(qstring_public_tcase, qobject_to_qstring_test);
 
     return s;
-- 
1.6.5.2.180.gc5b3e

[Qemu-devel] [PATCH 07/10] Introduce QError

[Luiz Capitulino]

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

- class          Error class name (eg. "ServiceUnavailable")
- description    A detailed error description, which can 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  Any run-time error data

This commit adds the basic interface plus two error classes, one
for 'device not found' errors and another one for 'service unavailable'
errors.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 Makefile  |    2 +-
 qerror.c  |  265 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qerror.h  |   46 +++++++++++
 qjson.c   |    2 +
 qobject.h |    1 +
 5 files changed, 315 insertions(+), 1 deletions(-)
 create mode 100644 qerror.c
 create mode 100644 qerror.h

diff --git a/Makefile b/Makefile
index d770e2a..c0b65b6 100644
--- a/Makefile
+++ b/Makefile
@@ -138,7 +138,7 @@ 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 json-lexer.o
-obj-y += json-streamer.o json-parser.o qjson.o
+obj-y += json-streamer.o json-parser.o qjson.o qerror.o
 obj-y += qemu-config.o block-migration.o
 
 obj-$(CONFIG_BRLAPI) += baum.o
diff --git a/qerror.c b/qerror.c
new file mode 100644
index 0000000..beb215d
--- /dev/null
+++ b/qerror.c
@@ -0,0 +1,265 @@
+/*
+ * 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 LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+#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' parameter is a printf-like string, the format of the format
+ * string is:
+ *
+ * %(KEY)
+ *
+ * Where KEY is a QDict key, which has to be passed to qerror_from_info().
+ *
+ * Example:
+ *
+ * "foo error on device: %(device) slot: %(slot_nr)"
+ *
+ * A single percent sign can be printed if followed by a second one,
+ * for example:
+ *
+ * "running out of foo: %(foo)%%"
+ */
+const QErrorStringTable qerror_table[] = {
+    {
+        .error_fmt   = QERR_DEVICE_NOT_FOUND,
+        .desc        = "device \"%(name)\" not found",
+    },
+    {
+        .error_fmt   = QERR_SERVICE_UNAVAILABLE,
+        .desc        = "%(reason)",
+    },
+    {}
+};
+
+/**
+ * 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;
+}
+
+static void qerror_abort(const QError *qerr, const char *fmt, ...)
+{
+    va_list ap;
+
+    fprintf(stderr, "qerror: ");
+
+    va_start(ap, fmt);
+    vfprintf(stderr, fmt, ap);
+    va_end(ap);
+
+    fprintf(stderr, " - call at %s:%d\n", qerr->file, qerr->linenr);
+    abort();
+}
+
+static void qerror_set_data(QError *qerr, const char *fmt, va_list *va)
+{
+    QObject *obj;
+
+    obj = qobject_from_jsonv(fmt, va);
+    if (!obj) {
+        qerror_abort(qerr, "invalid format '%s'", fmt);
+    }
+    if (qobject_type(obj) != QTYPE_QDICT) {
+        qerror_abort(qerr, "error format is not a QDict '%s'", fmt);
+    }
+
+    qerr->error = qobject_to_qdict(obj);
+
+    obj = qdict_get(qerr->error, "class");
+    if (!obj) {
+        qerror_abort(qerr, "missing 'class' key in '%s'", fmt);
+    }
+    if (qobject_type(obj) != QTYPE_QSTRING) {
+        qerror_abort(qerr, "'class' key value should be a QString");
+    }
+    
+    obj = qdict_get(qerr->error, "data");
+    if (!obj) {
+        qerror_abort(qerr, "missing 'data' key in '%s'", fmt);
+    }
+    if (qobject_type(obj) != QTYPE_QDICT) {
+        qerror_abort(qerr, "'data' key value should be a QDICT");
+    }
+}
+
+static void qerror_set_desc(const char *fmt, QError *qerr)
+{
+    int i;
+
+    // FIXME: inefficient loop
+
+    for (i = 0; qerror_table[i].error_fmt; i++) {
+        if (strcmp(qerror_table[i].error_fmt, fmt) == 0) {
+            qerr->entry = &qerror_table[i];
+            return;
+        }
+    }
+
+    qerror_abort(qerr, "error format '%s' not found", fmt);
+}
+
+/**
+ * qerror_from_info(): Create a new QError from error information
+ *
+ * The information consists of:
+ *
+ * - file   the file name of where the error occurred
+ * - linenr the line number of where the error occurred
+ * - fmt    JSON printf-like dictionary, there must exist keys 'class' and
+ *          'data'
+ * - va     va_list of all arguments specified by fmt
+ *
+ * Return strong reference.
+ */
+QError *qerror_from_info(const char *file, int linenr, const char *fmt,
+                         va_list *va)
+{
+    QError *qerr;
+
+    qerr = qerror_new();
+    qerr->linenr = linenr;
+    qerr->file = file;
+
+    if (!fmt) {
+        qerror_abort(qerr, "QDict not specified");
+    }
+
+    qerror_set_data(qerr, fmt, va);
+    qerror_set_desc(fmt, qerr);
+
+    return qerr;
+}
+
+static void parse_error(const QError *qerror, int c)
+{
+    qerror_abort(qerror, "expected '%c' in '%s'", c, qerror->entry->desc);
+}
+
+static const char *append_field(QString *outstr, const QError *qerror,
+                                const char *start)
+{
+    QObject *obj;
+    QDict *qdict;
+    QString *key_qs;
+    const char *end, *key;
+
+    if (*start != '%')
+        parse_error(qerror, '%');
+    start++;
+    if (*start != '(')
+        parse_error(qerror, '(');
+    start++;
+
+    end = strchr(start, ')');
+    if (!end)
+        parse_error(qerror, ')');
+
+    key_qs = qstring_from_substr(start, 0, end - start - 1);
+    key = qstring_get_str(key_qs);
+
+    qdict = qobject_to_qdict(qdict_get(qerror->error, "data"));
+    obj = qdict_get(qdict, key);
+    if (!obj) {
+        qerror_abort(qerror, "key '%s' not found in QDict", key);
+    }
+
+    switch (qobject_type(obj)) {
+        case QTYPE_QSTRING:
+            qstring_append(outstr, qdict_get_str(qdict, key));
+            break;
+        case QTYPE_QINT:
+            qstring_append_int(outstr, qdict_get_int(qdict, key));
+            break;
+        default:
+            qerror_abort(qerror, "invalid type '%c'", qobject_type(obj));
+    }
+
+    QDECREF(key_qs);
+    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->entry != NULL);
+
+    qstring = qstring_new();
+
+    for (p = qerror->entry->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);
+
+    QDECREF(qerr->error);
+    qemu_free(qerr);
+}
diff --git a/qerror.h b/qerror.h
new file mode 100644
index 0000000..d262863
--- /dev/null
+++ b/qerror.h
@@ -0,0 +1,46 @@
+/*
+ * 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 LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+#ifndef QERROR_H
+#define QERROR_H
+
+#include "qdict.h"
+#include <stdarg.h>
+
+typedef struct QErrorStringTable {
+    const char *desc;
+    const char *error_fmt;
+} QErrorStringTable;
+
+typedef struct QError {
+    QObject_HEAD;
+    QDict *error;
+    int linenr;
+    const char *file;
+    const QErrorStringTable *entry;
+} QError;
+
+QError *qerror_new(void);
+QError *qerror_from_info(const char *file, int linenr, const char *fmt,
+                         va_list *va);
+void qerror_print(const QError *qerror);
+QError *qobject_to_qerror(const QObject *obj);
+
+/*
+ * QError class list
+ */
+#define QERR_DEVICE_NOT_FOUND \
+        "{ 'class': 'DeviceNotFound', 'data': { 'name': %s } }"
+
+#define QERR_SERVICE_UNAVAILABLE \
+        "{ 'class': 'ServiceUnavailable', 'data': { 'reason': %s } }"
+
+#endif /* QERROR_H */
diff --git a/qjson.c b/qjson.c
index 12e6cf0..60c904d 100644
--- a/qjson.c
+++ b/qjson.c
@@ -224,6 +224,8 @@ static void to_json(const QObject *obj, QString *str)
         }
         break;
     }
+    case QTYPE_QERROR:
+        /* XXX: should QError be emitted? */
     case QTYPE_NONE:
         break;
     }
diff --git a/qobject.h b/qobject.h
index 2270ec1..07de211 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.180.gc5b3e

[Qemu-devel] [PATCH 08/10] 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 handler returns.

Additionally, a new macro called qemu_error_new() is introduced.
It builds on top of the QemuErrorSink API and should be used in
place of qemu_error().

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 |   42 +++++++++++++++++++++++++++++++++++++++++-
 sysemu.h  |    6 ++++++
 2 files changed, 47 insertions(+), 1 deletions(-)

diff --git a/monitor.c b/monitor.c
index 3286ba2..74abef9 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;
 };
@@ -224,6 +226,11 @@ static inline int monitor_handler_ported(const mon_cmd_t *cmd)
     return cmd->user_print != NULL;
 }
 
+static inline int monitor_has_error(const Monitor *mon)
+{
+    return mon->error != NULL;
+}
+
 static void monitor_print_qobject(Monitor *mon, const QObject *data)
 {
     switch (qobject_type(data)) {
@@ -3168,6 +3175,13 @@ fail:
     return 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;
@@ -3193,7 +3207,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);
@@ -3644,3 +3661,26 @@ void qemu_error(const char *fmt, ...)
         break;
     }
 }
+
+void qemu_error_full(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(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 b1887ef..656f6a4 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,11 @@ 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 *file, int linenr, const char *fmt, ...)
+                     __attribute__ ((format(printf, 3, 4)));
+
+#define qemu_error_new(fmt, ...) \
+    qemu_error_full(__FILE__, __LINE__, fmt, ## __VA_ARGS__)
 
 #ifdef _WIN32
 /* Polling handling */
-- 
1.6.5.2.180.gc5b3e

[Qemu-devel] [PATCH 09/10] qdev: Use QError for 'device 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 d19d531..875ca50 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_new(QERR_DEVICE_NOT_FOUND, driver);
         return NULL;
     }
     if (info->no_user) {
-- 
1.6.5.2.180.gc5b3e

[Qemu-devel] [PATCH 10/10] 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 74abef9..e42434f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1722,10 +1722,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_SERVICE_UNAVAILABLE,
+                      "Using KVM without synchronous MMU, ballooning disabled");
     else if (actual == 0)
-        monitor_printf(mon, "Ballooning not activated in VM\n");
+        qemu_error_new(QERR_SERVICE_UNAVAILABLE,
+                       "Ballooning not activated in VM");
     else
         *ret_data = QOBJECT(qint_from_int((int)(actual >> 20)));
 }
-- 
1.6.5.2.180.gc5b3e

Re: [Qemu-devel] [PATCH 07/10] Introduce QError

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> QError is a high-level data type which represents an exception
> in QEMU, it stores the following error information:
>
> - class          Error class name (eg. "ServiceUnavailable")
> - description    A detailed error description, which can 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  Any run-time error data
>
> This commit adds the basic interface plus two error classes, one
> for 'device not found' errors and another one for 'service unavailable'
> errors.

I'd rather add error classes in the first commit using them.

> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  Makefile  |    2 +-
>  qerror.c  |  265 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qerror.h  |   46 +++++++++++
>  qjson.c   |    2 +
>  qobject.h |    1 +
>  5 files changed, 315 insertions(+), 1 deletions(-)
>  create mode 100644 qerror.c
>  create mode 100644 qerror.h
>
> diff --git a/Makefile b/Makefile
> index d770e2a..c0b65b6 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -138,7 +138,7 @@ 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 json-lexer.o
> -obj-y += json-streamer.o json-parser.o qjson.o
> +obj-y += json-streamer.o json-parser.o qjson.o qerror.o
>  obj-y += qemu-config.o block-migration.o
>  
>  obj-$(CONFIG_BRLAPI) += baum.o
> diff --git a/qerror.c b/qerror.c
> new file mode 100644
> index 0000000..beb215d
> --- /dev/null
> +++ b/qerror.c
> @@ -0,0 +1,265 @@
> +/*
> + * 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 LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + */
> +#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' parameter is a printf-like string, the format of the format
> + * string is:
> + *
> + * %(KEY)
> + *
> + * Where KEY is a QDict key, which has to be passed to qerror_from_info().
> + *
> + * Example:
> + *
> + * "foo error on device: %(device) slot: %(slot_nr)"
> + *
> + * A single percent sign can be printed if followed by a second one,
> + * for example:
> + *
> + * "running out of foo: %(foo)%%"
> + */
> +const QErrorStringTable qerror_table[] = {
> +    {
> +        .error_fmt   = QERR_DEVICE_NOT_FOUND,
> +        .desc        = "device \"%(name)\" not found",
> +    },
> +    {
> +        .error_fmt   = QERR_SERVICE_UNAVAILABLE,
> +        .desc        = "%(reason)",
> +    },
> +    {}
> +};
> +
> +/**
> + * 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;
> +}
> +
> +static void qerror_abort(const QError *qerr, const char *fmt, ...)
> +{
> +    va_list ap;
> +
> +    fprintf(stderr, "qerror: ");
> +
> +    va_start(ap, fmt);
> +    vfprintf(stderr, fmt, ap);
> +    va_end(ap);
> +
> +    fprintf(stderr, " - call at %s:%d\n", qerr->file, qerr->linenr);
> +    abort();
> +}
> +
> +static void qerror_set_data(QError *qerr, const char *fmt, va_list *va)
> +{
> +    QObject *obj;
> +
> +    obj = qobject_from_jsonv(fmt, va);
> +    if (!obj) {
> +        qerror_abort(qerr, "invalid format '%s'", fmt);
> +    }
> +    if (qobject_type(obj) != QTYPE_QDICT) {
> +        qerror_abort(qerr, "error format is not a QDict '%s'", fmt);
> +    }
> +
> +    qerr->error = qobject_to_qdict(obj);
> +
> +    obj = qdict_get(qerr->error, "class");
> +    if (!obj) {
> +        qerror_abort(qerr, "missing 'class' key in '%s'", fmt);
> +    }
> +    if (qobject_type(obj) != QTYPE_QSTRING) {
> +        qerror_abort(qerr, "'class' key value should be a QString");
> +    }
> +    
> +    obj = qdict_get(qerr->error, "data");
> +    if (!obj) {
> +        qerror_abort(qerr, "missing 'data' key in '%s'", fmt);
> +    }
> +    if (qobject_type(obj) != QTYPE_QDICT) {
> +        qerror_abort(qerr, "'data' key value should be a QDICT");
> +    }
> +}
> +
> +static void qerror_set_desc(const char *fmt, QError *qerr)
> +{
> +    int i;
> +
> +    // FIXME: inefficient loop
> +
> +    for (i = 0; qerror_table[i].error_fmt; i++) {
> +        if (strcmp(qerror_table[i].error_fmt, fmt) == 0) {
> +            qerr->entry = &qerror_table[i];
> +            return;
> +        }
> +    }
> +
> +    qerror_abort(qerr, "error format '%s' not found", fmt);
> +}
> +
> +/**
> + * qerror_from_info(): Create a new QError from error information
> + *
> + * The information consists of:
> + *
> + * - file   the file name of where the error occurred
> + * - linenr the line number of where the error occurred
> + * - fmt    JSON printf-like dictionary, there must exist keys 'class' and
> + *          'data'
> + * - va     va_list of all arguments specified by fmt
> + *
> + * Return strong reference.
> + */
> +QError *qerror_from_info(const char *file, int linenr, const char *fmt,
> +                         va_list *va)
> +{
> +    QError *qerr;
> +
> +    qerr = qerror_new();
> +    qerr->linenr = linenr;
> +    qerr->file = file;
> +
> +    if (!fmt) {
> +        qerror_abort(qerr, "QDict not specified");
> +    }
> +
> +    qerror_set_data(qerr, fmt, va);
> +    qerror_set_desc(fmt, qerr);

Recommend to have both functions take qerr, fmt in the same order.
Since they both update qerr, I'd put qerr on the left.

> +
> +    return qerr;
> +}
> +
> +static void parse_error(const QError *qerror, int c)
> +{
> +    qerror_abort(qerror, "expected '%c' in '%s'", c, qerror->entry->desc);
> +}
> +
> +static const char *append_field(QString *outstr, const QError *qerror,
> +                                const char *start)
> +{
> +    QObject *obj;
> +    QDict *qdict;
> +    QString *key_qs;
> +    const char *end, *key;
> +
> +    if (*start != '%')
> +        parse_error(qerror, '%');

Can't happen, because it gets called only with *start == '%'.  Taking
pointer to the character following the '%' as argument would sidestep
the issue.  But I'm fine with leaving it as is.

> +    start++;
> +    if (*start != '(')
> +        parse_error(qerror, '(');
> +    start++;
> +
> +    end = strchr(start, ')');
> +    if (!end)
> +        parse_error(qerror, ')');
> +
> +    key_qs = qstring_from_substr(start, 0, end - start - 1);
> +    key = qstring_get_str(key_qs);
> +
> +    qdict = qobject_to_qdict(qdict_get(qerror->error, "data"));
> +    obj = qdict_get(qdict, key);
> +    if (!obj) {
> +        qerror_abort(qerror, "key '%s' not found in QDict", key);
> +    }
> +
> +    switch (qobject_type(obj)) {
> +        case QTYPE_QSTRING:
> +            qstring_append(outstr, qdict_get_str(qdict, key));
> +            break;
> +        case QTYPE_QINT:
> +            qstring_append_int(outstr, qdict_get_int(qdict, key));
> +            break;
> +        default:
> +            qerror_abort(qerror, "invalid type '%c'", qobject_type(obj));
> +    }
> +
> +    QDECREF(key_qs);

Looks like you create key_qs just because it's a convenient way to
extract key zero-terminated.  Correct?

> +    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).

s/ ou / or /

> + */
> +void qerror_print(const QError *qerror)
> +{
> +    const char *p;
> +    QString *qstring;
> +
> +    assert(qerror->entry != NULL);
> +
> +    qstring = qstring_new();
> +
> +    for (p = qerror->entry->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);
> +
> +    QDECREF(qerr->error);
> +    qemu_free(qerr);
> +}
> diff --git a/qerror.h b/qerror.h
> new file mode 100644
> index 0000000..d262863
> --- /dev/null
> +++ b/qerror.h
> @@ -0,0 +1,46 @@
> +/*
> + * 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 LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + */
> +#ifndef QERROR_H
> +#define QERROR_H
> +
> +#include "qdict.h"
> +#include <stdarg.h>
> +
> +typedef struct QErrorStringTable {
> +    const char *desc;
> +    const char *error_fmt;
> +} QErrorStringTable;
> +
> +typedef struct QError {
> +    QObject_HEAD;
> +    QDict *error;
> +    int linenr;
> +    const char *file;
> +    const QErrorStringTable *entry;
> +} QError;
> +
> +QError *qerror_new(void);
> +QError *qerror_from_info(const char *file, int linenr, const char *fmt,
> +                         va_list *va);
> +void qerror_print(const QError *qerror);
> +QError *qobject_to_qerror(const QObject *obj);
> +
> +/*
> + * QError class list
> + */
> +#define QERR_DEVICE_NOT_FOUND \
> +        "{ 'class': 'DeviceNotFound', 'data': { 'name': %s } }"
> +
> +#define QERR_SERVICE_UNAVAILABLE \
> +        "{ 'class': 'ServiceUnavailable', 'data': { 'reason': %s } }"
> +
> +#endif /* QERROR_H */
> diff --git a/qjson.c b/qjson.c
> index 12e6cf0..60c904d 100644
> --- a/qjson.c
> +++ b/qjson.c
> @@ -224,6 +224,8 @@ static void to_json(const QObject *obj, QString *str)
>          }
>          break;
>      }
> +    case QTYPE_QERROR:
> +        /* XXX: should QError be emitted? */

Pros & cons?

>      case QTYPE_NONE:
>          break;
>      }
> diff --git a/qobject.h b/qobject.h
> index 2270ec1..07de211 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;

Erroneous QERRs are detected only when they're passed to
qerror_from_info() at run-time, i.e. when the error happens.  Likewise
for erroneous qerror_table[].desc.  Perhaps a unit test to ensure
qerror_table[] is sane would make sense.  Can't protect from passing
unknown errors to qerror_from_info(), but that shouldn't be a problem in
practice.

Re: [Qemu-devel] [PATCH 08/10] monitor: QError support

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> 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 handler returns.
>
> Additionally, a new macro called qemu_error_new() is introduced.
> It builds on top of the QemuErrorSink API and should be used in
> place of qemu_error().
>
> 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
[...]
> diff --git a/sysemu.h b/sysemu.h
> index b1887ef..656f6a4 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,11 @@ 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 *file, int linenr, const char *fmt, ...)
> +                     __attribute__ ((format(printf, 3, 4)));
> +
> +#define qemu_error_new(fmt, ...) \
> +    qemu_error_full(__FILE__, __LINE__, fmt, ## __VA_ARGS__)
>  
>  #ifdef _WIN32
>  /* Polling handling */

The relationship between qemu_error_new() and qemu_error_full() is not
obvious from their names.  What about calling the latter
qemu_error_new_internal()?

Re: [Qemu-devel] [PATCH 09/10] qdev: Use QError for 'device not found' error

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> 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 d19d531..875ca50 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_new(QERR_DEVICE_NOT_FOUND, driver);
>          return NULL;
>      }
>      if (info->no_user) {

Not obvious from this patch, but we lose the "Try -device '?' for a
list" hint here.  In PATCH 7/10:

+#define QERR_DEVICE_NOT_FOUND \
+        "{ 'class': 'DeviceNotFound', 'data': { 'name': %s } }"
+

and

+    {
+        .error_fmt   = QERR_DEVICE_NOT_FOUND,
+        .desc        = "device \"%(name)\" not found",
+    },

Not a deal-breaker for me.

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

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> 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 74abef9..e42434f 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1722,10 +1722,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_SERVICE_UNAVAILABLE,
> +                      "Using KVM without synchronous MMU, ballooning disabled");
>      else if (actual == 0)
> -        monitor_printf(mon, "Ballooning not activated in VM\n");
> +        qemu_error_new(QERR_SERVICE_UNAVAILABLE,
> +                       "Ballooning not activated in VM");
>      else
>          *ret_data = QOBJECT(qint_from_int((int)(actual >> 20)));
>  }

In PATCH 7/10:

+#define QERR_SERVICE_UNAVAILABLE \
+        "{ 'class': 'ServiceUnavailable', 'data': { 'reason': %s } }"
+

and

+    {
+        .error_fmt   = QERR_SERVICE_UNAVAILABLE,
+        .desc        = "%(reason)",
+    },

How to do a ServiceUnavailable error with a description that is not a
compile time literal?  Add another macro and error table entry for that?

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

[Anthony Liguori]

Markus Armbruster wrote:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
>
>   
>> 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 74abef9..e42434f 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -1722,10 +1722,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_SERVICE_UNAVAILABLE,
>> +                      "Using KVM without synchronous MMU, ballooning disabled");
>>      else if (actual == 0)
>> -        monitor_printf(mon, "Ballooning not activated in VM\n");
>> +        qemu_error_new(QERR_SERVICE_UNAVAILABLE,
>> +                       "Ballooning not activated in VM");
>>      else
>>          *ret_data = QOBJECT(qint_from_int((int)(actual >> 20)));
>>  }
>>     
>
> In PATCH 7/10:
>
> +#define QERR_SERVICE_UNAVAILABLE \
> +        "{ 'class': 'ServiceUnavailable', 'data': { 'reason': %s } }"
> +
>
> and
>
> +    {
> +        .error_fmt   = QERR_SERVICE_UNAVAILABLE,
> +        .desc        = "%(reason)",
> +    },
>
> How to do a ServiceUnavailable error with a description that is not a
> compile time literal?  Add another macro and error table entry for that?
>   

An error that just contains reason is a good indication that the error 
is not the right level of abstraction.

There are two error conditions here.  One is that kvm is in use, but 
it's missing a capability and therefore we have to disable a feature.   
The second error is that the guest did not activate a device.

So the first error should be something like

#define QERR_KVM_MISSING_CAP \
              "{'class': 'KVMMissingCap', 'data' : { 'capability': %s, 
'feature': %s' }"

Where capability is the string form of the missing cap and feature 
describes the feature being disabled because of the missing cap.

The error format would be "Using KVM without %{capability}, %{feature} 
unavailable".  It would get raised with

qemu_error_new(QERR_KVM_MISSING_CAP, kvm_get_cap_desc(KVM_CAP_SYNC_MMU), 
"balloon");

The error text is slightly modified, but that's okay.

Likewise, the second error should be something like

#define QERR_DEVICE_NOT_ACTIVE \
          "{'class': 'DeviceNotActive', 'data' : { 'device': %s }"

qemu_error_new(QERR_DEVICE_NOT_ACTIVE, "balloon");

With a description of "The %{device} device has not been activated by 
the guest"

If I was writing a UI, I would respond very differently to these two 
errors.  For the first error, I would grey out the balloon feature 
because it's not possible to use ballooning with this very of KVM.  For 
the second error, I would prompt the user when they tried to do 
ballooning to make sure the driver was loaded in the guest.

This behavior can not be achieved the ServiceNotAvailable.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Markus Armbruster]

I'm sorry, but I'm still quite unhappy with the error reporting part of
QMP.

In short:

1. QError feels overengineered, particularly for a first iteration of a
   protocol.

   We go to great lengths to build highly structured error objects.
   There is only one sane reason for doing that: a demonstrated client
   need.  I can't see that need.

2. It falls short of the requirement that clients can reasonably
   classify errors they don't know.

3. It falls short of the requirement that clients can easily present a
   human-readable error description to their human users, regardless of
   whether they know the error or not.


In more detail[*]:

There are more than one way to screw up here.  One is certainly to fall
short of client requirements in a way that is costly to fix (think
incompatible protocol revision).  Another is to overshoot them in a way
that is costly to maintain.  A third one is to spend too much time on
figuring out the perfect solution.

I believe our true problem is that we're still confused and/or
disagreeing on client requirements, and this has led to a design that
tries to cover all the conceivable bases, and feels overengineered to
me.

There are only so many complexity credits you can spend in a program,
both globally and individually.  I'm very, very wary of making error
reporting more complex than absolutely, desperately necessary.
Reporting errors should remain as easy as we can make it.  The more
cumbersome it is to report an error, the less it is done, and the more
vaguely it is done.  If you have to edit more than the error site to
report an error accurately, then chances skyrocket that it won't be
reported, or it'll be reported inaccurately.  And not because coders are
stupid or lazy, but because they make sensible use of their very limited
complexity credits: if you can either get the job done with lousy error
messages, or not get it done at all, guess what the smart choice is.

So, before we accept the cost of highly structured error objects, I'd
like to see a convincing argument for their need.  And actual client
developers like Dan are in a much better position to make such an
argument than server developers (like me) speculating about client
needs.

If I understand Dan correctly, machine-readable error code +
human-readable description is just fine, as long as the error code is
reasonably specific and the description is precise and complete.  Have
we heard anything else from client developers?

I'm not a client developer, but let me make a few propositions on client
needs anyway:

* Clients are just as complexity-bound as the server.  They prefer their
  errors as simple as possible, but no simpler.

* The crucial question for the client isn't "what exactly went wrong".
  It is "how should I handle this error".  Answering that question
  should be easy (say, check the error code).  Figuring out what went
  wrong should still be possible for a human operator of the client.

* Clients don't want to be tightly coupled to the server.

* No matter how smart and up-to-date the client is, there will always be
  errors it doesn't know.  And it needs to answer the "how to handle"
  question whether it knows the error code or not!  That's why protocols
  like HTTP have simple rules to classify error codes.

  Likewise, it needs to be able to give a human operator enough
  information to figure out what went wrong whether it knows the error
  or not.  How do you expect clients to format a structured error object
  for an error they don't know into human-readable text?  Isn't it much
  easier and more robust to cut out the formatting middle-man and send
  the text along with the error?

* Clients need to present a human-readable error description to their
  human users, regardless of whether they know the error or not.

There's a general rule of programming that I've found quite hard to
learn and quite painful to disobey: always try the stupidest solution
that could possibly work first.

Based on what I've learned about client requirements so far, I figure
that solution is "easily classified error code + human-readable
description".

If we later realize that this solution was *too* stupid, we can simply
add a data member to the error object.



[*] Mostly warmed up Message-ID: <m3k4yrfrw4.fsf@crossbow.pond.sub.org>

Re: [Qemu-devel] [PATCH 07/10] Introduce QError

[Luiz Capitulino]

On Wed, 18 Nov 2009 16:16:11 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:

[...]

> > +static const char *append_field(QString *outstr, const QError *qerror,
> > +                                const char *start)
> > +{
> > +    QObject *obj;
> > +    QDict *qdict;
> > +    QString *key_qs;
> > +    const char *end, *key;
> > +
> > +    if (*start != '%')
> > +        parse_error(qerror, '%');
> 
> Can't happen, because it gets called only with *start == '%'.  Taking
> pointer to the character following the '%' as argument would sidestep
> the issue.  But I'm fine with leaving it as is.

 It's just an assertion.

> > +    start++;
> > +    if (*start != '(')
> > +        parse_error(qerror, '(');
> > +    start++;
> > +
> > +    end = strchr(start, ')');
> > +    if (!end)
> > +        parse_error(qerror, ')');
> > +
> > +    key_qs = qstring_from_substr(start, 0, end - start - 1);
> > +    key = qstring_get_str(key_qs);
> > +
> > +    qdict = qobject_to_qdict(qdict_get(qerror->error, "data"));
> > +    obj = qdict_get(qdict, key);
> > +    if (!obj) {
> > +        qerror_abort(qerror, "key '%s' not found in QDict", key);
> > +    }
> > +
> > +    switch (qobject_type(obj)) {
> > +        case QTYPE_QSTRING:
> > +            qstring_append(outstr, qdict_get_str(qdict, key));
> > +            break;
> > +        case QTYPE_QINT:
> > +            qstring_append_int(outstr, qdict_get_int(qdict, key));
> > +            break;
> > +        default:
> > +            qerror_abort(qerror, "invalid type '%c'", qobject_type(obj));
> > +    }
> > +
> > +    QDECREF(key_qs);
> 
> Looks like you create key_qs just because it's a convenient way to
> extract key zero-terminated.  Correct?

 Yes, as a substring of 'desc', which is passed through 'start'.

[...]

> > diff --git a/qjson.c b/qjson.c
> > index 12e6cf0..60c904d 100644
> > --- a/qjson.c
> > +++ b/qjson.c
> > @@ -224,6 +224,8 @@ static void to_json(const QObject *obj, QString *str)
> >          }
> >          break;
> >      }
> > +    case QTYPE_QERROR:
> > +        /* XXX: should QError be emitted? */
> 
> Pros & cons?

 It's probably convenient to have qjson emitting QError, I'm unsure
if we should do that for all kinds of QObjects though.

> >      case QTYPE_NONE:
> >          break;
> >      }
> > diff --git a/qobject.h b/qobject.h
> > index 2270ec1..07de211 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;
> 
> Erroneous QERRs are detected only when they're passed to
> qerror_from_info() at run-time, i.e. when the error happens.  Likewise
> for erroneous qerror_table[].desc.  Perhaps a unit test to ensure
> qerror_table[] is sane would make sense.  Can't protect from passing
> unknown errors to qerror_from_info(), but that shouldn't be a problem in
> practice.

 We could also have a debug function that could run once at startup
and do the check.

Re: [Qemu-devel] [PATCH 08/10] monitor: QError support

[Luiz Capitulino]

On Wed, 18 Nov 2009 16:16:40 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > 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 handler returns.
> >
> > Additionally, a new macro called qemu_error_new() is introduced.
> > It builds on top of the QemuErrorSink API and should be used in
> > place of qemu_error().
> >
> > 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
> [...]
> > diff --git a/sysemu.h b/sysemu.h
> > index b1887ef..656f6a4 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,11 @@ 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 *file, int linenr, const char *fmt, ...)
> > +                     __attribute__ ((format(printf, 3, 4)));
> > +
> > +#define qemu_error_new(fmt, ...) \
> > +    qemu_error_full(__FILE__, __LINE__, fmt, ## __VA_ARGS__)
> >  
> >  #ifdef _WIN32
> >  /* Polling handling */
> 
> The relationship between qemu_error_new() and qemu_error_full() is not
> obvious from their names.  What about calling the latter
> qemu_error_new_internal()?

 Okay, as I'm going to post a new version anyway.

Re: [Qemu-devel] [PATCH 09/10] qdev: Use QError for 'device not found' error

[Luiz Capitulino]

On Wed, 18 Nov 2009 16:17:02 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > 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 d19d531..875ca50 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_new(QERR_DEVICE_NOT_FOUND, driver);
> >          return NULL;
> >      }
> >      if (info->no_user) {
> 
> Not obvious from this patch, but we lose the "Try -device '?' for a
> list" hint here. 

 Yes, this happens because this is a generic error and '-device' is
qdev command-line specific.

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Anthony Liguori]

Markus Armbruster wrote:
> 1. QError feels overengineered, particularly for a first iteration of a
>    protocol.
>
>    We go to great lengths to build highly structured error objects.
>    There is only one sane reason for doing that: a demonstrated client
>    need.  I can't see that need.
>   

A GUI wants to present the user a mechanism to reduce the memory 
consumption of a VM.  There are three reasons why this would not work.  
The first is that ballooning was not configured for this guest.  The 
second is that ballooning was configured but the version of the KVM 
kernel module does not have a sync mmu.  The third is that an 
appropriate guest driver has not been loaded for the device.

If we don't specify a type of error, all the GUI can do is fail right 
before trying to balloon the guest.  That offers the user no insight why 
it failed other than popping up a message box with an error message 
(that may not even be in their locale).  For a non-english speaker, it's 
gibberish.

If you use three types of errors, then a GUI can differentiate these 
cases and presents the user different feedback.  For the first case, it 
can simply gray out the menu item.  For the second can, it can also gray 
out the menu item but it can provide a one time indication (in say, a 
status bar), that ballooning is unavailable because the kernel version 
does not support it.  For the third error, you would want to prompt the 
user to indicate the driver is not loaded in the guest.

It's highly likely that for this last case, you'd want generic code to 
generate this error.  Further more, in order to generate the error 
message for a user, you need to know what device does not have a 
functioning driver.  You may say it's obvious for something like info 
balloon but it's not so obvious if you assume we support things other 
than just virtio-pci-balloon.  For instance, with s390, it's a different 
balloon driver.  For xenner/xenpv, it's a different driver.  It really 
suggests we should send the qdev device name for the current balloon driver.

> 2. It falls short of the requirement that clients can reasonably
>    classify errors they don't know.
>   

I think this is wishful thinking.  I strongly suspect that you can't 
find a reasonably popular web browser that doesn't know all of the error 
codes that web servers generate.

We should document all of the errors that each QMP function can return.

That said, if you thought there was 4-5 common classes of errors, adding 
an additional 'base_class' field could be a completely reasonable thing 
to do.  As long as you're adding information vs. taking it away, I'm 
quite happy.

> 3. It falls short of the requirement that clients can easily present a
>    human-readable error description to their human users, regardless of
>    whether they know the error or not.
>   

That's just incorrect.  We provide an example conversion table that's 
akin to strerror() or a __repr__ for an exception in Python.

> In more detail[*]:
>
> There are more than one way to screw up here.  One is certainly to fall
> short of client requirements in a way that is costly to fix (think
> incompatible protocol revision).  Another is to overshoot them in a way
> that is costly to maintain.  A third one is to spend too much time on
> figuring out the perfect solution.
>
> I believe our true problem is that we're still confused and/or
> disagreeing on client requirements, and this has led to a design that
> tries to cover all the conceivable bases, and feels overengineered to
> me.
>   

I've described my requirements for what a client can do.  I'd like to 
understand how you disagree.

> There are only so many complexity credits you can spend in a program,
> both globally and individually.  I'm very, very wary of making error
> reporting more complex than absolutely, desperately necessary.
>   

We're following a very well established model of error reporting 
(object-based exceptions).  This is well established and well understood.

> Reporting errors should remain as easy as we can make it.  The more
> cumbersome it is to report an error, the less it is done, and the more
> vaguely it is done.  If you have to edit more than the error site to
> report an error accurately, then chances skyrocket that it won't be
> reported, or it'll be reported inaccurately. 

I don't buy these sort of arguments.  We are not designing a library 
that is open to abuse.  If our developers are fall victim to this, then 
we need to retrain them to be less lazy by not accepting patches that do 
a poor job reporting errors.

>  And not because coders are
> stupid or lazy, but because they make sensible use of their very limited
> complexity credits: if you can either get the job done with lousy error
> messages, or not get it done at all, guess what the smart choice is.
>   

Error reporting is extremely important in a management scenario.  You 
are severely limited by what actions you can take based on the amount of 
error information returned.  This is particularly when dealing with a 
multi-client management mechanism.  IMHO, this is a sensible place to 
spend complexity credits.

> If I understand Dan correctly, machine-readable error code +
> human-readable description is just fine, as long as the error code is
> reasonably specific and the description is precise and complete.  Have
> we heard anything else from client developers?
>   

There are no client developers because QMP doesn't exist.  Historically, 
we haven't provided high quality error messages so of course libvirt 
makes due without them today.

But I can answer on behalf of the management work we've done based on 
libvirt.

If you attempt to do a virDomainSetMemory() to a qemu domain, you only 
get an error if you're using a version of qemu that doesn't support the 
balloon command.  You do not get an error at all for the case where the 
kernel module doesn't support ballooning or a guest driver isn't loaded.

The concrete use case here is building an autoballoon mechanism part of 
a larger management suite.  If you're in an environment where memory 
overcommit is important, then you really do want to know whether 
ballooning isn't functioning because appropriate guest drivers aren't 
loaded.

> I'm not a client developer, but let me make a few propositions on client
> needs anyway:
>
> * Clients are just as complexity-bound as the server.  They prefer their
>   errors as simple as possible, but no simpler.
>   

I agree but the problem is that every client has a different definition 
of "simple as possible".  My client doesn't necessarily care about PCI 
hotplug error messages because it's not a function that we use.   
However, ballooning is tremendously important and I want to know 
everything I can about why it failed.

> * The crucial question for the client isn't "what exactly went wrong".
>   It is "how should I handle this error".  Answering that question
>   should be easy (say, check the error code).  Figuring out what went
>   wrong should still be possible for a human operator of the client.
>   

I disagree.  The client needs to know what went wrong if they are going 
to take a corrective action.

> * Clients don't want to be tightly coupled to the server.
>   

I don't follow this.

> * No matter how smart and up-to-date the client is, there will always be
>   errors it doesn't know.  And it needs to answer the "how to handle"
>   question whether it knows the error code or not!  That's why protocols
>   like HTTP have simple rules to classify error codes.
>   

HTTP is the exception and the HTTP error classes really leave an awful 
lot to be desired.  If you feel it's important to add error classes, I'd 
be happy to consider those patches.

>   Likewise, it needs to be able to give a human operator enough
>   information to figure out what went wrong whether it knows the error
>   or not.  How do you expect clients to format a structured error object
>   for an error they don't know into human-readable text?  Isn't it much
>   easier and more robust to cut out the formatting middle-man and send
>   the text along with the error?
>   

We do that by providing a conversion table.  The fundamental problem 
here is localization.

> * Clients need to present a human-readable error description to their
>   human users, regardless of whether they know the error or not.
>   

I would never pass an error string from a third party directly to a 
user.  I doubt you'll find a lot of good applications that do.  From a 
usability perspective, you need to be in control of your interactions 
with the user.  They grammar needs to be consistent and it has to be 
localized.  The best you would do with a third party string is log it to 
some log file for later examination by support.  In that case, dumping 
the class code and the supporting information in JSON form is just as 
good as a text description.

> There's a general rule of programming that I've found quite hard to
> learn and quite painful to disobey: always try the stupidest solution
> that could possibly work first.
>
> Based on what I've learned about client requirements so far, I figure
> that solution is "easily classified error code + human-readable
> description".
>   

I appreciate you feel strongly that this is the right thing to do.  That 
said, I disagree and I have specific use cases in mind that are not 
satisfied by the above mechanism.  In a situation like this, I tend to 
think the best thing to do is provide more information such that 
everyone can get the function they want.

If we add a base_class member to the error structure and we have the 
table for formatting error messages, then that should satisfy your 
requirements.  If you're concerned about a client having to have an 
up-to-date version of the table, we can introduce a monitor command to 
pretty print a json error object which would address that concern.

Then I think your argument boils down to, you think the remaining 
infrastructure is unnecessary for what you anticipate the uses to be.  
But hopefully, you'll at least concede that there are valid use cases 
beyond what you anticipate to be the normal case that do need this 
information.

> If we later realize that this solution was *too* stupid, we can simply
> add a data member to the error object.
>   

It's never that easy because a management tool has to support a least 
common denominator.

-- 
Regards,

Anthony Liguori

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

[Luiz Capitulino]

On Wed, 18 Nov 2009 09:58:30 -0600
Anthony Liguori <aliguori@linux.vnet.ibm.com> wrote:

> Markus Armbruster wrote:
> > Luiz Capitulino <lcapitulino@redhat.com> writes:
> >
> >   
> >> 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 74abef9..e42434f 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -1722,10 +1722,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_SERVICE_UNAVAILABLE,
> >> +                      "Using KVM without synchronous MMU, ballooning disabled");
> >>      else if (actual == 0)
> >> -        monitor_printf(mon, "Ballooning not activated in VM\n");
> >> +        qemu_error_new(QERR_SERVICE_UNAVAILABLE,
> >> +                       "Ballooning not activated in VM");
> >>      else
> >>          *ret_data = QOBJECT(qint_from_int((int)(actual >> 20)));
> >>  }
> >>     
> >
> > In PATCH 7/10:
> >
> > +#define QERR_SERVICE_UNAVAILABLE \
> > +        "{ 'class': 'ServiceUnavailable', 'data': { 'reason': %s } }"
> > +
> >
> > and
> >
> > +    {
> > +        .error_fmt   = QERR_SERVICE_UNAVAILABLE,
> > +        .desc        = "%(reason)",
> > +    },
> >
> > How to do a ServiceUnavailable error with a description that is not a
> > compile time literal?  Add another macro and error table entry for that?
> >   
> 
> An error that just contains reason is a good indication that the error 
> is not the right level of abstraction.
> 
> There are two error conditions here.  One is that kvm is in use, but 
> it's missing a capability and therefore we have to disable a feature.   
> The second error is that the guest did not activate a device.

 Thanks for the detailed explanation, I've included the new errors.

[Qemu-devel] Re: [PATCH 00/10]: QError v4

[Paolo Bonzini]

> 2. It falls short of the requirement that clients can reasonably
>     classify errors they don't know.

True.  (Though adding a classification mechanism can be done later once 
we have an idea of what errors are there at all).

> 3. It falls short of the requirement that clients can easily present a
>     human-readable error description to their human users, regardless of
>     whether they know the error or not.

That's true.  I would definitely have the interpolated desc field go on 
the wire too, as part of the JSON form of QError.

> If I understand Dan correctly, machine-readable error code +
> human-readable description is just fine, as long as the error code is
> reasonably specific and the description is precise and complete.  Have
> we heard anything else from client developers?

Then (except for the asynchronicity) the current qemu monitor protocol 
is also just fine, including the fact that we send migrate and get 
m\rmi\rmig\rmigr etc. back on the socket.

"error while creating snapshot on '%s'" may be easy to parse, but 
looking at a dictionary field in { "filename" : "blah.img" } is easier, 
and at the same time solves issues with escaping weird characters in the 
file name.

> * The crucial question for the client isn't "what exactly went wrong".
>    It is "how should I handle this error".  Answering that question
>    should be easy (say, check the error code).  Figuring out what went
>    wrong should still be possible for a human operator of the client.

The same error can be handled in a different way depending on the 
situation.  A missing image is in general a fatal error, but maybe 
another client could create images on the fly.

> Based on what I've learned about client requirements so far, I figure
> that solution is "easily classified error code + human-readable
> description".

How would you classify the error code?  By subsystem or by (for example) 
severity or anything else?  Does QEMU have subsystems that are 
well-identified enough to be carved in stone in QError?  (All genuine 
questions.  I have no idea).

Paolo

Re: [Qemu-devel] [PATCH 07/10] Introduce QError

[Daniel P. Berrange]

On Tue, Nov 17, 2009 at 05:43:54PM -0200, Luiz Capitulino wrote:
> QError is a high-level data type which represents an exception
> in QEMU, it stores the following error information:
> 
> - class          Error class name (eg. "ServiceUnavailable")
> - description    A detailed error description, which can 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

If we're going to collect these two, then also add in the function
name, since that's typically more useful than filename/line number
alone.


Regards,
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] [PATCH 08/10] monitor: QError support

[Daniel P. Berrange]

On Tue, Nov 17, 2009 at 05:43:55PM -0200, Luiz Capitulino wrote:
> 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 handler returns.
> 
> Additionally, a new macro called qemu_error_new() is introduced.
> It builds on top of the QemuErrorSink API and should be used in
> place of qemu_error().
> 
> When all conversion to qemu_error_new() is done, qemu_error() can
> be turned private.

> diff --git a/sysemu.h b/sysemu.h
> index b1887ef..656f6a4 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,11 @@ 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 *file, int linenr, const char *fmt, ...)
> +                     __attribute__ ((format(printf, 3, 4)));

Add in a 3rd param, 'const char *func'

> +
> +#define qemu_error_new(fmt, ...) \
> +    qemu_error_full(__FILE__, __LINE__, fmt, ## __VA_ARGS__)

And use  __func__  here

Regards,
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] [PATCH 07/10] Introduce QError

[Anthony Liguori]

Daniel P. Berrange wrote:
> On Tue, Nov 17, 2009 at 05:43:54PM -0200, Luiz Capitulino wrote:
>   
>> QError is a high-level data type which represents an exception
>> in QEMU, it stores the following error information:
>>
>> - class          Error class name (eg. "ServiceUnavailable")
>> - description    A detailed error description, which can 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
>>     
>
> If we're going to collect these two, then also add in the function
> name, since that's typically more useful than filename/line number
> alone.
>   

I'm not convinced it's a good idea to put that info on the wire.  It's 
unstable across any build of qemu.  However, since it's extra info, it 
doesn't bother me that much if people think it's useful for debugging 
purposes.

> Regards,
> Daniel
>   


-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 07/10] Introduce QError

[Luiz Capitulino]

On Wed, 18 Nov 2009 13:58:17 -0600
Anthony Liguori <aliguori@linux.vnet.ibm.com> wrote:

> Daniel P. Berrange wrote:
> > On Tue, Nov 17, 2009 at 05:43:54PM -0200, Luiz Capitulino wrote:
> >   
> >> QError is a high-level data type which represents an exception
> >> in QEMU, it stores the following error information:
> >>
> >> - class          Error class name (eg. "ServiceUnavailable")
> >> - description    A detailed error description, which can 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
> >>     
> >
> > If we're going to collect these two, then also add in the function
> > name, since that's typically more useful than filename/line number
> > alone.
> >   
> 
> I'm not convinced it's a good idea to put that info on the wire.  It's 
> unstable across any build of qemu.  However, since it's extra info, it 
> doesn't bother me that much if people think it's useful for debugging 
> purposes.

 It's really for debugging, so that we can have a detailed error
description when the error macro has a wrong syntax.

 That said, we could have a compile time switch to activate extra
debugging information on the wire. But that's a brainstorm.

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Jamie Lokier]

Anthony Liguori wrote:
> Markus Armbruster wrote:
> >3. It falls short of the requirement that clients can easily present a
> >   human-readable error description to their human users, regardless of
> >   whether they know the error or not.
> >  
> 
> That's just incorrect.  We provide an example conversion table that's 
> akin to strerror() or a __repr__ for an exception in Python.

Markus refers to errors that the client does not know - i.e. when the
client is older than qemu, or is not in the same development branch if
it's a branched qemu.  Which means the client won't have a fully up to
date conversion table.

Do you mean qemu provides it's current conversion table to the client
over the wire protocol?

-- Jamie

Re: [Qemu-devel] [PATCH 07/10] Introduce QError

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 18 Nov 2009 16:16:11 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>
> [...]
>
>> > +static const char *append_field(QString *outstr, const QError *qerror,
>> > +                                const char *start)
>> > +{
>> > +    QObject *obj;
>> > +    QDict *qdict;
>> > +    QString *key_qs;
>> > +    const char *end, *key;
>> > +
>> > +    if (*start != '%')
>> > +        parse_error(qerror, '%');
>> 
>> Can't happen, because it gets called only with *start == '%'.  Taking
>> pointer to the character following the '%' as argument would sidestep
>> the issue.  But I'm fine with leaving it as is.
>
>  It's just an assertion.

It's not coded as an assertion.  If we ever do coverage testing, it'll
stick out.  But again, I'm fine with it.

>> > +    start++;
>> > +    if (*start != '(')
>> > +        parse_error(qerror, '(');
>> > +    start++;
>> > +
>> > +    end = strchr(start, ')');
>> > +    if (!end)
>> > +        parse_error(qerror, ')');
>> > +
>> > +    key_qs = qstring_from_substr(start, 0, end - start - 1);
>> > +    key = qstring_get_str(key_qs);
>> > +
>> > +    qdict = qobject_to_qdict(qdict_get(qerror->error, "data"));
>> > +    obj = qdict_get(qdict, key);
>> > +    if (!obj) {
>> > +        qerror_abort(qerror, "key '%s' not found in QDict", key);
>> > +    }
>> > +
>> > +    switch (qobject_type(obj)) {
>> > +        case QTYPE_QSTRING:
>> > +            qstring_append(outstr, qdict_get_str(qdict, key));
>> > +            break;
>> > +        case QTYPE_QINT:
>> > +            qstring_append_int(outstr, qdict_get_int(qdict, key));
>> > +            break;
>> > +        default:
>> > +            qerror_abort(qerror, "invalid type '%c'", qobject_type(obj));
>> > +    }
>> > +
>> > +    QDECREF(key_qs);
>> 
>> Looks like you create key_qs just because it's a convenient way to
>> extract key zero-terminated.  Correct?
>
>  Yes, as a substring of 'desc', which is passed through 'start'.

Funny that the convenient way to extract a substring is to go through
QString.  Fine with me.

> [...]
>
>> > diff --git a/qjson.c b/qjson.c
>> > index 12e6cf0..60c904d 100644
>> > --- a/qjson.c
>> > +++ b/qjson.c
>> > @@ -224,6 +224,8 @@ static void to_json(const QObject *obj, QString *str)
>> >          }
>> >          break;
>> >      }
>> > +    case QTYPE_QERROR:
>> > +        /* XXX: should QError be emitted? */
>> 
>> Pros & cons?
>
>  It's probably convenient to have qjson emitting QError, I'm unsure
> if we should do that for all kinds of QObjects though.

For a general purpose system, I'd recommend to cover all types.  But as
long as this has just one user (QEMU), it can use the special purpose
excuse not to.

>> >      case QTYPE_NONE:
>> >          break;
>> >      }
>> > diff --git a/qobject.h b/qobject.h
>> > index 2270ec1..07de211 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;
>> 
>> Erroneous QERRs are detected only when they're passed to
>> qerror_from_info() at run-time, i.e. when the error happens.  Likewise
>> for erroneous qerror_table[].desc.  Perhaps a unit test to ensure
>> qerror_table[] is sane would make sense.  Can't protect from passing
>> unknown errors to qerror_from_info(), but that shouldn't be a problem in
>> practice.
>
>  We could also have a debug function that could run once at startup
> and do the check.

Yes.

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Markus Armbruster]

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

> Markus Armbruster wrote:
>> 1. QError feels overengineered, particularly for a first iteration of a
>>    protocol.
>>
>>    We go to great lengths to build highly structured error objects.
>>    There is only one sane reason for doing that: a demonstrated client
>>    need.  I can't see that need.
>>   
>
> A GUI wants to present the user a mechanism to reduce the memory
> consumption of a VM.  There are three reasons why this would not work.
> The first is that ballooning was not configured for this guest.  The
> second is that ballooning was configured but the version of the KVM
> kernel module does not have a sync mmu.  The third is that an
> appropriate guest driver has not been loaded for the device.
>
> If we don't specify a type of error, all the GUI can do is fail right
> before trying to balloon the guest.  That offers the user no insight
> why it failed other than popping up a message box with an error
> message (that may not even be in their locale).  For a non-english
> speaker, it's gibberish.
>
> If you use three types of errors, then a GUI can differentiate these
> cases and presents the user different feedback.  For the first case,
> it can simply gray out the menu item.  For the second can, it can also
> gray out the menu item but it can provide a one time indication (in
> say, a status bar), that ballooning is unavailable because the kernel
> version does not support it.  For the third error, you would want to
> prompt the user to indicate the driver is not loaded in the guest.

So far, this is a convincing argument for reasonably specific error
codes, and on that we're in violent agreement.  What you didn't
demonstrate here is a need for *structured* error reporting,
i.e. machine-readable data beyond a simple error code.

> It's highly likely that for this last case, you'd want generic code to
> generate this error.  Further more, in order to generate the error
> message for a user, you need to know what device does not have a
> functioning driver.  You may say it's obvious for something like info
> balloon but it's not so obvious if you assume we support things other
> than just virtio-pci-balloon.  For instance, with s390, it's a
> different balloon driver.  For xenner/xenpv, it's a different driver.
> It really suggests we should send the qdev device name for the current
> balloon driver.

This is your argument for structured error reporting.  It boils down to
client localization.

I challenge the idea that we should even try to get that covered now.
We need to get the basics right, and the only way to do that is to use
them in anger.  The more baggage we design into the thing before we use
it, the higher the risk that we screw it up.

With our JSON encoding we can easily extend the error object without
breaking existing users.  It's designed for growth.  One more reason not
to try to anticipate everything.

Iteration beats the committee.  Let's iterate.

>> 2. It falls short of the requirement that clients can reasonably
>>    classify errors they don't know.
>>   
>
> I think this is wishful thinking.  I strongly suspect that you can't
> find a reasonably popular web browser that doesn't know all of the
> error codes that web servers generate.

I'd argue that that wasn't the case while HTTP was still evolving, and I
bet it won't be the case while QMP is evolving.

> We should document all of the errors that each QMP function can return.

Yes, we should.

> That said, if you thought there was 4-5 common classes of errors,
> adding an additional 'base_class' field could be a completely
> reasonable thing to do.  As long as you're adding information
> vs. taking it away, I'm quite happy.

Fair enough.

HTTP and FTP encode the error class into the error code.  Clever, but
since we decided to use textual error codes, a separate textual error
class probably makes the most sense.

>> 3. It falls short of the requirement that clients can easily present a
>>    human-readable error description to their human users, regardless of
>>    whether they know the error or not.
>>   
>
> That's just incorrect.  We provide an example conversion table that's
> akin to strerror() or a __repr__ for an exception in Python.

As Jamie already said, that won't work unless we transfer the table over
the wire, or require client to match server perfectly.  I doubt you want
the latter.  Did you mean to suggest the former?

>> In more detail[*]:
>>
>> There are more than one way to screw up here.  One is certainly to fall
>> short of client requirements in a way that is costly to fix (think
>> incompatible protocol revision).  Another is to overshoot them in a way
>> that is costly to maintain.  A third one is to spend too much time on
>> figuring out the perfect solution.
>>
>> I believe our true problem is that we're still confused and/or
>> disagreeing on client requirements, and this has led to a design that
>> tries to cover all the conceivable bases, and feels overengineered to
>> me.
>>   
>
> I've described my requirements for what a client can do.  I'd like to
> understand how you disagree.

I have essentially two complaints:

* I'm afraid we've made QError much more complex than it need be (item 1
  above), at least for a first iteration of the protocol.

* The design has shortcomings that effectively require clients to know
  all errors (items 2 & 3 above).

>> There are only so many complexity credits you can spend in a program,
>> both globally and individually.  I'm very, very wary of making error
>> reporting more complex than absolutely, desperately necessary.
>>   
>
> We're following a very well established model of error reporting
> (object-based exceptions).  This is well established and well
> understood.

It's a well-established, well-understood mechanism within a single
program, where coupling between the parts is checked and supported by
the programming language(s).

But we're designing a (hopefully!) simple networking protocol.  Got
precedence for object-based exceptions in simple networking protocols?
Or are we breaking new ground here?

>> Reporting errors should remain as easy as we can make it.  The more
>> cumbersome it is to report an error, the less it is done, and the more
>> vaguely it is done.  If you have to edit more than the error site to
>> report an error accurately, then chances skyrocket that it won't be
>> reported, or it'll be reported inaccurately. 
>
> I don't buy these sort of arguments.  We are not designing a library
> that is open to abuse.  If our developers are fall victim to this,
> then we need to retrain them to be less lazy by not accepting patches
> that do a poor job reporting errors.

I'm afraid I'm not as sanguine as you on the topic of patch review.

>>  And not because coders are
>> stupid or lazy, but because they make sensible use of their very limited
>> complexity credits: if you can either get the job done with lousy error
>> messages, or not get it done at all, guess what the smart choice is.
>>   
>
> Error reporting is extremely important in a management scenario.  You
> are severely limited by what actions you can take based on the amount
> of error information returned.  This is particularly when dealing with
> a multi-client management mechanism.  IMHO, this is a sensible place
> to spend complexity credits.

I don't disagree with this as a general sentiment, I do disagree with
the idea that we need to spend them *now*, before we even tried to use
the thing in a management scenario.

>> If I understand Dan correctly, machine-readable error code +
>> human-readable description is just fine, as long as the error code is
>> reasonably specific and the description is precise and complete.  Have
>> we heard anything else from client developers?
>>   
>
> There are no client developers because QMP doesn't exist.
> Historically, we haven't provided high quality error messages so of
> course libvirt makes due without them today.
>
> But I can answer on behalf of the management work we've done based on
> libvirt.
>
> If you attempt to do a virDomainSetMemory() to a qemu domain, you only
> get an error if you're using a version of qemu that doesn't support
> the balloon command.  You do not get an error at all for the case
> where the kernel module doesn't support ballooning or a guest driver
> isn't loaded.
>
> The concrete use case here is building an autoballoon mechanism part
> of a larger management suite.  If you're in an environment where
> memory overcommit is important, then you really do want to know
> whether ballooning isn't functioning because appropriate guest drivers
> aren't loaded.

Again, this is a convincing argument for reasonably specific error
codes, not for highly structured errors.

>> I'm not a client developer, but let me make a few propositions on
>> client needs anyway:
>>
>> * Clients are just as complexity-bound as the server.  They prefer their
>>   errors as simple as possible, but no simpler.
>>   
>
> I agree but the problem is that every client has a different
> definition of "simple as possible".  My client doesn't necessarily
> care about PCI hotplug error messages because it's not a function that
> we use.   However, ballooning is tremendously important and I want to
> know everything I can about why it failed.
>
>> * The crucial question for the client isn't "what exactly went wrong".
>>   It is "how should I handle this error".  Answering that question
>>   should be easy (say, check the error code).  Figuring out what went
>>   wrong should still be possible for a human operator of the client.
>>   
>
> I disagree.  The client needs to know what went wrong if they are
> going to take a corrective action.

For errors the client doesn't know, it can't possibly answer "what
exactly went wrong".  It still has to answer the crucial question "how
should I handle this error".

For errors the client knows, I'd expect a (reasonably specific!) error
code to be sufficient information, except perhaps for a few particularly
interesting cases.  Your ballooning example isn't such a case as far as
I can see.

>> * Clients don't want to be tightly coupled to the server.
>>   
>
> I don't follow this.

We want to be able to use an old client with a new server and vice
versa.

>> * No matter how smart and up-to-date the client is, there will always be
>>   errors it doesn't know.  And it needs to answer the "how to handle"
>>   question whether it knows the error code or not!  That's why protocols
>>   like HTTP have simple rules to classify error codes.
>>   
>
> HTTP is the exception and the HTTP error classes really leave an awful
> lot to be desired.  If you feel it's important to add error classes,
> I'd be happy to consider those patches.
>
>>   Likewise, it needs to be able to give a human operator enough
>>   information to figure out what went wrong whether it knows the error
>>   or not.  How do you expect clients to format a structured error object
>>   for an error they don't know into human-readable text?  Isn't it much
>>   easier and more robust to cut out the formatting middle-man and send
>>   the text along with the error?
>>   
>
> We do that by providing a conversion table.  The fundamental problem
> here is localization.
>
>> * Clients need to present a human-readable error description to their
>>   human users, regardless of whether they know the error or not.
>>   
>
> I would never pass an error string from a third party directly to a
> user.  I doubt you'll find a lot of good applications that do.  From a
> usability perspective, you need to be in control of your interactions
> with the user.  They grammar needs to be consistent and it has to be
> localized.  The best you would do with a third party string is log it
> to some log file for later examination by support.  In that case,
> dumping the class code and the supporting information in JSON form is
> just as good as a text description.

How should the application report an error it doesn't know to the user?

>> There's a general rule of programming that I've found quite hard to
>> learn and quite painful to disobey: always try the stupidest solution
>> that could possibly work first.
>>
>> Based on what I've learned about client requirements so far, I figure
>> that solution is "easily classified error code + human-readable
>> description".
>>   
>
> I appreciate you feel strongly that this is the right thing to do.

At least in the first iteration.

> That said, I disagree and I have specific use cases in mind that are
> not satisfied by the above mechanism.  In a situation like this, I
> tend to think the best thing to do is provide more information such
> that everyone can get the function they want.
>
> If we add a base_class member to the error structure and we have the
> table for formatting error messages, then that should satisfy your
> requirements.  If you're concerned about a client having to have an
> up-to-date version of the table, we can introduce a monitor command to
> pretty print a json error object which would address that concern.

I don't like the extra round-trip to pretty-print the error much (what
if my connection fails just then?), but I figure I could live with that.

> Then I think your argument boils down to, you think the remaining
> infrastructure is unnecessary for what you anticipate the uses to be.
> But hopefully, you'll at least concede that there are valid use cases
> beyond what you anticipate to be the normal case that do need this
> information.

I gladly concede that I can't anticipate all uses.  That's my point,
actually.  I don't trust my anticipating, I want uses demonstrated
before building complex solutions for them.

>> If we later realize that this solution was *too* stupid, we can simply
>> add a data member to the error object.
>>   
>
> It's never that easy because a management tool has to support a least
> common denominator.

If we build complex solutions for anticipated needs, we run a high risk
of missing real needs.  And then we'll evolve the protocol "sideways",
which is even less easy for management tools than evolving "upwards".

We'll iterate anyway, so why not embrace it?  Start with something
simple and functional, then extend it to address what's found lacking.

Re: [Qemu-devel] Re: [PATCH 00/10]: QError v4

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

>> 2. It falls short of the requirement that clients can reasonably
>>     classify errors they don't know.
>
> True.  (Though adding a classification mechanism can be done later
> once we have an idea of what errors are there at all).

Yes.

If we think there's a risk screwing up the classification, because we
don't yet understand the errors sufficiently, and we think a screwup
would be is awkward to fix later, then deferring to a later iteration
would make sense.

>> 3. It falls short of the requirement that clients can easily present a
>>     human-readable error description to their human users, regardless of
>>     whether they know the error or not.
>
> That's true.  I would definitely have the interpolated desc field go
> on the wire too, as part of the JSON form of QError.

That would be my preference, too.

>> If I understand Dan correctly, machine-readable error code +
>> human-readable description is just fine, as long as the error code is
>> reasonably specific and the description is precise and complete.  Have
>> we heard anything else from client developers?
>
> Then (except for the asynchronicity) the current qemu monitor protocol
> is also just fine, including the fact that we send migrate and get
> m\rmi\rmig\rmigr etc. back on the socket.
>
> "error while creating snapshot on '%s'" may be easy to parse, but
> looking at a dictionary field in { "filename" : "blah.img" } is
> easier, and at the same time solves issues with escaping weird
> characters in the file name.

As soon as the client needs to know more than a (reasonably specific)
error code, structured error data helps.

But I can't see that need in your particular example.  The client asks
the server to create a snapshot.  The operation fails.  The client
already knows what the name of the snapshot is, doesn't it?  No need to
get it from the error.

>> * The crucial question for the client isn't "what exactly went wrong".
>>    It is "how should I handle this error".  Answering that question
>>    should be easy (say, check the error code).  Figuring out what went
>>    wrong should still be possible for a human operator of the client.
>
> The same error can be handled in a different way depending on the
> situation.  A missing image is in general a fatal error, but maybe
> another client could create images on the fly.

The latter client surely knows this error.

My question is how to help clients handle errors they don't know.  In
your example, the help would boil down to "this error is generally
fatal".

>> Based on what I've learned about client requirements so far, I figure
>> that solution is "easily classified error code + human-readable
>> description".
>
> How would you classify the error code?  By subsystem or by (for
> example) severity or anything else?  Does QEMU have subsystems that
> are well-identified enough to be carved in stone in QError?  (All
> genuine questions.  I have no idea).

My first cut (really just a shot from the hip) was[*]:

    Something like client error (i.e. your command was stupid, and
    trying again will be just as stupid), permanent server error (it
    wasn't stupid, but I can't do it for you), transient server error (I
    couldn't do it for you, but trying again later could help).


[*] Message-ID: <877huy6hzm.fsf@pike.pond.sub.org>

[Qemu-devel] Re: [PATCH 07/10] Introduce QError

[Paolo Bonzini]

On 11/19/2009 09:42 AM, Markus Armbruster wrote:
>> >
>> >    It's probably convenient to have qjson emitting QError, I'm unsure
>> >  if we should do that for all kinds of QObjects though.
>
> For a general purpose system, I'd recommend to cover all types.  But as
> long as this has just one user (QEMU), it can use the special purpose
> excuse not to.

You're not sending QErrors on the wire yet, are you?  Once you do, I 
suspect it will be easiest to handle to_json for QError too (or make it 
a virtual method as it was in my patches...).

Paolo

[Qemu-devel] Re: [PATCH 00/10]: QError v4

[Paolo Bonzini]

On 11/19/2009 11:25 AM, Markus Armbruster wrote:
> But I can't see that need in your particular example.  The client asks
> the server to create a snapshot.  The operation fails.  The client
> already knows what the name of the snapshot is, doesn't it?  No need to
> get it from the error.

Yeah, I just took a random error from grep so your objection makes total 
sense.  But is that still true when we have asynchronous notifications?

Paolo

Re: [Qemu-devel] Re: [PATCH 00/10]: QError v4

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 11/19/2009 11:25 AM, Markus Armbruster wrote:
>> But I can't see that need in your particular example.  The client asks
>> the server to create a snapshot.  The operation fails.  The client
>> already knows what the name of the snapshot is, doesn't it?  No need to
>> get it from the error.
>
> Yeah, I just took a random error from grep so your objection makes
> total sense.  But is that still true when we have asynchronous
> notifications?

I don't know.  We'll figure it out when we get them.

Asynchronous notifications aren't errors, are they?  I'd expect them to
be different kinds of objects.  And then what makes sense for one of
them may or may not transfer to the other.

Re: [Qemu-devel] [PATCH 09/10] qdev: Use QError for 'device not found' error

[Amit Shah]

On (Wed) Nov 18 2009 [16:17:02], Markus Armbruster wrote:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > @@ -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_new(QERR_DEVICE_NOT_FOUND, driver);
> >          return NULL;
> >      }
> >      if (info->no_user) {
> 
> Not obvious from this patch, but we lose the "Try -device '?' for a
> list" hint here.  In PATCH 7/10:

BTW that hint isn't always appropriate as it's printed on the monitor
when doing 'device_add' as well.

		Amit

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Anthony Liguori]

Jamie Lokier wrote:
> Anthony Liguori wrote:
>   
>> Markus Armbruster wrote:
>>     
>>> 3. It falls short of the requirement that clients can easily present a
>>>   human-readable error description to their human users, regardless of
>>>   whether they know the error or not.
>>>  
>>>       
>> That's just incorrect.  We provide an example conversion table that's 
>> akin to strerror() or a __repr__ for an exception in Python.
>>     
>
> Markus refers to errors that the client does not know - i.e. when the
> client is older than qemu, or is not in the same development branch if
> it's a branched qemu.  Which means the client won't have a fully up to
> date conversion table.
>
> Do you mean qemu provides it's current conversion table to the client
> over the wire protocol?
>   

(qemu) format_error "{'class': 'DeviceNotFound', 'data' : {'addr': 
'00:11:22'} }"
Device 0:11:22 is not present

Is what I'm thinking.  I don't think it's needed but it solves the 
"problem".

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Anthony Liguori]

Markus Armbruster wrote:
>> It's highly likely that for this last case, you'd want generic code to
>> generate this error.  Further more, in order to generate the error
>> message for a user, you need to know what device does not have a
>> functioning driver.  You may say it's obvious for something like info
>> balloon but it's not so obvious if you assume we support things other
>> than just virtio-pci-balloon.  For instance, with s390, it's a
>> different balloon driver.  For xenner/xenpv, it's a different driver.
>> It really suggests we should send the qdev device name for the current
>> balloon driver.
>>     
>
> This is your argument for structured error reporting.  It boils down to
> client localization.
>
> I challenge the idea that we should even try to get that covered now.
> We need to get the basics right, and the only way to do that is to use
> them in anger.  The more baggage we design into the thing before we use
> it, the higher the risk that we screw it up.
>   

Current proposal is:

qemu_error_new(QERR_DEVICE_NOT_FOUND, "00:11:22");

Alternative proposal[1]:

qemu_error_new(QERR_DEVICE_NOT_FOUND, "Could not find PCI device 00:11:22");

Alternative proposal[2]:

qemu_error_new(QERR_DEVICE_NOT_FOUND);

Your argument has merit if you're countering with [2].  [1] is incorrect 
because it makes localization impossible.  This isn't an evolutionary 
feature, this is a hard requirement as far as I'm concerned.  At least 
[2] allows localization.

I dislike [2] though because it means that our errors are not going to 
be able to be generic enough.  I really want the information of which 
device was not found to be part of the error message.

>> I've described my requirements for what a client can do.  I'd like to
>> understand how you disagree.
>>     
>
> I have essentially two complaints:
>
> * I'm afraid we've made QError much more complex than it need be (item 1
>   above), at least for a first iteration of the protocol.
>
> * The design has shortcomings that effectively require clients to know
>   all errors (items 2 & 3 above).
>   

My main concern is that we're making an important feature impossible.  
If we're arguing for errno style errors verses structured exceptions, I 
think that's a more reasonable argument.  I'm really concerned about the 
long term ramifications about combining errno style errors with free 
formed, non-localized text error messages.

>> I would never pass an error string from a third party directly to a
>> user.  I doubt you'll find a lot of good applications that do.  From a
>> usability perspective, you need to be in control of your interactions
>> with the user.  They grammar needs to be consistent and it has to be
>> localized.  The best you would do with a third party string is log it
>> to some log file for later examination by support.  In that case,
>> dumping the class code and the supporting information in JSON form is
>> just as good as a text description.
>>     
>
> How should the application report an error it doesn't know to the user?
>   

An error it doesn't know about is a bug in the application.  Adding a 
new type of error to a monitor function is equivalent to changing it's 
behavior.  It should require a versioning change.

>>> If we later realize that this solution was *too* stupid, we can simply
>>> add a data member to the error object.
>>>   
>>>       
>> It's never that easy because a management tool has to support a least
>> common denominator.
>>     
>
> If we build complex solutions for anticipated needs, we run a high risk
> of missing real needs.  And then we'll evolve the protocol "sideways",
> which is even less easy for management tools than evolving "upwards".
>
> We'll iterate anyway, so why not embrace it?  Start with something
> simple and functional, then extend it to address what's found lacking.
>   

I'm not arguing for a mythical use-case that could come in five years.  
This is something I want to support right now.

My basic concerns boil down to:

 1) It must be possible to support localization from the very beginning

 2) Client developers should not have to parse an english language, 
subject to change, string in order to get more information about an 
error.  IOW, the error object should always produce the same descriptive 
message regardless of where or how it's generated as long as it's fields 
are equal.

For 2, there are two ways to do this:  have an error message table based 
on error codes with no structured data or have an error template table 
based on error codes and a dictionary of optional data.

The later is a superset of the former so it seems to be really obvious 
to start with the later.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Luiz Capitulino]

On Fri, 20 Nov 2009 09:56:46 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> Jamie Lokier wrote:
> > Anthony Liguori wrote:
> >   
> >> Markus Armbruster wrote:
> >>     
> >>> 3. It falls short of the requirement that clients can easily present a
> >>>   human-readable error description to their human users, regardless of
> >>>   whether they know the error or not.
> >>>  
> >>>       
> >> That's just incorrect.  We provide an example conversion table that's 
> >> akin to strerror() or a __repr__ for an exception in Python.
> >>     
> >
> > Markus refers to errors that the client does not know - i.e. when the
> > client is older than qemu, or is not in the same development branch if
> > it's a branched qemu.  Which means the client won't have a fully up to
> > date conversion table.

 What's the proper fix for this? I can only think of having QError on
library, which is what we're going to do, anyway.

> > Do you mean qemu provides it's current conversion table to the client
> > over the wire protocol?
> >   
> 
> (qemu) format_error "{'class': 'DeviceNotFound', 'data' : {'addr': 
> '00:11:22'} }"
> Device 0:11:22 is not present
> 
> Is what I'm thinking.  I don't think it's needed but it solves the 
> "problem".

 Couldn't the client print the error class for errors it doesn't
know about? Like:

"Unknown error: 'CloudProtocol'"

 I know this is bad, but seems a lot better than printing something
with the wrong locale (ie, 'desc').

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Anthony Liguori]

Luiz Capitulino wrote:
> On Fri, 20 Nov 2009 09:56:46 -0600
> Anthony Liguori <anthony@codemonkey.ws> wrote:
>
>   
>> Jamie Lokier wrote:
>>     
>>> Anthony Liguori wrote:
>>>   
>>>       
>>>> Markus Armbruster wrote:
>>>>     
>>>>         
>>>>> 3. It falls short of the requirement that clients can easily present a
>>>>>   human-readable error description to their human users, regardless of
>>>>>   whether they know the error or not.
>>>>>  
>>>>>       
>>>>>           
>>>> That's just incorrect.  We provide an example conversion table that's 
>>>> akin to strerror() or a __repr__ for an exception in Python.
>>>>     
>>>>         
>>> Markus refers to errors that the client does not know - i.e. when the
>>> client is older than qemu, or is not in the same development branch if
>>> it's a branched qemu.  Which means the client won't have a fully up to
>>> date conversion table.
>>>       
>
>  What's the proper fix for this? I can only think of having QError on
> library, which is what we're going to do, anyway.
>   

There are two use cases to consider.  The first is logging of errors.  
Logging the json representation of a QError should be just as good for 
postmortem debugging as a formatted description.  That's really the 
point, there is no additional information in the formatted description.

The second use case is presenting an error message to a user.  My 
contention is that a good UI is not going to present an unknown error to 
a user either in JSON representation or in a non-localized string.  Both 
are equally gibberish to non-English speakers.

I think your suggestion of just offering the error class is the best 
I've heard so far.  That at least makes the message google-able.

>>> Do you mean qemu provides it's current conversion table to the client
>>> over the wire protocol?
>>>   
>>>       
>> (qemu) format_error "{'class': 'DeviceNotFound', 'data' : {'addr': 
>> '00:11:22'} }"
>> Device 0:11:22 is not present
>>
>> Is what I'm thinking.  I don't think it's needed but it solves the 
>> "problem".
>>     
>
>  Couldn't the client print the error class for errors it doesn't
> know about? Like:
>
> "Unknown error: 'CloudProtocol'"
>
>  I know this is bad, but seems a lot better than printing something
> with the wrong locale (ie, 'desc').
>   

Yes, I agree completely.  I was offering format_error only as a 
concession.  I honestly believe that the structured data is just as good 
to a developer as the message and that the message if not localized is 
gibberish to the user.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> Jamie Lokier wrote:
>> Anthony Liguori wrote:
>>   
>>> Markus Armbruster wrote:
>>>     
>>>> 3. It falls short of the requirement that clients can easily present a
>>>>   human-readable error description to their human users, regardless of
>>>>   whether they know the error or not.
>>>>        
>>> That's just incorrect.  We provide an example conversion table
>>> that's akin to strerror() or a __repr__ for an exception in Python.
>>>     
>>
>> Markus refers to errors that the client does not know - i.e. when the
>> client is older than qemu, or is not in the same development branch if
>> it's a branched qemu.  Which means the client won't have a fully up to
>> date conversion table.
>>
>> Do you mean qemu provides it's current conversion table to the client
>> over the wire protocol?
>>   
>
> (qemu) format_error "{'class': 'DeviceNotFound', 'data' : {'addr':
> 00:11:22'} }"
> Device 0:11:22 is not present
>
> Is what I'm thinking.  I don't think it's needed but it solves the
> "problem".

Any particular reason not put it into the error object and be done with
it?

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Anthony Liguori]

Markus Armbruster wrote:
> Any particular reason not put it into the error object and be done with
> it?
>   

As long as it's generated and not supplied by the caller of 
qemu_error_new, I really don't mind.



Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> Luiz Capitulino wrote:
>> On Fri, 20 Nov 2009 09:56:46 -0600
>> Anthony Liguori <anthony@codemonkey.ws> wrote:
>>
>>   
>>> Jamie Lokier wrote:
>>>     
>>>> Anthony Liguori wrote:
>>>>         
>>>>> Markus Armbruster wrote:
>>>>>             
>>>>>> 3. It falls short of the requirement that clients can easily present a
>>>>>>   human-readable error description to their human users, regardless of
>>>>>>   whether they know the error or not.
>>>>>>                  
>>>>> That's just incorrect.  We provide an example conversion table
>>>>> that's akin to strerror() or a __repr__ for an exception in
>>>>> Python.
>>>>>             
>>>> Markus refers to errors that the client does not know - i.e. when the
>>>> client is older than qemu, or is not in the same development branch if
>>>> it's a branched qemu.  Which means the client won't have a fully up to
>>>> date conversion table.
>>>>       
>>
>>  What's the proper fix for this? I can only think of having QError on
>> library, which is what we're going to do, anyway.
>>   
>
> There are two use cases to consider.  The first is logging of errors.
> Logging the json representation of a QError should be just as good for
> postmortem debugging as a formatted description.  That's really the
> point, there is no additional information in the formatted
> description.
>
> The second use case is presenting an error message to a user.  My
> contention is that a good UI is not going to present an unknown error
> to a user either in JSON representation or in a non-localized string.
> Both are equally gibberish to non-English speakers.

Incomplete localization should fail gracefully.  That's why gettext()
returns its argument when the message catalog doesn't have a
translation.

This might be gibberish for some users, but even then it's eminently
google-able gibberish.

Moreover, not all users preferring a non-English locale are unable to
figure out an English message, either by themselves or with help from a
friend.

I figure the set of human users who can handle JSON better than English
is vanishingly small.

> I think your suggestion of just offering the error class is the best
> I've heard so far.  That at least makes the message google-able.

A properly localized description of the error class can't replace the
full error message, because it is less specific.  But it's a fine
supplement.  Of course, you may lack a translation for the error class,
as well.  Should be less likely, though, because error classes should
change less frequently than instances.

>>>> Do you mean qemu provides it's current conversion table to the client
>>>> over the wire protocol?
>>>>         
>>> (qemu) format_error "{'class': 'DeviceNotFound', 'data' : {'addr':
>>> 00:11:22'} }"
>>> Device 0:11:22 is not present
>>>
>>> Is what I'm thinking.  I don't think it's needed but it solves the
>>> "problem".
>>>     
>>
>>  Couldn't the client print the error class for errors it doesn't
>> know about? Like:
>>
>> "Unknown error: 'CloudProtocol'"
>>
>>  I know this is bad, but seems a lot better than printing something
>> with the wrong locale (ie, 'desc').
>>   
>
> Yes, I agree completely.  I was offering format_error only as a
> concession.  I honestly believe that the structured data is just as
> good to a developer as the message and that the message if not
> localized is gibberish to the user.

Good tools make the simple simple and the difficult possible.

Besides sophisticated management tools solving difficult problems, we
should make QMP as easy and convenient as possible for simple tools,
like ad hoc scripts.  If such scripts have to jump through hoops to get
error reporting fit for human beings, we failed the "make the simple
simple" part.

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> Markus Armbruster wrote:
>>> It's highly likely that for this last case, you'd want generic code to
>>> generate this error.  Further more, in order to generate the error
>>> message for a user, you need to know what device does not have a
>>> functioning driver.  You may say it's obvious for something like info
>>> balloon but it's not so obvious if you assume we support things other
>>> than just virtio-pci-balloon.  For instance, with s390, it's a
>>> different balloon driver.  For xenner/xenpv, it's a different driver.
>>> It really suggests we should send the qdev device name for the current
>>> balloon driver.
>>>     
>>
>> This is your argument for structured error reporting.  It boils down to
>> client localization.
>>
>> I challenge the idea that we should even try to get that covered now.
>> We need to get the basics right, and the only way to do that is to use
>> them in anger.  The more baggage we design into the thing before we use
>> it, the higher the risk that we screw it up.
>>   
>
> Current proposal is:
>
> qemu_error_new(QERR_DEVICE_NOT_FOUND, "00:11:22");
>
> Alternative proposal[1]:
>
> qemu_error_new(QERR_DEVICE_NOT_FOUND, "Could not find PCI device 00:11:22");
>
> Alternative proposal[2]:
>
> qemu_error_new(QERR_DEVICE_NOT_FOUND);
>
> Your argument has merit if you're countering with [2].  [1] is
> incorrect because it makes localization impossible.  This isn't an
> evolutionary feature, this is a hard requirement as far as I'm
> concerned.  At least [2] allows localization.
>
> I dislike [2] though because it means that our errors are not going to
> be able to be generic enough.  I really want the information of which
> device was not found to be part of the error message.
>
>>> I've described my requirements for what a client can do.  I'd like to
>>> understand how you disagree.
>>>     
>>
>> I have essentially two complaints:
>>
>> * I'm afraid we've made QError much more complex than it need be (item 1
>>   above), at least for a first iteration of the protocol.
>>
>> * The design has shortcomings that effectively require clients to know
>>   all errors (items 2 & 3 above).
>>   
>
> My main concern is that we're making an important feature impossible.
> If we're arguing for errno style errors verses structured exceptions,
> I think that's a more reasonable argument.  I'm really concerned about
> the long term ramifications about combining errno style errors with
> free formed, non-localized text error messages.

We have:

(1) machine-readable error code

(2) human-readable error message

(3) machine-readable additional error data

The old monitor prints just (3).

You propose to have QMP send (1) and (3).  This forces all clients to
come up with (2) themselves.  Why that's problematic is discussed
elsewhere in this thread.

Imagine we already had a QMP that sent (1) and (2), encoded in JSON.
Then nothing could stop you to add a "data" part holding (3).  Ergo,
keeping things simple by sending just (1) and (2) now does not make it
impossible to send (3) later, hence does not make it impossible to
provide your important feature in a future iteration.

>>> I would never pass an error string from a third party directly to a
>>> user.  I doubt you'll find a lot of good applications that do.  From a
>>> usability perspective, you need to be in control of your interactions
>>> with the user.  They grammar needs to be consistent and it has to be
>>> localized.  The best you would do with a third party string is log it
>>> to some log file for later examination by support.  In that case,
>>> dumping the class code and the supporting information in JSON form is
>>> just as good as a text description.
>>>     
>>
>> How should the application report an error it doesn't know to the user?
>>   
>
> An error it doesn't know about is a bug in the application.  Adding a
> new type of error to a monitor function is equivalent to changing it's
> behavior.  It should require a versioning change.

What do you mean by versioning change?  And what does it mean for
clients?

>>>> If we later realize that this solution was *too* stupid, we can simply
>>>> add a data member to the error object.
>>>>         
>>> It's never that easy because a management tool has to support a least
>>> common denominator.
>>>     
>>
>> If we build complex solutions for anticipated needs, we run a high risk
>> of missing real needs.  And then we'll evolve the protocol "sideways",
>> which is even less easy for management tools than evolving "upwards".
>>
>> We'll iterate anyway, so why not embrace it?  Start with something
>> simple and functional, then extend it to address what's found lacking.
>>   
>
> I'm not arguing for a mythical use-case that could come in five years.
> This is something I want to support right now.
>
> My basic concerns boil down to:
>
> 1) It must be possible to support localization from the very beginning

If you insist on supporting localization in the client right now,
despite costs & risks, there's nothing I can do, and nothing more for me
to say on my first concern (QMP overengineered for a first iteration of
the protocol).

> 2) Client developers should not have to parse an english language,
> subject to change, string in order to get more information about an
> error.  IOW, the error object should always produce the same
> descriptive message regardless of where or how it's generated as long
> as it's fields are equal.
>
> For 2, there are two ways to do this:  have an error message table
> based on error codes with no structured data or have an error template
> table based on error codes and a dictionary of optional data.
>
> The later is a superset of the former so it seems to be really obvious
> to start with the later.

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Anthony Liguori]

Markus Armbruster wrote:
> We have:
>
> (1) machine-readable error code
>
> (2) human-readable error message
>
> (3) machine-readable additional error data
>
> The old monitor prints just (3).
>   

s:(3):(2):

> You propose to have QMP send (1) and (3).  This forces all clients to
> come up with (2) themselves.  Why that's problematic is discussed
> elsewhere in this thread.
>   

As I said else where, sending (2) along with (1) and (3) doesn't bother 
me.  It's whether (2) is generated automatically from (1) and (2) or 
whether within qemu, the caller who creates the error supplies that text 
free hand.  That's what I'm fundamentally opposed to.

> Imagine we already had a QMP that sent (1) and (2), encoded in JSON.
> Then nothing could stop you to add a "data" part holding (3).

If (2) is generated from (1) based on a table, then I agree with you.  
If (2) is open coded, then I disagree fundamentally because you're 
mixing in information from (3) in there.

>   Ergo,
> keeping things simple by sending just (1) and (2) now does not make it
> impossible to send (3) later, hence does not make it impossible to
> provide your important feature in a future iteration.
>   

Let me give a concrete example of where your logic falls apart.  Say you 
start out with:

qemu_error_new(QERR_DEVICE_NOT_READY, "Ballooning is not enabled in the 
guest");

And somewhere else you have:

qemu_error_new(QERR_DEVICE_NOT_READY, "The virtio-net driver is not 
loaded in the guest");

There is no way to unify these into a coherent message.  It's forever 
free form and the there will always be additional information in the 
description that requires parsing from a client.  This is obviously more 
complicated for the client.  It's also impossible to internationalize 
effectively because even if you introduce a data field, you aren't 
passing enough information to generate these messages.  In this case, 
it's a subtle case of grammatical mismatch although both messages are 
saying the exact same thing.

>> An error it doesn't know about is a bug in the application.  Adding a
>> new type of error to a monitor function is equivalent to changing it's
>> behavior.  It should require a versioning change.
>>     
>
> What do you mean by versioning change?  And what does it mean for
> clients?
>   

We really haven't gotten into versioning/feature negotation, but in my 
mind, if we add a new error type for a function, that's the same as 
adding an additional argument.  It's a new feature that requires some 
sort of version/feature mechanism.

>> My basic concerns boil down to:
>>
>> 1) It must be possible to support localization from the very beginning
>>     
>
> If you insist on supporting localization in the client right now,
> despite costs & risks, there's nothing I can do, and nothing more for me
> to say on my first concern (QMP overengineered for a first iteration of
> the protocol).
>   

Your proposal makes it impossible forever so yes, it's a hard 
requirement.  I'm making a lot of attempts to meet you half way but if 
your argument is "I don't like this, it's too complicated, I don't care 
about localization" then I don't think we're going to get over that.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> Markus Armbruster wrote:
>> We have:
>>
>> (1) machine-readable error code
>>
>> (2) human-readable error message
>>
>> (3) machine-readable additional error data
>>
>> The old monitor prints just (3).
>>   
>
> s:(3):(2):

D'oh!

>> You propose to have QMP send (1) and (3).  This forces all clients to
>> come up with (2) themselves.  Why that's problematic is discussed
>> elsewhere in this thread.
>>   
>
> As I said else where, sending (2) along with (1) and (3) doesn't
> bother me.  It's whether (2) is generated automatically from (1) and
> (2) or whether within qemu, the caller who creates the error supplies
> that text free hand.  That's what I'm fundamentally opposed to.

Having the caller pass both (2) and (3) would be dumb indeed.

>> Imagine we already had a QMP that sent (1) and (2), encoded in JSON.
>> Then nothing could stop you to add a "data" part holding (3).
>
> If (2) is generated from (1) based on a table, then I agree with you.
> If (2) is open coded, then I disagree fundamentally because you're
> mixing in information from (3) in there.

I'm thinking and talking primarily about the protocol, and that probably
makes me too terse on implementation.

I didn't mean to suggest that for adding the data part we should add new
arguments providing the data.  That would be dumb indeed.

Instead, I'd like to start with an extremely simple error reporting
mechanism, which requires an equally simple conversion, namely from
monitor_printf("human-readable error") to something of the form
qmp_error(error_code, "human-readable error").

If we later decide we want structured data in the error object, we can
still change it to something closer to the current proposal.  From the
protocol's point of view, it's a straightforward extension.  In the
server code, it's a change, not an extension.  And that's fine.

>>   Ergo,
>> keeping things simple by sending just (1) and (2) now does not make it
>> impossible to send (3) later, hence does not make it impossible to
>> provide your important feature in a future iteration.
>>   
>
> Let me give a concrete example of where your logic falls apart.  Say
> you start out with:
>
> qemu_error_new(QERR_DEVICE_NOT_READY, "Ballooning is not enabled in
> the guest");
>
> And somewhere else you have:
>
> qemu_error_new(QERR_DEVICE_NOT_READY, "The virtio-net driver is not
> loaded in the guest");
>
> There is no way to unify these into a coherent message.  It's forever
> free form and the there will always be additional information in the
> description that requires parsing from a client.  This is obviously
> more complicated for the client.  It's also impossible to
> internationalize effectively because even if you introduce a data
> field, you aren't passing enough information to generate these
> messages.  In this case, it's a subtle case of grammatical mismatch
> although both messages are saying the exact same thing.

Again, you're making an argument for specific, machine-readable errors
(an end), not for structured error data (means).  Reasonably specific
error codes could achieve that end just as well.  QERR_DEVICE_NOT_READY
isn't sufficiently specific.

In fact, even with additional structured data, I'd expect sharing the
same error code for such different errors to be rather inconvenient for
clients.  What's easier, switching over a sufficiently specific error
code, or switching over an insufficiently specific error code, then
digging through additional data to figure out what went wrong?

I'm afraid we're running in cycles...  Could explain the somewhat
irritated tone I sense further down.

>>> An error it doesn't know about is a bug in the application.  Adding a
>>> new type of error to a monitor function is equivalent to changing it's
>>> behavior.  It should require a versioning change.
>>>     
>>
>> What do you mean by versioning change?  And what does it mean for
>> clients?
>>   
>
> We really haven't gotten into versioning/feature negotation, but in my
> mind, if we add a new error type for a function, that's the same as
> adding an additional argument.  It's a new feature that requires some
> sort of version/feature mechanism.

This topic is somewhat tangential to the business at hand, but I think
it sheds some light on our different points of view, so here goes.

If we were talking about an interface within a single program, I think
I'd agree with you.  The program's parts should always be kept fully
synchronized with respect to the interface.  Easy, because you link them
together into a single package, and the tools help enforce the interface
contract.

However, here we're talking about separate programs, client and server.
Updating client and server in lock-step may be practical in some
deployments, but certainly not in others, e.g. when they run on
different computers that are separately administered.

Now, what should a client do when it discovers the monitor command it
needs to use has grown a new error?  Refuse to use the command for fear
of having to present a sub-par error message to the user in case it
fails?

Yes, the client needs updating in such a scenario.  But since it's a
normal, expected situation, I wouldn't call it a client bug.  And
regardless how we call it, we better handle it gracefully.

>>> My basic concerns boil down to:
>>>
>>> 1) It must be possible to support localization from the very beginning
>>>     
>>
>> If you insist on supporting localization in the client right now,
>> despite costs & risks, there's nothing I can do, and nothing more for me
>> to say on my first concern (QMP overengineered for a first iteration of
>> the protocol).
>>   
>
> Your proposal makes it impossible forever so yes, it's a hard
> requirement.

If my proposal made it impossible forever, you'd be right.  But it
clearly doesn't.

>               I'm making a lot of attempts to meet you half way but if
> your argument is "I don't like this, it's too complicated, I don't
> care about localization" then I don't think we're going to get over
> that.

I wouldn't consider that a fair characterization of my argument.  I
propose to take small, simple, safe steps forward instead of big leaps,
and I'm happy to delay localization in the client some for that.  Maybe
we just have to agree to disagree here.

I appreciate your willingness to accomodate me on what I consider
shortcomings of the protocol, which is really a different topic.  I'm
sure we can work that out.

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Anthony Liguori]

Markus Armbruster wrote:
> I'm thinking and talking primarily about the protocol, and that probably
> makes me too terse on implementation.
>
> I didn't mean to suggest that for adding the data part we should add new
> arguments providing the data.  That would be dumb indeed.
>
> Instead, I'd like to start with an extremely simple error reporting
> mechanism, which requires an equally simple conversion, namely from
> monitor_printf("human-readable error") to something of the form
> qmp_error(error_code, "human-readable error").
>   

This is what I want to focus on because I think everything just boils 
down to this. Namely:

qmp_error(error_code, "human-readable error");

In my mind, the purpose of QMP is to provide a machine readable form of 
the monitor protocol. We have a monitor protocol today but because it's 
written as free-form text (often English sentences), it's 
difficult/impossible to parse which makes managing qemu extremely 
difficult. We have problems with escaping characters, use of 
non-deterministic grammars, etc.

What I'm fundamentally opposed to is the "human-readable error" part of 
your proposal because it's free-form and unstructured. It has all the 
same problems as the current monitor interface. I'm opposed to this for 
the same reason I would be opposed to an "info balloon" command output of:

{"data" : {"target" : "The guest has 32MB of memory"}}

If all you were doing is outputting "info balloon" to a user in a 
command line tool, sure, this would be easier for the client to deal 
with. However, the whole purpose of QMP is to allow tools to make sense 
out of the monitor commands.

The contents of an error are just as important as the output of a 
command. The error code helps a bit but the problem with a qmp_error() 
that takes a string parameter is that there is additional information in 
that string that is inaccessible to a client.

I'm certainly willing to consider alternative ways to do qmp_error() but 
taking a free form string is not an option in my mind. It goes against 
the fundamentals of what we're trying to build with QMP.

So if you're opposed to structured error data, just having 
qmp_error(error_code) is a reasonable alternative. I don't think it's 
the right thing to do, but I think it's still within the spirit of the 
goals of QMP.

I think this is the fundamental thing to come to an agreement on in this 
discussion before we can even delve into the merits or lack thereof of 
structured error data.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Luiz Capitulino]

On Sat, 21 Nov 2009 11:02:41 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Now, what should a client do when it discovers the monitor command it
> needs to use has grown a new error?  Refuse to use the command for fear
> of having to present a sub-par error message to the user in case it
> fails?

 What's your solution for this problem?

> Yes, the client needs updating in such a scenario.  But since it's a
> normal, expected situation, I wouldn't call it a client bug.  And
> regardless how we call it, we better handle it gracefully.

 The best thing the client can do here is to print the error class.

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Luiz Capitulino]

On Sun, 22 Nov 2009 10:08:16 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> I'm certainly willing to consider alternative ways to do qmp_error() but 
> taking a free form string is not an option in my mind. It goes against 
> the fundamentals of what we're trying to build with QMP.

 Agreed.

> So if you're opposed to structured error data, just having 
> qmp_error(error_code) is a reasonable alternative. I don't think it's 
> the right thing to do, but I think it's still within the spirit of the 
> goals of QMP.

 You mean, we would have calls like:

qemu_error_new(error_code, 'device '%s' not found', name);

 and on the wire:

{ "error": 1234 }

 Did I get it right?

 If so, I can see some problems with it:

1. It's impossible to know what 1234 means by watching the
   protocol on the wire. Although this is a machine protocol,
   this is a good feature

2. We may have errors where having the error data is needed,
   and iirc someone gave an example of this some hundreds
   of emails ago

3. A new error will require a new code. Classes have the
   advantage of becoming stable over time and we'll end up
   just automatically reusing existing ones

 Note that (3) can make class-based error _easier_ to use
as there won't be the need to create new classes over time.

 This is not true for code-based errors. To guarantee there
are no duplicates, we'll have to centralize them in qerror.h,
this means that an enum will have to be updated for *each*
created error.

 I should also say that we're reaching the point where seeing
the current proposal work in practice would be more productive
then discussing it in theory.

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Anthony Liguori]

Luiz Capitulino wrote:
> On Sun, 22 Nov 2009 10:08:16 -0600
> Anthony Liguori <anthony@codemonkey.ws> wrote:
>
>   
>> I'm certainly willing to consider alternative ways to do qmp_error() but 
>> taking a free form string is not an option in my mind. It goes against 
>> the fundamentals of what we're trying to build with QMP.
>>     
>
>  Agreed.
>
>   
>> So if you're opposed to structured error data, just having 
>> qmp_error(error_code) is a reasonable alternative. I don't think it's 
>> the right thing to do, but I think it's still within the spirit of the 
>> goals of QMP.
>>     
>
>  You mean, we would have calls like:
>
> qemu_error_new(error_code, 'device '%s' not found', name);
>   

Except drop the 'device %s not found' bit.

>  and on the wire:
>
> { "error": 1234 }
>
>  Did I get it right?
>
>  If so, I can see some problems with it:
>
> 1. It's impossible to know what 1234 means by watching the
>    protocol on the wire. Although this is a machine protocol,
>    this is a good feature
>
> 2. We may have errors where having the error data is needed,
>    and iirc someone gave an example of this some hundreds
>    of emails ago
>
> 3. A new error will require a new code. Classes have the
>    advantage of becoming stable over time and we'll end up
>    just automatically reusing existing ones
>   

Yes, this is why I'm happy with the current state of QError.  We address 
all of these problems nicely :-)

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Luiz Capitulino]

On Mon, 23 Nov 2009 07:11:53 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> Luiz Capitulino wrote:
> > On Sun, 22 Nov 2009 10:08:16 -0600
> > Anthony Liguori <anthony@codemonkey.ws> wrote:
> >
> >   
> >> I'm certainly willing to consider alternative ways to do qmp_error() but 
> >> taking a free form string is not an option in my mind. It goes against 
> >> the fundamentals of what we're trying to build with QMP.
> >>     
> >
> >  Agreed.
> >
> >   
> >> So if you're opposed to structured error data, just having 
> >> qmp_error(error_code) is a reasonable alternative. I don't think it's 
> >> the right thing to do, but I think it's still within the spirit of the 
> >> goals of QMP.
> >>     
> >
> >  You mean, we would have calls like:
> >
> > qemu_error_new(error_code, 'device '%s' not found', name);
> >   
> 
> Except drop the 'device %s not found' bit.

 We would need a table to have the strings for the user protocol
then. Not having the table is a key point in Markus's argument,
I guess.

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Alexander Graf]

Am 23.11.2009 um 14:34 schrieb Luiz Capitulino <lcapitulino@redhat.com>:

> On Mon, 23 Nov 2009 07:11:53 -0600
> Anthony Liguori <anthony@codemonkey.ws> wrote:
>
>> Luiz Capitulino wrote:
>>> On Sun, 22 Nov 2009 10:08:16 -0600
>>> Anthony Liguori <anthony@codemonkey.ws> wrote:
>>>
>>>
>>>> I'm certainly willing to consider alternative ways to do qmp_error 
>>>> () but
>>>> taking a free form string is not an option in my mind. It goes  
>>>> against
>>>> the fundamentals of what we're trying to build with QMP.
>>>>
>>>
>>> Agreed.
>>>
>>>
>>>> So if you're opposed to structured error data, just having
>>>> qmp_error(error_code) is a reasonable alternative. I don't think  
>>>> it's
>>>> the right thing to do, but I think it's still within the spirit  
>>>> of the
>>>> goals of QMP.
>>>>
>>>
>>> You mean, we would have calls like:
>>>
>>> qemu_error_new(error_code, 'device '%s' not found', name);
>>>
>>
>> Except drop the 'device %s not found' bit.
>
> We would need a table to have the strings for the user protocol
> then. Not having the table is a key point in Markus's argument,
> I guess.

We would need that for internationalization anyways, right?

Though I don't dislike the idea of passing a plain default syntax over  
the wire as a bonus / fallback. Especially for cases where the ui is  
older than qemu.

So why not have the table inside qemu do a lookup and send that as  
default format in addition to the error code (that should usually be  
used to find out the format)?

Alex
>

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> Markus Armbruster wrote:
>> I'm thinking and talking primarily about the protocol, and that probably
>> makes me too terse on implementation.
>>
>> I didn't mean to suggest that for adding the data part we should add new
>> arguments providing the data.  That would be dumb indeed.
>>
>> Instead, I'd like to start with an extremely simple error reporting
>> mechanism, which requires an equally simple conversion, namely from
>> monitor_printf("human-readable error") to something of the form
>> qmp_error(error_code, "human-readable error").
>>   
>
> This is what I want to focus on because I think everything just boils
> down to this. Namely:
>
> qmp_error(error_code, "human-readable error");
>
> In my mind, the purpose of QMP is to provide a machine readable form
> of the monitor protocol. We have a monitor protocol today but because
> it's written as free-form text (often English sentences), it's
> difficult/impossible to parse which makes managing qemu extremely
> difficult. We have problems with escaping characters, use of
> non-deterministic grammars, etc.

Agreed.

> What I'm fundamentally opposed to is the "human-readable error" part
> of your proposal because it's free-form and unstructured. It has all
> the same problems as the current monitor interface. I'm opposed to
> this for the same reason I would be opposed to an "info balloon"
> command output of:
>
> {"data" : {"target" : "The guest has 32MB of memory"}}
>
> If all you were doing is outputting "info balloon" to a user in a
> command line tool, sure, this would be easier for the client to deal
> with. However, the whole purpose of QMP is to allow tools to make
> sense out of the monitor commands.
>
> The contents of an error are just as important as the output of a
> command.

Agreed.

>          The error code helps a bit but the problem with a qmp_error()
> that takes a string parameter is that there is additional information
> in that string that is inaccessible to a client.
>
> I'm certainly willing to consider alternative ways to do qmp_error()
> but taking a free form string is not an option in my mind. It goes
> against the fundamentals of what we're trying to build with QMP.
>
> So if you're opposed to structured error data, just having
> qmp_error(error_code) is a reasonable alternative. I don't think it's
> the right thing to do, but I think it's still within the spirit of the
> goals of QMP.

The error code is for machines, the free-form text is for humans.
There's ample precedence for this usage in existing protocols, e.g. RFC
2616 - Hypertext Transfer Protocol -- HTTP/1.1, section 6.1.1:

    6.1.1 Status Code and Reason Phrase

    The Status-Code element is a 3-digit integer result code of the
    attempt to understand and satisfy the request.  [...]  The
    Reason-Phrase is intended to give a short textual description of the
    Status-Code.  The Status-Code is intended for use by automata and
    the Reason-Phrase is intended for the human user.  The client is not
    required to examine or display the Reason- Phrase.

and RFC 959 - File Transfer Protocol:

    4.2.  FTP REPLIES

    [...]

    An FTP reply consists of a three digit number (transmitted as three
    alphanumeric characters) followed by some text.  The number is
    intended for use by automata to determine what state to enter next;
    the text is intended for the human user.  It is intended that the
    three digits contain enough encoded information that the
    user-process (the User-PI) will not need to examine the text and may
    either discard it or pass it on to the user, as appropriate.  In
    particular, the text may be server-dependent, so there are likely to
    be varying texts for each reply code.

> I think this is the fundamental thing to come to an agreement on in
> this discussion before we can even delve into the merits or lack
> thereof of structured error data.

First and foremost, I'd like to have reasonably specific error codes.
Give each distinct failure its own error code (it's not like there's a
shortage of codes).  Then, the error code should be enough information
for the client to handle the error sensibly.

I'd like to have a simple classification scheme for error codes, to help
clients handle errors they don't know.  Discussed elsewhere in this
thread.

In addition to the machine-readable error code, I'd like to include a
human-readable error message.  It is strictly for humans, not for
machines.  It helps humans interpret what goes on on the wire (useful
for debugging).  Some clients may find it useful to pass it on to their
human users, or log it.

I'm not fundamentally opposed to adding structured error data to that.
I just think doing it in the first iteration is unwise.  Of course, the
people writing and merging the patch are certainly free to do it anyway.

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Sat, 21 Nov 2009 11:02:41 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Now, what should a client do when it discovers the monitor command it
>> needs to use has grown a new error?  Refuse to use the command for fear
>> of having to present a sub-par error message to the user in case it
>> fails?
>
>  What's your solution for this problem?
>
>> Yes, the client needs updating in such a scenario.  But since it's a
>> normal, expected situation, I wouldn't call it a client bug.  And
>> regardless how we call it, we better handle it gracefully.
>
>  The best thing the client can do here is to print the error class.

In my opinion, the best such a client can do to report such an error to
a human user is to show the (localized) error class (assuming it knows
*that*, which is not a given), plus the (not localized) human-readable
message the server sent along with the unknown error code.  I'd expect
the latter part to be more useful in practice than the former.

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Luiz Capitulino]

On Mon, 23 Nov 2009 14:50:05 +0100
Alexander Graf <agraf@suse.de> wrote:

> 
> Am 23.11.2009 um 14:34 schrieb Luiz Capitulino <lcapitulino@redhat.com>:
> 
> > On Mon, 23 Nov 2009 07:11:53 -0600
> > Anthony Liguori <anthony@codemonkey.ws> wrote:
> >
> >> Luiz Capitulino wrote:
> >>> On Sun, 22 Nov 2009 10:08:16 -0600
> >>> Anthony Liguori <anthony@codemonkey.ws> wrote:
> >>>
> >>>
> >>>> I'm certainly willing to consider alternative ways to do qmp_error 
> >>>> () but
> >>>> taking a free form string is not an option in my mind. It goes  
> >>>> against
> >>>> the fundamentals of what we're trying to build with QMP.
> >>>>
> >>>
> >>> Agreed.
> >>>
> >>>
> >>>> So if you're opposed to structured error data, just having
> >>>> qmp_error(error_code) is a reasonable alternative. I don't think  
> >>>> it's
> >>>> the right thing to do, but I think it's still within the spirit  
> >>>> of the
> >>>> goals of QMP.
> >>>>
> >>>
> >>> You mean, we would have calls like:
> >>>
> >>> qemu_error_new(error_code, 'device '%s' not found', name);
> >>>
> >>
> >> Except drop the 'device %s not found' bit.
> >
> > We would need a table to have the strings for the user protocol
> > then. Not having the table is a key point in Markus's argument,
> > I guess.
> 
> We would need that for internationalization anyways, right?
> 
> Though I don't dislike the idea of passing a plain default syntax over  
> the wire as a bonus / fallback. Especially for cases where the ui is  
> older than qemu.
> 
> So why not have the table inside qemu do a lookup and send that as  
> default format in addition to the error code (that should usually be  
> used to find out the format)?

 The current proposal does have a table, the argument against it is
that we're making error reporting complex, as developers will have to
edit more than one place to report errors.

Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Alexander Graf]

On 24.11.2009, at 12:55, Luiz Capitulino wrote:

> On Mon, 23 Nov 2009 14:50:05 +0100
> Alexander Graf <agraf@suse.de> wrote:
> 
>> 
>> Am 23.11.2009 um 14:34 schrieb Luiz Capitulino <lcapitulino@redhat.com>:
>> 
>>> On Mon, 23 Nov 2009 07:11:53 -0600
>>> Anthony Liguori <anthony@codemonkey.ws> wrote:
>>> 
>>>> Luiz Capitulino wrote:
>>>>> On Sun, 22 Nov 2009 10:08:16 -0600
>>>>> Anthony Liguori <anthony@codemonkey.ws> wrote:
>>>>> 
>>>>> 
>>>>>> I'm certainly willing to consider alternative ways to do qmp_error 
>>>>>> () but
>>>>>> taking a free form string is not an option in my mind. It goes  
>>>>>> against
>>>>>> the fundamentals of what we're trying to build with QMP.
>>>>>> 
>>>>> 
>>>>> Agreed.
>>>>> 
>>>>> 
>>>>>> So if you're opposed to structured error data, just having
>>>>>> qmp_error(error_code) is a reasonable alternative. I don't think  
>>>>>> it's
>>>>>> the right thing to do, but I think it's still within the spirit  
>>>>>> of the
>>>>>> goals of QMP.
>>>>>> 
>>>>> 
>>>>> You mean, we would have calls like:
>>>>> 
>>>>> qemu_error_new(error_code, 'device '%s' not found', name);
>>>>> 
>>>> 
>>>> Except drop the 'device %s not found' bit.
>>> 
>>> We would need a table to have the strings for the user protocol
>>> then. Not having the table is a key point in Markus's argument,
>>> I guess.
>> 
>> We would need that for internationalization anyways, right?
>> 
>> Though I don't dislike the idea of passing a plain default syntax over  
>> the wire as a bonus / fallback. Especially for cases where the ui is  
>> older than qemu.
>> 
>> So why not have the table inside qemu do a lookup and send that as  
>> default format in addition to the error code (that should usually be  
>> used to find out the format)?
> 
> The current proposal does have a table, the argument against it is
> that we're making error reporting complex, as developers will have to
> edit more than one place to report errors.

So why not generate the table from the format strings? So in the code there would only be

qemu_error_new("device '%s' not found", name);

and some code parsing magic on commit would put that in the official table?


Alex