[Qemu-devel] [PATCH v0 0/9] QError

[Qemu-devel] [PATCH v0 0/9] QError

[Luiz Capitulino]

 This series adds infrastructure which will be needed by the future monitor
protocol to handle and emit error information correctly.

 QError is a QObject designed to stored error information in way that it can
be emitted by the protocol code or printed in an user friendly way.

 To use it in the Monitor and its handlers, I've added a function called
qemu_error_structed(), which builds on top of Gerd's QemuErrorSink API, this
means that it is also capable of printing error information to stderr.

 Basically, we have a QError member in the Monitor struct, which will be set
if an error has occurred. The Monitor's top level code will check it at
handler exit time and handle the error properly.

 QError represents an error, and it has the following properties:

- Error code
- Error general description
- Error specific data

 Code and description are stored in a static table in the QError module itself,
the error specific data is generated at error time by the function calling
qemu_error_structed().

 This design is based on the way I plan to emit error information in the
protocol, which is:

{ "error": { "code": json-number,
             "desc": json-string,
             "data": json-value } "id": json-value, "timestamp": json-string }

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

 Hopefully this is the last big piece of infrastructure before having the
protocol parser itself.

 Please, review carefully.

 Thanks.

PS: Applies on top of my last series.

[Qemu-devel] [PATCH 1/9] QDict: Introduce qdict_iter()

[Luiz Capitulino]

This adds iterator support to QDict, it will be used by the
(to be introduced) QError module.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qdict.c |   19 +++++++++++++++++++
 qdict.h |    3 +++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/qdict.c b/qdict.c
index a302f4c..0e04cb1 100644
--- a/qdict.c
+++ b/qdict.c
@@ -242,6 +242,25 @@ const char *qdict_get_try_str(const QDict *qdict, const char *key)
 }
 
 /**
+ * qdict_iter(): Iterate over all the dictionary's stored values.
+ *
+ * This function allows the user to provide an iterator, which will be
+ * called for each stored value in the dictionary.
+ */
+void qdict_iter(const QDict *qdict,
+                void (*iter)(const char *key, QObject *obj, void *opaque),
+                void *opaque)
+{
+    int i;
+    QDictEntry *entry;
+
+    for (i = 0; i < QDICT_HASH_SIZE; i++) {
+        QLIST_FOREACH(entry, &qdict->table[i], next)
+            iter(entry->key, entry->value, opaque);
+    }
+}
+
+/**
  * qentry_destroy(): Free all the memory allocated by a QDictEntry
  */
 static void qentry_destroy(QDictEntry *e)
diff --git a/qdict.h b/qdict.h
index 3102ca2..14b2633 100644
--- a/qdict.h
+++ b/qdict.h
@@ -27,6 +27,9 @@ void qdict_del(QDict *qdict, const char *key);
 int qdict_haskey(const QDict *qdict, const char *key);
 QObject *qdict_get(const QDict *qdict, const char *key);
 QDict *qobject_to_qdict(const QObject *obj);
+void qdict_iter(const QDict *qdict,
+                void (*iter)(const char *key, QObject *obj, void *opaque),
+                void *opaque);
 
 /* Helper to qdict_put_obj(), accepts any object */
 #define qdict_put(qdict, key, obj) \
-- 
1.6.5.rc3.8.g8ba5e

[Qemu-devel] [PATCH 2/9] check-qdict: Add test for qdict_iter()

[Luiz Capitulino]

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

diff --git a/check-qdict.c b/check-qdict.c
index c37d448..c80ff22 100644
--- a/check-qdict.c
+++ b/check-qdict.c
@@ -191,6 +191,40 @@ START_TEST(qobject_to_qdict_test)
 }
 END_TEST
 
+static int iter_called;
+static const int iter_max = 42;
+
+static void iter_func(const char *key, QObject *obj, void *opaque)
+{
+    QInt *qi;
+
+    fail_unless(key != NULL);
+    fail_unless(strstr(key, "key_") != NULL);
+    fail_unless(opaque == NULL),
+
+    qi = qobject_to_qint(obj);
+    fail_unless(qi != NULL);
+    fail_unless((qint_get_int(qi) >= 0) && (qint_get_int(qi) <= iter_max));
+
+    iter_called++;
+}
+
+START_TEST(qdict_iter_test)
+{
+    int i;
+
+    for (i = 0; i < iter_max; i++) {
+        char key[12];
+        sprintf(key, "key_%d", i);
+        qdict_put(tests_dict, key, qint_from_int(i));
+    }
+
+    iter_called = 0;
+    qdict_iter(tests_dict, iter_func, NULL);
+    fail_unless(iter_called == iter_max, "%d\n", iter_called);
+}
+END_TEST
+
 /*
  * Errors test-cases
  */
@@ -333,6 +367,7 @@ static Suite *qdict_suite(void)
     tcase_add_test(qdict_public2_tcase, qdict_haskey_test);
     tcase_add_test(qdict_public2_tcase, qdict_del_test);
     tcase_add_test(qdict_public2_tcase, qobject_to_qdict_test);
+    tcase_add_test(qdict_public2_tcase, qdict_iter_test);
 
     qdict_errors_tcase = tcase_create("Errors");
     suite_add_tcase(s, qdict_errors_tcase);
-- 
1.6.5.rc3.8.g8ba5e

[Qemu-devel] [PATCH 3/9] qmisc: Introduce qobject_from_va()

[Luiz Capitulino]

It will be used by the (to be introduced) QError module.

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

diff --git a/qmisc.c b/qmisc.c
index 42b6f22..2bca278 100644
--- a/qmisc.c
+++ b/qmisc.c
@@ -172,6 +172,36 @@ static QObject *build_qobject(const char **fmt, va_list *args)
 }
 
 /**
+ * qobject_from_va(): Create a QObject from the specified va_list
+ *
+ * Same as qobject_from_fmt(), but with a va_list argument.
+ */
+QObject *qobject_from_va(const char *format, va_list va)
+{
+    va_list lva;
+    QObject *obj;
+    const char *fmt = format;
+
+    va_copy(lva, va);
+
+    switch (*fmt) {
+        case '[':
+            fmt++;
+            obj = do_mklist(&fmt, &lva, ']', count_format(fmt, ']'));
+            break;
+        case '{':
+            fmt++;
+            obj = do_mkdict(&fmt, &lva, '}', count_format(fmt, '}'));
+            break;
+        default:
+            obj = build_qobject(&fmt, &lva);
+            break;
+    }
+
+    return obj;
+}
+
+/**
  * qobject_from_fmt(): build QObjects from a specified format.
  *
  * Valid characters of the format:
@@ -203,19 +233,7 @@ QObject *qobject_from_fmt(const char *fmt, ...)
     QObject *obj;
 
     va_start(args, fmt);
-    switch (*fmt) {
-        case '[':
-            fmt++;
-            obj = do_mklist(&fmt, &args, ']', count_format(fmt, ']'));
-            break;
-        case '{':
-            fmt++;
-            obj = do_mkdict(&fmt, &args, '}', count_format(fmt, '}'));
-            break;
-        default:
-            obj = build_qobject(&fmt, &args);
-            break;
-    }
+    obj = qobject_from_va(fmt, args);
     va_end(args);
 
     return obj;
diff --git a/qmisc.h b/qmisc.h
index ac481fe..16070f9 100644
--- a/qmisc.h
+++ b/qmisc.h
@@ -12,8 +12,10 @@
 #ifndef QMISC_H
 #define QMISC_H
 
+#include <stdarg.h>
 #include "qobject.h"
 
+QObject *qobject_from_va(const char *format, va_list va);
 QObject *qobject_from_fmt(const char *fmt, ...);
 
 #endif /* QMISC_H */
-- 
1.6.5.rc3.8.g8ba5e

Re: [Qemu-devel] [PATCH 3/9] qmisc: Introduce qobject_from_va()

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> It will be used by the (to be introduced) QError module.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  qmisc.c |   44 +++++++++++++++++++++++++++++++-------------
>  qmisc.h |    2 ++
>  2 files changed, 33 insertions(+), 13 deletions(-)
>
> diff --git a/qmisc.c b/qmisc.c
> index 42b6f22..2bca278 100644
> --- a/qmisc.c
> +++ b/qmisc.c
> @@ -172,6 +172,36 @@ static QObject *build_qobject(const char **fmt, va_list *args)
>  }
>  
>  /**
> + * qobject_from_va(): Create a QObject from the specified va_list
> + *
> + * Same as qobject_from_fmt(), but with a va_list argument.
> + */
> +QObject *qobject_from_va(const char *format, va_list va)
> +{
> +    va_list lva;
> +    QObject *obj;
> +    const char *fmt = format;
> +
> +    va_copy(lva, va);


Why copy?  Just curious...

[...]

Re: [Qemu-devel] [PATCH 3/9] qmisc: Introduce qobject_from_va()

[Luiz Capitulino]

On Tue, 13 Oct 2009 23:52:18 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > It will be used by the (to be introduced) QError module.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  qmisc.c |   44 +++++++++++++++++++++++++++++++-------------
> >  qmisc.h |    2 ++
> >  2 files changed, 33 insertions(+), 13 deletions(-)
> >
> > diff --git a/qmisc.c b/qmisc.c
> > index 42b6f22..2bca278 100644
> > --- a/qmisc.c
> > +++ b/qmisc.c
> > @@ -172,6 +172,36 @@ static QObject *build_qobject(const char **fmt, va_list *args)
> >  }
> >  
> >  /**
> > + * qobject_from_va(): Create a QObject from the specified va_list
> > + *
> > + * Same as qobject_from_fmt(), but with a va_list argument.
> > + */
> > +QObject *qobject_from_va(const char *format, va_list va)
> > +{
> > +    va_list lva;
> > +    QObject *obj;
> > +    const char *fmt = format;
> > +
> > +    va_copy(lva, va);
> 
> 
> Why copy?  Just curious...

 Because I'm using va_arg(), not sure if matters though.

[Qemu-devel] Re: [PATCH 3/9] qmisc: Introduce qobject_from_va()

[Paolo Bonzini]

>>> +QObject *qobject_from_va(const char *format, va_list va)
>>> +{
>>> +    va_list lva;
>>> +    QObject *obj;
>>> +    const char *fmt = format;
>>> +
>>> +    va_copy(lva, va);
>>
>>
>> Why copy?  Just curious...
>
>   Because I'm using va_arg(), not sure if matters though.

No, you do not need that.  The va_arg will be destructive, but anyway 
you're not going to do more than one pass on the va_list.

Paolo

[Qemu-devel] [PATCH 4/9] Introduce QError

[Luiz Capitulino]

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

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

1. Code, which should be added to enum QErrorCode first
2. Description, the general error cause
3. Prefix, usually subsystem's name
4. A print function

The prefix and print function are optional. If no print function is
specified the default one will be used, which prints the prefix
before printing the error description.

Note that the default function is very simple and a deep level of
data structure nesting has not been tested.

QError exports the following functions:

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

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

diff --git a/Makefile b/Makefile
index e60eb4b..1c1ee86 100644
--- a/Makefile
+++ b/Makefile
@@ -125,7 +125,7 @@ obj-y += buffered_file.o migration.o migration-tcp.o net.o qemu-sockets.o
 obj-y += qemu-char.o aio.o net-checksum.o savevm.o
 obj-y += msmouse.o ps2.o
 obj-y += qdev.o qdev-properties.o
-obj-y += qint.o qstring.o qdict.o qlist.o qmisc.o qemu-config.o
+obj-y += qint.o qstring.o qdict.o qlist.o qmisc.o qerror.o qemu-config.o
 
 obj-$(CONFIG_BRLAPI) += baum.o
 obj-$(CONFIG_WIN32) += tap-win32.o
diff --git a/qerror.c b/qerror.c
new file mode 100644
index 0000000..bbea770
--- /dev/null
+++ b/qerror.c
@@ -0,0 +1,189 @@
+/*
+ * QError: QEMU Error data-type.
+ *
+ * Copyright (C) 2009 Red Hat Inc.
+ *
+ * Authors:
+ *  Luiz Capitulino <lcapitulino@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ */
+#include "sysemu.h"
+#include "qint.h"
+#include "qmisc.h"
+#include "qlist.h"
+#include "qerror.h"
+#include "qstring.h"
+#include "qemu-common.h"
+
+static void qerror_destroy_obj(QObject *obj);
+
+static const QType qerror_type = {
+    .code = QTYPE_QERROR,
+    .destroy = qerror_destroy_obj,
+};
+
+static QErrorTable qerror_table[] = {
+    {
+        .code   = QERR_UNKNOWN,
+        .desc   = "unknown error",
+    },
+};
+
+/**
+ * qerror_new(): Create a new QError
+ *
+ * Return strong reference.
+ */
+QError *qerror_new(void)
+{
+    QError *qerr;
+
+    qerr = qemu_mallocz(sizeof(*qerr));
+    QOBJECT_INIT(qerr, &qerror_type);
+
+    return qerr;
+}
+
+/*
+ * qerror_from_va(): Create a new QError from a va_list
+ *
+ * The specified format and va_list are used to build the specific
+ * error data object, which is stored in the 'data' member of QError.
+ *
+ * Return strong reference.
+ */
+QError *qerror_from_va(QErrorCode code, const char *fmt, va_list va)
+{
+    QError *qerr;
+
+    assert((code > 0) && (code < QERR_MAX));
+
+    qerr = qerror_new();
+    qerr->entry = &qerror_table[code];
+    if (fmt) {
+        qerr->data = qobject_from_va(fmt, va);
+        if (!qerr->data) {
+            qemu_free(qerr);
+            return NULL;
+        }
+    }
+
+    return qerr;
+}
+
+static int show_dict_sep;
+static int show_list_sep;
+static void qerror_qobject_print(const QObject *obj);
+
+static void print_qdict(const char *key, QObject *obj, void *opaque)
+{
+    if (show_dict_sep)
+        qemu_error(",");
+
+    qemu_error("%s=", key);
+    qerror_qobject_print(obj);
+
+    show_dict_sep = 1;
+}
+
+static void print_qlist(QObject *obj, void *opaque)
+{
+    if (show_list_sep)
+        qemu_error(",");
+
+    qerror_qobject_print(obj);
+
+    show_list_sep = 1;
+}
+
+static void qerror_qobject_print(const QObject *obj)
+{
+    if (!obj)
+        return;
+
+    switch (qobject_type(obj)) {
+        case QTYPE_QINT:
+            qemu_error("%" PRId64, qint_get_int(qobject_to_qint(obj)));
+            break;
+        case QTYPE_QSTRING:
+            qemu_error("%s", qstring_get_str(qobject_to_qstring(obj)));
+            break;
+        case QTYPE_QDICT:
+            show_dict_sep = 0;
+            qdict_iter(qobject_to_qdict(obj), print_qdict, NULL);
+            break;
+        case QTYPE_QLIST:
+            show_list_sep = 0;
+            qlist_iter(qobject_to_qlist(obj), print_qlist, NULL);
+            break;
+        default:
+            abort();
+            break;
+    }
+}
+
+/**
+ * qerror_default_print(): Standard function to print QError data
+ */
+static void qerror_default_print(const QError *qerror)
+{
+    const QErrorTable *entry = qerror->entry;
+
+    if (entry->prefix)
+        qemu_error("%s: ", entry->prefix);
+
+    qemu_error("%s", entry->desc);
+    if (qerror->data) {
+        qemu_error(": ");
+        qerror_qobject_print(qerror->data);
+    }
+
+    qemu_error("\n");
+}
+
+/**
+ * qerror_print(): Print QError data
+ *
+ * This function calls the user_print() callback associated
+ * with the error to print QError data in human readable
+ * format.
+ *
+ * If the error does not define a user_print() callback, the
+ * standard one will be called.
+ */
+void qerror_print(const QError *qerror)
+{
+    if (qerror->entry->user_print) {
+        qerror->entry->user_print(qerror);
+    } else {
+        qerror_default_print(qerror);
+    }
+}
+
+/**
+ * qobject_to_qerror(): Convert a QObject into a QError
+ */
+QError *qobject_to_qerror(const QObject *obj)
+{
+    if (qobject_type(obj) != QTYPE_QERROR) {
+        return NULL;
+    }
+
+    return container_of(obj, QError, base);
+}
+
+/**
+ * qerror_destroy_obj(): Free all memory allocated by a QError
+ */
+static void qerror_destroy_obj(QObject *obj)
+{
+    QError *qerr;
+
+    assert(obj != NULL);
+    qerr = qobject_to_qerror(obj);
+
+    qobject_decref(qerr->data);
+    qemu_free(qerr);
+}
diff --git a/qerror.h b/qerror.h
new file mode 100644
index 0000000..ed25ef1
--- /dev/null
+++ b/qerror.h
@@ -0,0 +1,47 @@
+/*
+ * QError header file.
+ *
+ * Copyright (C) 2009 Red Hat Inc.
+ *
+ * Authors:
+ *  Luiz Capitulino <lcapitulino@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ */
+#ifndef QERROR_H
+#define QERROR_H
+
+#include <stdarg.h>
+#include "qobject.h"
+
+/*
+ * IMPORTANT: errors numbers must not change after they have been
+ * added here.
+ */
+typedef enum QErrorCode {
+    QERR_UNKNOWN,
+    QERR_MAX,
+} QErrorCode;
+
+struct QError;
+
+typedef struct QErrorTable {
+    QErrorCode code;
+    const char *prefix;
+    const char *desc;
+    void (*user_print)(const struct QError *qerror);
+} QErrorTable;
+
+typedef struct QError {
+    QObject_HEAD;
+    QObject *data; /* error specific data */
+    const QErrorTable *entry;
+} QError;
+
+QError *qerror_new(void);
+QError *qerror_from_va(QErrorCode code, const char *fmt, va_list va);
+QError *qobject_to_qerror(const QObject *obj);
+void qerror_print(const QError *qerror);
+
+#endif /* QERROR_H */
diff --git a/qobject.h b/qobject.h
index 4cc9287..484d4dd 100644
--- a/qobject.h
+++ b/qobject.h
@@ -41,6 +41,7 @@ typedef enum {
     QTYPE_QSTRING,
     QTYPE_QDICT,
     QTYPE_QLIST,
+    QTYPE_QERROR,
 } qtype_code;
 
 struct QObject;
-- 
1.6.5.rc3.8.g8ba5e

[Qemu-devel] [PATCH 5/9] monitor: QError support

[Luiz Capitulino]

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

Additionally, it introduces qemu_error_structed() which should
be used by functions that are called by monitor handlers and
print error information.

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

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

1. An error happens in the handler, it calls qemu_error_structed()
2. qemu_error_structed() builds the right QObjects with the error
   information and stores them 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 |   44 +++++++++++++++++++++++++++++++++++++++++++-
 sysemu.h  |    2 ++
 2 files changed, 45 insertions(+), 1 deletions(-)

diff --git a/monitor.c b/monitor.c
index 39d3201..6f0ad11 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;
 };
@@ -3148,6 +3150,18 @@ fail:
     return NULL;
 }
 
+static inline int monitor_has_error(const Monitor *mon)
+{
+    return mon->error != NULL;
+}
+
+static void monitor_print_error(Monitor *mon)
+{
+    qerror_print(mon->error);
+    QDECREF(mon->error);
+    mon->error = NULL;
+}
+
 static void monitor_handle_command(Monitor *mon, const char *cmdline)
 {
     QDict *qdict;
@@ -3173,7 +3187,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);
@@ -3624,3 +3641,28 @@ void qemu_error(const char *fmt, ...)
         break;
     }
 }
+
+void qemu_error_structed(QErrorCode code, const char *fmt, ...)
+{
+    va_list va;
+    QError *qerror;
+
+    assert(qemu_error_sink != NULL);
+
+    va_start(va, fmt);
+    qerror = qerror_from_va(code, fmt, va);
+    va_end(va);
+
+    assert(qerror != NULL);
+
+    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 896916f..02247fe 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>
@@ -70,6 +71,7 @@ int qemu_loadvm_state(QEMUFile *f);
 void qemu_errors_to_file(FILE *fp);
 void qemu_errors_to_mon(Monitor *mon);
 void qemu_errors_to_previous(void);
+void qemu_error_structed(QErrorCode code, const char *fmt, ...);
 void qemu_error(const char *fmt, ...) __attribute__ ((format(printf, 1, 2)));
 
 #ifdef _WIN32
-- 
1.6.5.rc3.8.g8ba5e

Re: [Qemu-devel] [PATCH 5/9] monitor: QError support

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> This commit paves the way for QError support in the Monitor,
> it adds a QError member to the Monitor struct and functions
> to check and print it.
>
> Additionally, it introduces qemu_error_structed() which should
> be used by functions that are called by monitor handlers and
> print error information.
>
> This new function has to be used in place of qemu_error(), which
> will become private when all the conversion is done.
>
> Basically, the Monitor's error flow is something like this:
>
> 1. An error happens in the handler, it calls qemu_error_structed()
> 2. qemu_error_structed() builds the right QObjects with the error
>    information and stores them 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 896916f..02247fe 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>
> @@ -70,6 +71,7 @@ int qemu_loadvm_state(QEMUFile *f);
>  void qemu_errors_to_file(FILE *fp);
>  void qemu_errors_to_mon(Monitor *mon);
>  void qemu_errors_to_previous(void);
> +void qemu_error_structed(QErrorCode code, const char *fmt, ...);

Needs __attribute__((format(printf, 2, 3))).

>  void qemu_error(const char *fmt, ...) __attribute__ ((format(printf, 1, 2)));
>  
>  #ifdef _WIN32

[Qemu-devel] Re: [PATCH 5/9] monitor: QError support

[Paolo Bonzini]

On 10/13/2009 11:59 PM, Markus Armbruster wrote:
>> >  +void qemu_error_structed(QErrorCode code, const char *fmt, ...);
> Needs __attribute__((format(printf, 2, 3))).

If I read the code correctly, qemu_object_from_va is what in the end 
processes the valist and it is not exactly compatible with printf 
syntax.  For example, in patch 7/9 you have

      qemu_error_structed(QERR_QDEV_NFOUND, "{ s: s }", "name", driver);

Paolo

[Qemu-devel] Re: [PATCH 5/9] monitor: QError support

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 10/13/2009 11:59 PM, Markus Armbruster wrote:
>>> >  +void qemu_error_structed(QErrorCode code, const char *fmt, ...);
>> Needs __attribute__((format(printf, 2, 3))).
>
> If I read the code correctly, qemu_object_from_va is what in the end
> processes the valist and it is not exactly compatible with printf
> syntax.  For example, in patch 7/9 you have
>
>      qemu_error_structed(QERR_QDEV_NFOUND, "{ s: s }", "name", driver);

You're right, of course.

Pity we lose the checking.

[Qemu-devel] [PATCH 6/9] QError: Add qdev not found error

[Luiz Capitulino]

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

diff --git a/qerror.c b/qerror.c
index bbea770..88a8208 100644
--- a/qerror.c
+++ b/qerror.c
@@ -24,11 +24,23 @@ static const QType qerror_type = {
     .destroy = qerror_destroy_obj,
 };
 
+static void qemu_err_qdev_nodev(const QError *qerror)
+{
+    QDict *qdict = qobject_to_qdict(qerror->data);
+    qemu_error("Device \"%s\" not found.  Try -device '?' for a list.\n",
+               qdict_get_str(qdict, "name"));
+}
+
 static QErrorTable qerror_table[] = {
     {
         .code   = QERR_UNKNOWN,
         .desc   = "unknown error",
     },
+    {
+        .code   = QERR_QDEV_NFOUND,
+        .desc   = "device not found",
+        .user_print = qemu_err_qdev_nodev,
+    },
 };
 
 /**
diff --git a/qerror.h b/qerror.h
index ed25ef1..820f25e 100644
--- a/qerror.h
+++ b/qerror.h
@@ -21,6 +21,7 @@
  */
 typedef enum QErrorCode {
     QERR_UNKNOWN,
+    QERR_QDEV_NFOUND,
     QERR_MAX,
 } QErrorCode;
 
-- 
1.6.5.rc3.8.g8ba5e

Re: [Qemu-devel] [PATCH 6/9] QError: Add qdev not found error

[Hollis Blanchard]

On Tue, 2009-10-13 at 13:57 -0300, Luiz Capitulino wrote:
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  qerror.c |   12 ++++++++++++
>  qerror.h |    1 +
>  2 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/qerror.c b/qerror.c
> index bbea770..88a8208 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -24,11 +24,23 @@ static const QType qerror_type = {
>      .destroy = qerror_destroy_obj,
>  };
> 
> +static void qemu_err_qdev_nodev(const QError *qerror)
> +{
> +    QDict *qdict = qobject_to_qdict(qerror->data);
> +    qemu_error("Device \"%s\" not found.  Try -device '?' for a list.\n",
> +               qdict_get_str(qdict, "name"));
> +}
> +
>  static QErrorTable qerror_table[] = {
>      {
>          .code   = QERR_UNKNOWN,
>          .desc   = "unknown error",
>      },
> +    {
> +        .code   = QERR_QDEV_NFOUND,
> +        .desc   = "device not found",
> +        .user_print = qemu_err_qdev_nodev,
> +    },
>  };
> 
>  /**
> diff --git a/qerror.h b/qerror.h
> index ed25ef1..820f25e 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -21,6 +21,7 @@
>   */
>  typedef enum QErrorCode {
>      QERR_UNKNOWN,
> +    QERR_QDEV_NFOUND,
>      QERR_MAX,
>  } QErrorCode;

I'm not really seeing the point: what is gained by moving the error text
from the original site into this function-inside-a-structure?

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [Qemu-devel] [PATCH 6/9] QError: Add qdev not found error

[Luiz Capitulino]

On Wed, 14 Oct 2009 16:02:10 -0700
Hollis Blanchard <hollisb@us.ibm.com> wrote:

> On Tue, 2009-10-13 at 13:57 -0300, Luiz Capitulino wrote:
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  qerror.c |   12 ++++++++++++
> >  qerror.h |    1 +
> >  2 files changed, 13 insertions(+), 0 deletions(-)
> > 
> > diff --git a/qerror.c b/qerror.c
> > index bbea770..88a8208 100644
> > --- a/qerror.c
> > +++ b/qerror.c
> > @@ -24,11 +24,23 @@ static const QType qerror_type = {
> >      .destroy = qerror_destroy_obj,
> >  };
> > 
> > +static void qemu_err_qdev_nodev(const QError *qerror)
> > +{
> > +    QDict *qdict = qobject_to_qdict(qerror->data);
> > +    qemu_error("Device \"%s\" not found.  Try -device '?' for a list.\n",
> > +               qdict_get_str(qdict, "name"));
> > +}
> > +
> >  static QErrorTable qerror_table[] = {
> >      {
> >          .code   = QERR_UNKNOWN,
> >          .desc   = "unknown error",
> >      },
> > +    {
> > +        .code   = QERR_QDEV_NFOUND,
> > +        .desc   = "device not found",
> > +        .user_print = qemu_err_qdev_nodev,
> > +    },
> >  };
> > 
> >  /**
> > diff --git a/qerror.h b/qerror.h
> > index ed25ef1..820f25e 100644
> > --- a/qerror.h
> > +++ b/qerror.h
> > @@ -21,6 +21,7 @@
> >   */
> >  typedef enum QErrorCode {
> >      QERR_UNKNOWN,
> > +    QERR_QDEV_NFOUND,
> >      QERR_MAX,
> >  } QErrorCode;
> 
> I'm not really seeing the point: what is gained by moving the error text
> from the original site into this function-inside-a-structure?

 Compatibility and a way of having pretty printing functions w/o
breaking the machine protocol.

Re: [Qemu-devel] [PATCH 6/9] QError: Add qdev not found error

[Hollis Blanchard]

On Thu, 2009-10-15 at 10:34 -0300, Luiz Capitulino wrote:
> On Wed, 14 Oct 2009 16:02:10 -0700
> Hollis Blanchard <hollisb@us.ibm.com> wrote:
> 
> > On Tue, 2009-10-13 at 13:57 -0300, Luiz Capitulino wrote:
> > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > > ---
> > >  qerror.c |   12 ++++++++++++
> > >  qerror.h |    1 +
> > >  2 files changed, 13 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/qerror.c b/qerror.c
> > > index bbea770..88a8208 100644
> > > --- a/qerror.c
> > > +++ b/qerror.c
> > > @@ -24,11 +24,23 @@ static const QType qerror_type = {
> > >      .destroy = qerror_destroy_obj,
> > >  };
> > > 
> > > +static void qemu_err_qdev_nodev(const QError *qerror)
> > > +{
> > > +    QDict *qdict = qobject_to_qdict(qerror->data);
> > > +    qemu_error("Device \"%s\" not found.  Try -device '?' for a list.\n",
> > > +               qdict_get_str(qdict, "name"));
> > > +}
> > > +
> > >  static QErrorTable qerror_table[] = {
> > >      {
> > >          .code   = QERR_UNKNOWN,
> > >          .desc   = "unknown error",
> > >      },
> > > +    {
> > > +        .code   = QERR_QDEV_NFOUND,
> > > +        .desc   = "device not found",
> > > +        .user_print = qemu_err_qdev_nodev,
> > > +    },
> > >  };
> > > 
> > >  /**
> > > diff --git a/qerror.h b/qerror.h
> > > index ed25ef1..820f25e 100644
> > > --- a/qerror.h
> > > +++ b/qerror.h
> > > @@ -21,6 +21,7 @@
> > >   */
> > >  typedef enum QErrorCode {
> > >      QERR_UNKNOWN,
> > > +    QERR_QDEV_NFOUND,
> > >      QERR_MAX,
> > >  } QErrorCode;
> > 
> > I'm not really seeing the point: what is gained by moving the error text
> > from the original site into this function-inside-a-structure?
> 
>  Compatibility and a way of having pretty printing functions w/o
> breaking the machine protocol.

Huh? Compatibility with what? I don't understand this "pretty printing"
comment either.

You could easily have
	qemu_error(code, "device not found");
throughout the code, and transmit that as
	{ "error": { "code": code, "desc": "device not found" } }

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [Qemu-devel] [PATCH 6/9] QError: Add qdev not found error

[Luiz Capitulino]

On Thu, 15 Oct 2009 10:16:00 -0700
Hollis Blanchard <hollisb@us.ibm.com> wrote:

> On Thu, 2009-10-15 at 10:34 -0300, Luiz Capitulino wrote:
> > On Wed, 14 Oct 2009 16:02:10 -0700
> > Hollis Blanchard <hollisb@us.ibm.com> wrote:
> > 
> > > On Tue, 2009-10-13 at 13:57 -0300, Luiz Capitulino wrote:
> > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > > > ---
> > > >  qerror.c |   12 ++++++++++++
> > > >  qerror.h |    1 +
> > > >  2 files changed, 13 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/qerror.c b/qerror.c
> > > > index bbea770..88a8208 100644
> > > > --- a/qerror.c
> > > > +++ b/qerror.c
> > > > @@ -24,11 +24,23 @@ static const QType qerror_type = {
> > > >      .destroy = qerror_destroy_obj,
> > > >  };
> > > > 
> > > > +static void qemu_err_qdev_nodev(const QError *qerror)
> > > > +{
> > > > +    QDict *qdict = qobject_to_qdict(qerror->data);
> > > > +    qemu_error("Device \"%s\" not found.  Try -device '?' for a list.\n",
> > > > +               qdict_get_str(qdict, "name"));
> > > > +}
> > > > +
> > > >  static QErrorTable qerror_table[] = {
> > > >      {
> > > >          .code   = QERR_UNKNOWN,
> > > >          .desc   = "unknown error",
> > > >      },
> > > > +    {
> > > > +        .code   = QERR_QDEV_NFOUND,
> > > > +        .desc   = "device not found",
> > > > +        .user_print = qemu_err_qdev_nodev,
> > > > +    },
> > > >  };
> > > > 
> > > >  /**
> > > > diff --git a/qerror.h b/qerror.h
> > > > index ed25ef1..820f25e 100644
> > > > --- a/qerror.h
> > > > +++ b/qerror.h
> > > > @@ -21,6 +21,7 @@
> > > >   */
> > > >  typedef enum QErrorCode {
> > > >      QERR_UNKNOWN,
> > > > +    QERR_QDEV_NFOUND,
> > > >      QERR_MAX,
> > > >  } QErrorCode;
> > > 
> > > I'm not really seeing the point: what is gained by moving the error text
> > > from the original site into this function-inside-a-structure?
> > 
> >  Compatibility and a way of having pretty printing functions w/o
> > breaking the machine protocol.
> 
> Huh? Compatibility with what?

 With existing errors. I'm avoiding changing them because existing
applications which parse QEMU output may rely on them.

 On the other hand, I'm not sure if this is a hard requirement.

> I don't understand this "pretty printing" comment either.

 The structured error output can sometimes be too rigid for humans,
this qdev error is an example. When we fail we pass the 'Try -device'
hint, which doesn't make sense for the protocol.

 Also, it's not uncommon to have error strings like this:

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

 Which is what I call 'pretty printing'.
 
> You could easily have
> 	qemu_error(code, "device not found");

 This doesn't solve the problems I mentioned above, besides I don't
see why you need to specify both, the error code and the description,
they describe the same thing.

 Also, they must not change after the protocol gets into production,
having them defined in the same place will help.

Re: [Qemu-devel] [PATCH 6/9] QError: Add qdev not found error

[Hollis Blanchard]

On Thu, 2009-10-15 at 14:52 -0300, Luiz Capitulino wrote:
> 
> > You could easily have
> >       qemu_error(code, "device not found");
> 
>  This doesn't solve the problems I mentioned above, besides I don't
> see why you need to specify both, the error code and the description,
> they describe the same thing.

This is how FTP and other protocols work, for example.

You supplied a perfect example:
	qemu_error(404, "husb: host usb device %d.%d is already open\n",
                      bus_num, addr);

In this case, an interactive client could only display the "pretty"
text.

A machine client would interpret the code as Markus described in a
previous mail (temporary server error, permanent server error, etc). In
case of a permanent server error, the client could *also* pass through
the pretty text to display to the user.

>  Also, they must not change after the protocol gets into production,
> having them defined in the same place will help.

This seems really unnecessary and too much abstraction.

Aside from that, we should certainly be able to change the pretty text,
for example, to provide additional clarification to the user. The
machine-interpreted code, on the other hand, wouldn't change.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [Qemu-devel] [PATCH 6/9] QError: Add qdev not found error

[Luiz Capitulino]

On Thu, 15 Oct 2009 11:13:53 -0700
Hollis Blanchard <hollisb@us.ibm.com> wrote:

> On Thu, 2009-10-15 at 14:52 -0300, Luiz Capitulino wrote:
> > 
> > > You could easily have
> > >       qemu_error(code, "device not found");
> > 
> >  This doesn't solve the problems I mentioned above, besides I don't
> > see why you need to specify both, the error code and the description,
> > they describe the same thing.
> 
> This is how FTP and other protocols work, for example.

 I meant in the qemu_error() call, we will certainly emit them both
in the protocol.

> You supplied a perfect example:
> 	qemu_error(404, "husb: host usb device %d.%d is already open\n",
>                       bus_num, addr);
> 
> In this case, an interactive client could only display the "pretty"
> text.
> 
> A machine client would interpret the code as Markus described in a
> previous mail (temporary server error, permanent server error, etc). In
> case of a permanent server error, the client could *also* pass through
> the pretty text to display to the user.

 Ok, so this design is really different from what I had in mind.

 In your design error codes are more like errors classes, right? So,
using your example, 404 could have the meaning 'already open, temporary',
this way it can be used with USB, PCI or even for files.

 How do you plan to emit it on the wire? Like this:

{ "error": { "code": 404,
             "desc": "husb: host usb device 0.12 is already open" } }

 In case I'm right, there are two problems with it:

1. It's hard to retrieve the variable information
2. Maybe minor, but I see too much information for a protocol

 In the design I'm proposing, *each* different error will get a code,
no problem to have classes of errors though, but the point is that
error text doesn't go on the call to qemu_error().

 The above example can be emitted like this:

{ "error": { "code": 12
             "desc": "device already open",
             "data": { "bus": 0, "address": 12 } } }

 Note that this also can be reused by any bus, as the "data" information
is built at error time and can contain anything.

> >  Also, they must not change after the protocol gets into production,
> > having them defined in the same place will help.
> 
> This seems really unnecessary and too much abstraction.

 I don't agree it's unnecessary, but I agree it has too much abstraction
and making the errors harder to report will make developers tentative
in not reporting errors at all (Markus argument).

 Maybe we need clearer requirements?

> Aside from that, we should certainly be able to change the pretty text,
> for example, to provide additional clarification to the user. The
> machine-interpreted code, on the other hand, wouldn't change.

 How do you plan to do it in the design you're proposing? If you
use the text from qemu_error() to build the user and protocol
outputs, then you can't change it.

 In current QError this is possible.

Re: [Qemu-devel] [PATCH 6/9] QError: Add qdev not found error

[Hollis Blanchard]

On Thu, 2009-10-15 at 16:08 -0300, Luiz Capitulino wrote:
> On Thu, 15 Oct 2009 11:13:53 -0700
> Hollis Blanchard <hollisb@us.ibm.com> wrote:
> 
> > On Thu, 2009-10-15 at 14:52 -0300, Luiz Capitulino wrote:
> > > 
> > > > You could easily have
> > > >       qemu_error(code, "device not found");
> > > 
> > >  This doesn't solve the problems I mentioned above, besides I don't
> > > see why you need to specify both, the error code and the description,
> > > they describe the same thing.
> > 
> > This is how FTP and other protocols work, for example.
> 
>  I meant in the qemu_error() call, we will certainly emit them both
> in the protocol.
> 
> > You supplied a perfect example:
> > 	qemu_error(404, "husb: host usb device %d.%d is already open\n",
> >                       bus_num, addr);
> > 
> > In this case, an interactive client could only display the "pretty"
> > text.
> > 
> > A machine client would interpret the code as Markus described in a
> > previous mail (temporary server error, permanent server error, etc). In
> > case of a permanent server error, the client could *also* pass through
> > the pretty text to display to the user.
> 
>  Ok, so this design is really different from what I had in mind.

I'm starting to see that. :)

>  In your design error codes are more like errors classes, right? So,
> using your example, 404 could have the meaning 'already open, temporary',
> this way it can be used with USB, PCI or even for files.
> 
>  How do you plan to emit it on the wire? Like this:
> 
> { "error": { "code": 404,
>              "desc": "husb: host usb device 0.12 is already open" } }
> 
>  In case I'm right, there are two problems with it:
> 
> 1. It's hard to retrieve the variable information
> 2. Maybe minor, but I see too much information for a protocol

I'm really confused by your objections. This is simply HTTP-style
"Status Code" and "Reason
Phrase" (http://tools.ietf.org/html/rfc2616#section-6.1.1) in JSON form.

1. The client receiving the response clearly has a JSON interpreter, so
how is it hard to retrieve anything?
2. Have you ever used software that said "Failed." without explanation?
Unless you're suggesting that all clients would build in their own
code->description tables (I hope to god you're not :), we must transmit
the description in the protocol.

> > Aside from that, we should certainly be able to change the pretty text,
> > for example, to provide additional clarification to the user. The
> > machine-interpreted code, on the other hand, wouldn't change.
> 
>  How do you plan to do it in the design you're proposing? If you
> use the text from qemu_error() to build the user and protocol
> outputs, then you can't change it.

Today it looks like this:
        C: open host USB device foo
        S: error 404, host USB device foo is already open

Tomorrow it could look like this:
        C: open host USB device foo
        S: error 404, host USB device foo is already open by PID 27

As described in HTTP's section 6.1.1:
   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.

Since the automata only interprets the status code, nothing breaks, and
the user benefits from more information.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [Qemu-devel] [PATCH 6/9] QError: Add qdev not found error

[Anthony Liguori]

>>> Aside from that, we should certainly be able to change the pretty text,
>>> for example, to provide additional clarification to the user. The
>>> machine-interpreted code, on the other hand, wouldn't change.
>>>       
>>  How do you plan to do it in the design you're proposing? If you
>> use the text from qemu_error() to build the user and protocol
>> outputs, then you can't change it.
>>     
>
> Today it looks like this:
>         C: open host USB device foo
>         S: error 404, host USB device foo is already open
>
> Tomorrow it could look like this:
>         C: open host USB device foo
>         S: error 404, host USB device foo is already open by PID 27
>   

What's tough about this sort of error handling is that it's not 
conducive to localization.  It's not unusual for the server to have a 
different locale than the client so you really need the client to be 
able to translate error messages into meaningful messages in the client 
locale.

This is the typical argument for highly structured error reporting.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 6/9] QError: Add qdev not found error

[Hollis Blanchard]

On Thu, 2009-10-15 at 15:57 -0500, Anthony Liguori wrote:
> >>> Aside from that, we should certainly be able to change the pretty text,
> >>> for example, to provide additional clarification to the user. The
> >>> machine-interpreted code, on the other hand, wouldn't change.
> >>>       
> >>  How do you plan to do it in the design you're proposing? If you
> >> use the text from qemu_error() to build the user and protocol
> >> outputs, then you can't change it.
> >>     
> >
> > Today it looks like this:
> >         C: open host USB device foo
> >         S: error 404, host USB device foo is already open
> >
> > Tomorrow it could look like this:
> >         C: open host USB device foo
> >         S: error 404, host USB device foo is already open by PID 27
> >   
> 
> What's tough about this sort of error handling is that it's not 
> conducive to localization.  It's not unusual for the server to have a 
> different locale than the client so you really need the client to be 
> able to translate error messages into meaningful messages in the client 
> locale.

It's hard enough to keep localized strings updated for every release
within a single code tree. Beyond that, you expect that every *client*
will update its localized strings for every *server* release? If you
agree that is an unrealistic expectation, then even a "highly
structured" reporting mechanism won't help.

On the other hand, if you truly want to invest the energy required to
localize qemu (the server), I can imagine setting the per-client locale
as a monitor command.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [Qemu-devel] [PATCH 6/9] QError: Add qdev not found error

[Anthony Liguori]

hollisb@linux.vnet.ibm.com wrote:
> It's hard enough to keep localized strings updated for every release
> within a single code tree. Beyond that, you expect that every *client*
> will update its localized strings for every *server* release?

What I meant by highly structured is that you wouldn't pass a string at 
all.  There would just be an error code and whatever information went 
along with that error code.  It would be up to the client to decide how 
to present a message to the user.

 From a usability perspective, this is better both for localization but 
also to ensure a consistent user experience with respect to how errors 
are described to a user.

Basically, I think the conventional wisdom in UI design is that you 
never want to pass error messages from a  server directly to a user.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 6/9] QError: Add qdev not found error

[Hollis Blanchard]

On Thu, 2009-10-15 at 16:27 -0500, Anthony Liguori wrote:
> hollisb@linux.vnet.ibm.com wrote:
> > It's hard enough to keep localized strings updated for every release
> > within a single code tree. Beyond that, you expect that every *client*
> > will update its localized strings for every *server* release?
> 
> What I meant by highly structured is that you wouldn't pass a string at 
> all.  There would just be an error code and whatever information went 
> along with that error code.  It would be up to the client to decide how 
> to present a message to the user.
> 
>  From a usability perspective, this is better both for localization but 
> also to ensure a consistent user experience with respect to how errors 
> are described to a user.
> 
> Basically, I think the conventional wisdom in UI design is that you 
> never want to pass error messages from a  server directly to a user.

I think it's also conventional wisdom not to show the user vague "some
error occurred" messages. :)

How about this (basically what Paolo suggested):

{ "error": { "code": 12,
             "desc": "device %{bus}:%{address} already open",
             "data": { "bus": 0, "address": 12 } } }

'desc' *may* be used by the client, or may be replaced with a localized
version. This ensures that users need never be confronted with an
"Error: 12" dialog box; a client can at least display some (English)
information for unknown errors.

I am now seeing how Luiz got to where he did with the original patch:
once you have unique error codes, why embed the format string at the
call site? (I think this would have been a better patch description in
the first place, rather than unspecified "compatibility".)

However, I think the part that's still bothering me is the user_print
function pointer. It seems to me that there should be no difference
between the text sent to a remote client and the text displayed to a
local user, and if that were true it would remove some ugliness.

-- 
Hollis Blanchard
IBM Linux Technology Center

[Qemu-devel] Re: [PATCH 6/9] QError: Add qdev not found error

[Paolo Bonzini]

On 10/16/2009 12:44 AM, Hollis Blanchard wrote:
> How about this (basically what Paolo suggested):
>
> { "error": { "code": 12,
>               "desc": "device %{bus}:%{address} already open",
>               "data": { "bus": 0, "address": 12 } } }
>
> 'desc'*may*  be used by the client, or may be replaced with a localized
> version.

I would say that desc need not go on the wire too.  The client might not 
even want to show the same string to the user, for example they may want 
to say "mouse already" open.

The "device %{bus}:%{address} already open" would be strictly inside 
QEMU, for consumption of the monitor interface.  Of course since the 
server is in QEMU too it makes sense to consolidate it in the same 
struct, but this does not mean that everything in the struct needs to go 
on the wire.

Paolo

[Qemu-devel] Re: [PATCH 6/9] QError: Add qdev not found error

[Luiz Capitulino]

On Fri, 16 Oct 2009 10:06:10 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 10/16/2009 12:44 AM, Hollis Blanchard wrote:
> > How about this (basically what Paolo suggested):
> >
> > { "error": { "code": 12,
> >               "desc": "device %{bus}:%{address} already open",
> >               "data": { "bus": 0, "address": 12 } } }
> >
> > 'desc'*may*  be used by the client, or may be replaced with a localized
> > version.
> 
> I would say that desc need not go on the wire too.  The client might not 
> even want to show the same string to the user, for example they may want 
> to say "mouse already" open.
> 
> The "device %{bus}:%{address} already open" would be strictly inside 
> QEMU, for consumption of the monitor interface.  Of course since the 
> server is in QEMU too it makes sense to consolidate it in the same 
> struct, but this does not mean that everything in the struct needs to go 
> on the wire.

 This is what my current proposal does, "desc" goes on the wire
because it's a simple description but messages for users consumption
are printed by .user_print.

 I think we could make this very simple if we use a solution along
the lines suggested by Hollis and do _not_ allow variable information
(ie. 'handler data') to go on the wire.

 I mean, we could let current errors as they are but add error codes
and new error descriptions to be used by the protocol only.

 For example, a call to:

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

 Would become:

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

 When in user protocol the message is printed normally, when in protocol
mode the error code is used to index the error table and on the wire
we emit:

{ "error": { "code": 404, "desc": "device already open" } }

 Now I need to know if it's ok to have such simple error information
on the wire.

Re: [Qemu-devel] Re: [PATCH 6/9] QError: Add qdev not found error

[Daniel P. Berrange]

On Fri, Oct 16, 2009 at 10:05:44AM -0300, Luiz Capitulino wrote:
> On Fri, 16 Oct 2009 10:06:10 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > On 10/16/2009 12:44 AM, Hollis Blanchard wrote:
> > > How about this (basically what Paolo suggested):
> > >
> > > { "error": { "code": 12,
> > >               "desc": "device %{bus}:%{address} already open",
> > >               "data": { "bus": 0, "address": 12 } } }
> > >
> > > 'desc'*may*  be used by the client, or may be replaced with a localized
> > > version.
> > 
> > I would say that desc need not go on the wire too.  The client might not 
> > even want to show the same string to the user, for example they may want 
> > to say "mouse already" open.
> > 
> > The "device %{bus}:%{address} already open" would be strictly inside 
> > QEMU, for consumption of the monitor interface.  Of course since the 
> > server is in QEMU too it makes sense to consolidate it in the same 
> > struct, but this does not mean that everything in the struct needs to go 
> > on the wire.
> 
>  This is what my current proposal does, "desc" goes on the wire
> because it's a simple description but messages for users consumption
> are printed by .user_print.
> 
>  I think we could make this very simple if we use a solution along
> the lines suggested by Hollis and do _not_ allow variable information
> (ie. 'handler data') to go on the wire.
> 
>  I mean, we could let current errors as they are but add error codes
> and new error descriptions to be used by the protocol only.
> 
>  For example, a call to:
> 
> monitor_printf(mon, "husb: host usb device %d.%d is already open\n",
>                       bus_num, addr);
> 
>  Would become:
> 
> qemu_error_structed(404, "husb: host usb device %d.%d is already open\n",
>                           bus_num, addr);
> 
>  When in user protocol the message is printed normally, when in protocol
> mode the error code is used to index the error table and on the wire
> we emit:
> 
> { "error": { "code": 404, "desc": "device already open" } }
> 
>  Now I need to know if it's ok to have such simple error information
> on the wire.

In so much as its providing an error code + message I think that is
sufficient, but I think we should take care to ensure that the error
description contains enough information to allow useful troubleshooting.
As such doing a simple index lookup from error code -> message is not
sufficient, because it would be throwing away potentially important
error data.

Consider a user files a bug report attaching client app logs which
show the monitor command issued and the it error returned. Ask yourself,
is that enough information to locate where in the code the error came
from & can you make a reasonable attempt at identifying a root cause. 

In your example above, the usb_add command already included the bus_num
+ addr, so there'd be no need to also include that data in the reply.
But consider when QEMU actually tries to open the host USB device, there
are easily 10-20 different things that could go wrong, and many even
simple system calls like open() could generate many different errors.
It is important that this fine detail be reported back to the client
app for developers to have a good attempt at troubleshooting

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] Re: [PATCH 6/9] QError: Add qdev not found error

[Luiz Capitulino]

On Mon, 19 Oct 2009 11:25:19 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Fri, Oct 16, 2009 at 10:05:44AM -0300, Luiz Capitulino wrote:
> > On Fri, 16 Oct 2009 10:06:10 +0200
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> > > On 10/16/2009 12:44 AM, Hollis Blanchard wrote:
> > > > How about this (basically what Paolo suggested):
> > > >
> > > > { "error": { "code": 12,
> > > >               "desc": "device %{bus}:%{address} already open",
> > > >               "data": { "bus": 0, "address": 12 } } }
> > > >
> > > > 'desc'*may*  be used by the client, or may be replaced with a localized
> > > > version.
> > > 
> > > I would say that desc need not go on the wire too.  The client might not 
> > > even want to show the same string to the user, for example they may want 
> > > to say "mouse already" open.
> > > 
> > > The "device %{bus}:%{address} already open" would be strictly inside 
> > > QEMU, for consumption of the monitor interface.  Of course since the 
> > > server is in QEMU too it makes sense to consolidate it in the same 
> > > struct, but this does not mean that everything in the struct needs to go 
> > > on the wire.
> > 
> >  This is what my current proposal does, "desc" goes on the wire
> > because it's a simple description but messages for users consumption
> > are printed by .user_print.
> > 
> >  I think we could make this very simple if we use a solution along
> > the lines suggested by Hollis and do _not_ allow variable information
> > (ie. 'handler data') to go on the wire.
> > 
> >  I mean, we could let current errors as they are but add error codes
> > and new error descriptions to be used by the protocol only.
> > 
> >  For example, a call to:
> > 
> > monitor_printf(mon, "husb: host usb device %d.%d is already open\n",
> >                       bus_num, addr);
> > 
> >  Would become:
> > 
> > qemu_error_structed(404, "husb: host usb device %d.%d is already open\n",
> >                           bus_num, addr);
> > 
> >  When in user protocol the message is printed normally, when in protocol
> > mode the error code is used to index the error table and on the wire
> > we emit:
> > 
> > { "error": { "code": 404, "desc": "device already open" } }
> > 
> >  Now I need to know if it's ok to have such simple error information
> > on the wire.
> 
> In so much as its providing an error code + message I think that is
> sufficient, but I think we should take care to ensure that the error
> description contains enough information to allow useful troubleshooting.
> As such doing a simple index lookup from error code -> message is not
> sufficient, because it would be throwing away potentially important
> error data.

 Yes, it's going to throw away important info. We could have a big enough
table with all possible errors, but this seems insane at first.

 Another solution could be to change the semantics of the error data,
instead of passing to it information which is already there we could pass
additional info which is not part of the original message.

 On the other hand, I think this will make the usage of qemu_error_structed()
a bit confusing.

> Consider a user files a bug report attaching client app logs which
> show the monitor command issued and the it error returned. Ask yourself,
> is that enough information to locate where in the code the error came
> from & can you make a reasonable attempt at identifying a root cause. 
> 
> In your example above, the usb_add command already included the bus_num
> + addr, so there'd be no need to also include that data in the reply.
> But consider when QEMU actually tries to open the host USB device, there
> are easily 10-20 different things that could go wrong, and many even
> simple system calls like open() could generate many different errors.
> It is important that this fine detail be reported back to the client
> app for developers to have a good attempt at troubleshooting

 I agree, although there is going to be a huge effort in the conversion
work, as I think error handling in QEMU is not that good.

Re: [Qemu-devel] Re: [PATCH 6/9] QError: Add qdev not found error

[Daniel P. Berrange]

On Mon, Oct 19, 2009 at 10:28:17AM -0200, Luiz Capitulino wrote:
> On Mon, 19 Oct 2009 11:25:19 +0100
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
> 
> > On Fri, Oct 16, 2009 at 10:05:44AM -0300, Luiz Capitulino wrote:
> > > On Fri, 16 Oct 2009 10:06:10 +0200
> > > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > 
> > > > On 10/16/2009 12:44 AM, Hollis Blanchard wrote:
> > > > > How about this (basically what Paolo suggested):
> > > > >
> > > > > { "error": { "code": 12,
> > > > >               "desc": "device %{bus}:%{address} already open",
> > > > >               "data": { "bus": 0, "address": 12 } } }
> > > > >
> > > > > 'desc'*may*  be used by the client, or may be replaced with a localized
> > > > > version.
> > > > 
> > > > I would say that desc need not go on the wire too.  The client might not 
> > > > even want to show the same string to the user, for example they may want 
> > > > to say "mouse already" open.
> > > > 
> > > > The "device %{bus}:%{address} already open" would be strictly inside 
> > > > QEMU, for consumption of the monitor interface.  Of course since the 
> > > > server is in QEMU too it makes sense to consolidate it in the same 
> > > > struct, but this does not mean that everything in the struct needs to go 
> > > > on the wire.
> > > 
> > >  This is what my current proposal does, "desc" goes on the wire
> > > because it's a simple description but messages for users consumption
> > > are printed by .user_print.
> > > 
> > >  I think we could make this very simple if we use a solution along
> > > the lines suggested by Hollis and do _not_ allow variable information
> > > (ie. 'handler data') to go on the wire.
> > > 
> > >  I mean, we could let current errors as they are but add error codes
> > > and new error descriptions to be used by the protocol only.
> > > 
> > >  For example, a call to:
> > > 
> > > monitor_printf(mon, "husb: host usb device %d.%d is already open\n",
> > >                       bus_num, addr);
> > > 
> > >  Would become:
> > > 
> > > qemu_error_structed(404, "husb: host usb device %d.%d is already open\n",
> > >                           bus_num, addr);
> > > 
> > >  When in user protocol the message is printed normally, when in protocol
> > > mode the error code is used to index the error table and on the wire
> > > we emit:
> > > 
> > > { "error": { "code": 404, "desc": "device already open" } }
> > > 
> > >  Now I need to know if it's ok to have such simple error information
> > > on the wire.
> > 
> > In so much as its providing an error code + message I think that is
> > sufficient, but I think we should take care to ensure that the error
> > description contains enough information to allow useful troubleshooting.
> > As such doing a simple index lookup from error code -> message is not
> > sufficient, because it would be throwing away potentially important
> > error data.
> 
>  Yes, it's going to throw away important info. We could have a big enough
> table with all possible errors, but this seems insane at first.
> 
>  Another solution could be to change the semantics of the error data,
> instead of passing to it information which is already there we could pass
> additional info which is not part of the original message.
> 
>  On the other hand, I think this will make the usage of qemu_error_structed()
> a bit confusing.

Why not include all of it, the error code, the generic string for the
error code, and then the formatted specific error string the monitor
currently has. eg

  { "error": { "code": 404,
               "desc": "device already open",
               "detail": "husb: host usb device 001.003 is already open" } }

Since the latter error messages all already exist, it should make it easier
to adapt, and allow clients flexibility in how they report/handle the 
errors whether to show the detail error to users, or just the generic msg

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 :|

[Qemu-devel] Re: [PATCH 6/9] QError: Add qdev not found error

[Anthony Liguori]

Paolo Bonzini wrote:
> On 10/16/2009 12:44 AM, Hollis Blanchard wrote:
>> How about this (basically what Paolo suggested):
>>
>> { "error": { "code": 12,
>>               "desc": "device %{bus}:%{address} already open",
>>               "data": { "bus": 0, "address": 12 } } }
>>
>> 'desc'*may*  be used by the client, or may be replaced with a localized
>> version.
>
> I would say that desc need not go on the wire too.  The client might 
> not even want to show the same string to the user, for example they 
> may want to say "mouse already" open.
>
> The "device %{bus}:%{address} already open" would be strictly inside 
> QEMU, for consumption of the monitor interface.  Of course since the 
> server is in QEMU too it makes sense to consolidate it in the same 
> struct, but this does not mean that everything in the struct needs to 
> go on the wire.

Agreed.  It could also go in a client library and we could provide 
something equivalent to strerror().  The advantage of putting this in a 
client library is that we could potentially localize it.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 6/9] QError: Add qdev not found error

[Jamie Lokier]

Anthony Liguori wrote:
> hollisb@linux.vnet.ibm.com wrote:
> >It's hard enough to keep localized strings updated for every release
> >within a single code tree. Beyond that, you expect that every *client*
> >will update its localized strings for every *server* release?
> 
> What I meant by highly structured is that you wouldn't pass a string at 
> all.  There would just be an error code and whatever information went 
> along with that error code.  It would be up to the client to decide how 
> to present a message to the user.

The main problem with that is when you add a new error, several things
need to be added in different places, including in clients, so it
discourages adding new errors and existing ones are abused.

Down that road lies "Error, something failed", as well as "File not
found" for things which aren't files.

The practice of providing a string at the call site encourages
messages to be informative and accurate first, with localisation and
error-specific actions added later as needed.

The manual for GNU gettext explains quite well why gettext takes a
message string as argument, instead of a "message code".  Imho, a
similar case can be made for error messages at call sites.

> From a usability perspective, this is better both for localization but 
> also to ensure a consistent user experience with respect to how errors 
> are described to a user.

It's not great for localisation if N clients all maintain separate,
different sets of localisation for the same messages, especially if
the set of messages is changing regularly too from one qemu version to
the next.

It's also not a consistent user experience, if each client has a
different message for the same obscure errors.

Only the most widely used clients are likely to get localised, and
even those will get out of date quickly, e.g. when using a distro's
client with a newer-than-distry qemu.

> Basically, I think the conventional wisdom in UI design is that you 
> never want to pass error messages from a  server directly to a user.

You can probably guess I don't agree with that.  I'm not sure if it is
actually conventional wisdom, either.  It happens a lot in popular
things with highly refined UIs like web browsers, FTP clients and the
like.  They tend to _add_ their own explanation to the server-provided
error messages, but show the server's message too, especially for
uncommon error types that the client doesn't know about (because it
can't know them all).

-- Jamie

[Qemu-devel] Re: [PATCH 6/9] QError: Add qdev not found error

[Paolo Bonzini]

On 10/18/2009 06:25 AM, Jamie Lokier wrote:
> The manual for GNU gettext explains quite well why gettext takes a
> message string as argument, instead of a "message code".  Imho, a
> similar case can be made for error messages at call sites.

That's true.  However here we have the case of having errors consumed by 
programs as well as users, so we want something that can be easily made 
into language bindings.

In other words, this situation is much more similar to errno/strerror, 
than to a compiler error message (which will often be used by other 
programs, but where the actual error text will be read by a person; this 
can and should use gettext).

Paolo

[Qemu-devel] Re: [PATCH 6/9] QError: Add qdev not found error

[Hollis Blanchard]

On Sun, 2009-10-18 at 14:17 +0200, Paolo Bonzini wrote:
> On 10/18/2009 06:25 AM, Jamie Lokier wrote:
> > The manual for GNU gettext explains quite well why gettext takes a
> > message string as argument, instead of a "message code".  Imho, a
> > similar case can be made for error messages at call sites.
> 
> That's true.  However here we have the case of having errors consumed by 
> programs as well as users, so we want something that can be easily made 
> into language bindings.
> 
> In other words, this situation is much more similar to errno/strerror, 
> than to a compiler error message (which will often be used by other 
> programs, but where the actual error text will be read by a person; this 
> can and should use gettext).

So to extend your analogy, I think we're reaching the conclusion that
both errno *and* string should be returned to the client, right? The
client can localize based on errno, and in addition optionally provide
the server-provided string to the user.

-- 
Hollis Blanchard
IBM Linux Technology Center

[Qemu-devel] Re: [PATCH 6/9] QError: Add qdev not found error

[Paolo Bonzini]

On 10/19/2009 06:50 PM, Hollis Blanchard wrote:
> On Sun, 2009-10-18 at 14:17 +0200, Paolo Bonzini wrote:
>> On 10/18/2009 06:25 AM, Jamie Lokier wrote:
>>> The manual for GNU gettext explains quite well why gettext takes a
>>> message string as argument, instead of a "message code".  Imho, a
>>> similar case can be made for error messages at call sites.
>>
>> That's true.  However here we have the case of having errors consumed by
>> programs as well as users, so we want something that can be easily made
>> into language bindings.
>>
>> In other words, this situation is much more similar to errno/strerror,
>> than to a compiler error message (which will often be used by other
>> programs, but where the actual error text will be read by a person; this
>> can and should use gettext).
>
> So to extend your analogy, I think we're reaching the conclusion that
> both errno *and* string should be returned to the client, right?

I'd say so, yes.  And any parameters too, all as JSON.  I think errno is 
just one kind of data that can be in an exception, and you want 
different exceptions for not opening a sound device or not opening an 
image file (even if both may fail with EACCES or ENOENT).

Paolo

Re: [Qemu-devel] [PATCH 6/9] QError: Add qdev not found error

[Gerd Hoffmann]

   Hi,

> { "error": { "code": 404,
>               "desc": "husb: host usb device 0.12 is already open" } }
>
>   In case I'm right, there are two problems with it:
>
> 1. It's hard to retrieve the variable information

Markus asked it already, I'm asking again:

*Do we actually need the variable information?*

The point is that the information usually is available from context 
anyway, i.e. if you get the reply above from a command like 'usb_add 
host:0.12' you know it is host device 0.12 *without* parsing reply 
because you know you've asked for host device 0.12.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 6/9] QError: Add qdev not found error

[Luiz Capitulino]

On Fri, 16 Oct 2009 09:30:35 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>    Hi,
> 
> > { "error": { "code": 404,
> >               "desc": "husb: host usb device 0.12 is already open" } }
> >
> >   In case I'm right, there are two problems with it:
> >
> > 1. It's hard to retrieve the variable information
> 
> Markus asked it already, I'm asking again:
> 
> *Do we actually need the variable information?*

 That's a good question and to answer it properly I think we should
have to go over all errors and check if any of them generate
information which is not available from the context.

 There is also the compatibility issue, I'm not sure if it's ok
to change the error output, we would have to check at least the
majors QEMU users.

[Qemu-devel] Re: [PATCH 6/9] QError: Add qdev not found error

[Paolo Bonzini]

On 10/16/2009 02:39 PM, Luiz Capitulino wrote:
>   That's a good question and to answer it properly I think we should
> have to go over all errors and check if any of them generate
> information which is not available from the context.

As more communication (including errors) becomes asynchronous, the 
probability of this happening will quickly go to 1.

I think we just need good QObject printf/scanf like functions.

Paolo

Re: [Qemu-devel] [PATCH 6/9] QError: Add qdev not found error

[Anthony Liguori]

Gerd Hoffmann wrote:
>   Hi,
>
>> { "error": { "code": 404,
>>               "desc": "husb: host usb device 0.12 is already open" } }
>>
>>   In case I'm right, there are two problems with it:
>>
>> 1. It's hard to retrieve the variable information
>
> Markus asked it already, I'm asking again:
>
> *Do we actually need the variable information?*
>
> The point is that the information usually is available from context 
> anyway, i.e. if you get the reply above from a command like 'usb_add 
> host:0.12' you know it is host device 0.12 *without* parsing reply 
> because you know you've asked for host device 0.12.

I would recommend treating errors like exceptions.  In fact, if I were 
writing a python client library, this is exactly how I would model it.

In fact, it would make sense to switch code to some sort of class name.  
This would be the exception class.  The data argument would be the 
exception object.  desc would basically be __str__() method although 
more limited.

Very often, exceptions have very little data associated with them.  
Sometimes it's useful to include a bit of info though.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 6/9] QError: Add qdev not found error

[Luiz Capitulino]

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

> Gerd Hoffmann wrote:
> >   Hi,
> >
> >> { "error": { "code": 404,
> >>               "desc": "husb: host usb device 0.12 is already open" } }
> >>
> >>   In case I'm right, there are two problems with it:
> >>
> >> 1. It's hard to retrieve the variable information
> >
> > Markus asked it already, I'm asking again:
> >
> > *Do we actually need the variable information?*
> >
> > The point is that the information usually is available from context 
> > anyway, i.e. if you get the reply above from a command like 'usb_add 
> > host:0.12' you know it is host device 0.12 *without* parsing reply 
> > because you know you've asked for host device 0.12.
> 
> I would recommend treating errors like exceptions.  In fact, if I were 
> writing a python client library, this is exactly how I would model it.
> 
> In fact, it would make sense to switch code to some sort of class name.  
> This would be the exception class.  The data argument would be the 
> exception object.  desc would basically be __str__() method although 
> more limited.

 This seems useful (and a lot of C projects end up creating their
"error object" anyway), but I'm getting concerned.

 First, and more important, we have our immediate needs. Which is to have
most of the protocol code merged in 15 days.

 Second, I fear we are going too far with the objects idea. It
solves the monitor's problem well and although can be used by
other subsystems I wonder if pushing them to the extreme like that
is the way to go.

[Qemu-devel] Re: [PATCH 6/9] QError: Add qdev not found error

[Paolo Bonzini]

On 10/16/2009 04:17 PM, Luiz Capitulino wrote:
>   Second, I fear we are going too far with the objects idea. It
> solves the monitor's problem well and although can be used by
> other subsystems I wonder if pushing them to the extreme like that
> is the way to go.

I think that in practice Anthony is asking you only to write the 
stringified enum name (e.g. "QEMU_ERROR_NODEV"), instead of the enum 
value (i.e. 2). :-)

I agree about what the immediate needs are.  On the other hand, making 
things too complicated to extend later is not a good idea...

Paolo, who's amazed at the amount of design changes that went into QEMU 
since last June...

[Qemu-devel] Re: [PATCH 6/9] QError: Add qdev not found error

[Anthony Liguori]

Paolo Bonzini wrote:
> On 10/16/2009 04:17 PM, Luiz Capitulino wrote:
>>   Second, I fear we are going too far with the objects idea. It
>> solves the monitor's problem well and although can be used by
>> other subsystems I wonder if pushing them to the extreme like that
>> is the way to go.
>
> I think that in practice Anthony is asking you only to write the 
> stringified enum name (e.g. "QEMU_ERROR_NODEV"), instead of the enum 
> value (i.e. 2). :-)

Yup.

> I agree about what the immediate needs are.  On the other hand, making 
> things too complicated to extend later is not a good idea...

There are just a few things we have to get right and we should take our 
time on those.  The error semantics are one of those important things 
that we'll regret in the future if we rush.

-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 6/9] QError: Add qdev not found error

[Paolo Bonzini]

On 10/15/2009 09:08 PM, Luiz Capitulino wrote:
> { "error": { "code": 12
>               "desc": "device already open",
>               "data": { "bus": 0, "address": 12 } } }
>
>   Note that this also can be reused by any bus, as the "data" information
> is built at error time and can contain anything.

The "desc" is not even necessary on the wire.

Paolo

Re: [Qemu-devel] Re: [PATCH 6/9] QError: Add qdev not found error

[Jamie Lokier]

Paolo Bonzini wrote:
> On 10/15/2009 09:08 PM, Luiz Capitulino wrote:
> >{ "error": { "code": 12
> >              "desc": "device already open",
> >              "data": { "bus": 0, "address": 12 } } }
> >
> >  Note that this also can be reused by any bus, as the "data" information
> >is built at error time and can contain anything.
> 
> The "desc" is not even necessary on the wire.

When you send an error code that that client doesn't know yet (because
you can't update every client immediately), it'll be very helpful to
users to see "device already open" instead of "unknown error 12".

-- Jamie

Re: [Qemu-devel] Re: [PATCH 6/9] QError: Add qdev not found error

[Jamie Lokier]

Jamie Lokier wrote:
> Paolo Bonzini wrote:
> > On 10/15/2009 09:08 PM, Luiz Capitulino wrote:
> > >{ "error": { "code": 12
> > >              "desc": "device already open",
> > >              "data": { "bus": 0, "address": 12 } } }
> > >
> > >  Note that this also can be reused by any bus, as the "data" information
> > >is built at error time and can contain anything.
> > 
> > The "desc" is not even necessary on the wire.
> 
> When you send an error code that that client doesn't know yet (because
> you can't update every client immediately), it'll be very helpful to
> users to see "device already open" instead of "unknown error 12".

About that technique in general.  It works much better when the client
and server are managed together, for example as a single project, or
by the same people working on both.

Then you can keep the client's set of error codes in sync with the
server in every version.

But that's not possible when there are umpteen clients maintained by
other people on their own schedule, each used by users who may combine
them with newer servers.  Which I gather is something that the new
monitor protocol is intended to support.

-- Jamie

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

[Luiz Capitulino]

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

diff --git a/hw/qdev.c b/hw/qdev.c
index 906e897..3ce48f7 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -28,6 +28,7 @@
 #include "net.h"
 #include "qdev.h"
 #include "sysemu.h"
+#include "qerror.h"
 #include "monitor.h"
 
 static int qdev_hotplug = 0;
@@ -176,8 +177,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     /* find driver */
     info = qdev_find_info(NULL, driver);
     if (!info) {
-        qemu_error("Device \"%s\" not found.  Try -device '?' for a list.\n",
-                   driver);
+        qemu_error_structed(QERR_QDEV_NFOUND, "{ s: s }", "name", driver);
         return NULL;
     }
     if (info->no_user) {
-- 
1.6.5.rc3.8.g8ba5e

Re: [Qemu-devel] [PATCH 7/9] qdev: Use QError for 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 906e897..3ce48f7 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -28,6 +28,7 @@
>  #include "net.h"
>  #include "qdev.h"
>  #include "sysemu.h"
> +#include "qerror.h"
>  #include "monitor.h"
>  
>  static int qdev_hotplug = 0;
> @@ -176,8 +177,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>      /* find driver */
>      info = qdev_find_info(NULL, driver);
>      if (!info) {
> -        qemu_error("Device \"%s\" not found.  Try -device '?' for a list.\n",
> -                   driver);
> +        qemu_error_structed(QERR_QDEV_NFOUND, "{ s: s }", "name", driver);
>          return NULL;
>      }
>      if (info->no_user) {

And here we pay the price for structured error objects: reporting an
error becomes more difficult.  The harder you make reporting errors, the
fewer errors get caught and reported.  Also, the result is harder to
read.

If we need the structure, then it's just a price we have to pay.  Do we
need it?

In the example above, it gets us two pieces of information in addition
to the "monitor command failed" bit: the error code, which tells us the
exact kind of failure, and a data object to go with the error code, in
this case containing the name of the device.

Now let's look at errors from the client's perspective.

Clients need to classify errors to figure out how to respond to them.
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).

Some classical protocols (HTTP, FTP) provide error class (they cleverly
encode it into the error code).  This gives clients a chance to sanely
handle errors they don't know.

Even with error class figured out, clients may still be interested in
the exact error code, at least in some cases.

They may also be interested in a human readable description of the
error, to present to their human users.  Some classical protocols
provide that in addition to an error code.  This gives clients a chance
to sanely report errors they don't know to human users.

I suspect that the additional error data is the error's least
interesting part most of the time.  When we get QERR_QDEV_NFOUND, I
figure it's usually clear what device we were trying to find.

But these are just my educated guesses.  I'd love to hear what folks
involved with actual clients have to say.  Anyone from libvirt?

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

[Paolo Bonzini]

On 10/14/2009 12:34 AM, Markus Armbruster 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 906e897..3ce48f7 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -28,6 +28,7 @@
>>   #include "net.h"
>>   #include "qdev.h"
>>   #include "sysemu.h"
>> +#include "qerror.h"
>>   #include "monitor.h"
>>
>>   static int qdev_hotplug = 0;
>> @@ -176,8 +177,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>       /* find driver */
>>       info = qdev_find_info(NULL, driver);
>>       if (!info) {
>> -        qemu_error("Device \"%s\" not found.  Try -device '?' for a list.\n",
>> -                   driver);
>> +        qemu_error_structed(QERR_QDEV_NFOUND, "{ s: s }", "name", driver);
>>           return NULL;
>>       }
>>       if (info->no_user) {
>
> And here we pay the price for structured error objects: reporting an
> error becomes more difficult.  The harder you make reporting errors, the
> fewer errors get caught and reported.  Also, the result is harder to
> read.

If you count also the new code in 6/9, reporting an error definitely 
becomes harder.  Finding a middle ground would be great, and I think 
that the right solution is to put as many things as possible in a single 
place.  For example:

1) there is a lot of code that is duplicated in every occurrence of an 
error.  One possibility to avoid this, would be to use a more 
printf-like syntax for qemu_object_from_va, for example one of these:

      .... "{ name: %s }", driver);
      .... "{ 'name': %s }", driver);

Then you can move the first argument to the error table:

+        .code   = QERR_QDEV_NFOUND,
+        .desc   = "device not found",
+        .keys   = "{ name: %s }"

so that the qemu_error_structed call looks like

     qemu_error_structed (QERR_QDEV_NFOUND, driver);

2) do we need full flexibility of a function for printing errors?  What 
about adding a print-from-qobject function, so that you could replace 
qerror_table[].user_print with a string like

          .formatted_print = "Device \"%{name}\" not found.  Try -device"
                             " '?' for a list."

3) I don't think this technique is used in qemu, but what about putting 
all errors in a header file like

QEMU_ERROR(QERR_DEV_NFOUND,
            "device not found",
            whatever);

Then you can use the header file to generate both the enum and the error 
table:

typedef enum QErrorCode {
#define QEMU_ERROR(code_, ...) code_,
#include "qemu.def"
#undef QEMU_ERROR
} QErrorCode;

...

static QErrorTable qerror_table[] = {
#define QEMU_ERROR(code_, desc_, data_) \
     { .code = (code_), .desc = (desc_), .data = (data_),
#include "qemu-error.def"
#undef QEMU_ERROR
};

This has the disadvantage of not allowing usage of designated initializers.

Paolo

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

[Luiz Capitulino]

On Wed, 14 Oct 2009 15:29:41 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 10/14/2009 12:34 AM, Markus Armbruster 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 906e897..3ce48f7 100644
> >> --- a/hw/qdev.c
> >> +++ b/hw/qdev.c
> >> @@ -28,6 +28,7 @@
> >>   #include "net.h"
> >>   #include "qdev.h"
> >>   #include "sysemu.h"
> >> +#include "qerror.h"
> >>   #include "monitor.h"
> >>
> >>   static int qdev_hotplug = 0;
> >> @@ -176,8 +177,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> >>       /* find driver */
> >>       info = qdev_find_info(NULL, driver);
> >>       if (!info) {
> >> -        qemu_error("Device \"%s\" not found.  Try -device '?' for a list.\n",
> >> -                   driver);
> >> +        qemu_error_structed(QERR_QDEV_NFOUND, "{ s: s }", "name", driver);
> >>           return NULL;
> >>       }
> >>       if (info->no_user) {
> >
> > And here we pay the price for structured error objects: reporting an
> > error becomes more difficult.  The harder you make reporting errors, the
> > fewer errors get caught and reported.  Also, the result is harder to
> > read.
> 
> If you count also the new code in 6/9, reporting an error definitely 
> becomes harder.  Finding a middle ground would be great, and I think 
> that the right solution is to put as many things as possible in a single 
> place.  For example:
> 
> 1) there is a lot of code that is duplicated in every occurrence of an 
> error.  One possibility to avoid this, would be to use a more 
> printf-like syntax for qemu_object_from_va, for example one of these:
> 
>       .... "{ name: %s }", driver);
>       .... "{ 'name': %s }", driver);
> 
> Then you can move the first argument to the error table:
> 
> +        .code   = QERR_QDEV_NFOUND,
> +        .desc   = "device not found",
> +        .keys   = "{ name: %s }"
> 
> so that the qemu_error_structed call looks like
> 
>      qemu_error_structed (QERR_QDEV_NFOUND, driver);
> 
> 2) do we need full flexibility of a function for printing errors?  What 
> about adding a print-from-qobject function, so that you could replace 
> qerror_table[].user_print with a string like
> 
>           .formatted_print = "Device \"%{name}\" not found.  Try -device"
>                              " '?' for a list."

 I like both, but I see them as future improvements.

> 3) I don't think this technique is used in qemu, but what about putting 
> all errors in a header file like

 We have qemu-monitor.hx, which is something like it.

> QEMU_ERROR(QERR_DEV_NFOUND,
>             "device not found",
>             whatever);
> 
> Then you can use the header file to generate both the enum and the error 
> table:
> 
> typedef enum QErrorCode {
> #define QEMU_ERROR(code_, ...) code_,
> #include "qemu.def"
> #undef QEMU_ERROR
> } QErrorCode;
> 
> ...
> 
> static QErrorTable qerror_table[] = {
> #define QEMU_ERROR(code_, desc_, data_) \
>      { .code = (code_), .desc = (desc_), .data = (data_),
> #include "qemu-error.def"
> #undef QEMU_ERROR
> };
> 
> This has the disadvantage of not allowing usage of designated initializers.

 Not sure if it really helps.

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

[Luiz Capitulino]

On Wed, 14 Oct 2009 00:34:21 +0200
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 906e897..3ce48f7 100644
> > --- a/hw/qdev.c
> > +++ b/hw/qdev.c
> > @@ -28,6 +28,7 @@
> >  #include "net.h"
> >  #include "qdev.h"
> >  #include "sysemu.h"
> > +#include "qerror.h"
> >  #include "monitor.h"
> >  
> >  static int qdev_hotplug = 0;
> > @@ -176,8 +177,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> >      /* find driver */
> >      info = qdev_find_info(NULL, driver);
> >      if (!info) {
> > -        qemu_error("Device \"%s\" not found.  Try -device '?' for a list.\n",
> > -                   driver);
> > +        qemu_error_structed(QERR_QDEV_NFOUND, "{ s: s }", "name", driver);
> >          return NULL;
> >      }
> >      if (info->no_user) {
> 
> And here we pay the price for structured error objects: reporting an
> error becomes more difficult.  The harder you make reporting errors, the
> fewer errors get caught and reported.  Also, the result is harder to
> read.
> 
> If we need the structure, then it's just a price we have to pay.  Do we
> need it?

 Yes, if we want clients to able to determine error causes.

 Of course that we can decide to provide less information, but this
structure has three users (monitor protocol, command-line and monitor),
so adding information for one of them means adding for them all.

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

[Daniel P. Berrange]

On Wed, Oct 14, 2009 at 12:34:21AM +0200, Markus Armbruster 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 906e897..3ce48f7 100644
> > --- a/hw/qdev.c
> > +++ b/hw/qdev.c
> > @@ -28,6 +28,7 @@
> >  #include "net.h"
> >  #include "qdev.h"
> >  #include "sysemu.h"
> > +#include "qerror.h"
> >  #include "monitor.h"
> >  
> >  static int qdev_hotplug = 0;
> > @@ -176,8 +177,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> >      /* find driver */
> >      info = qdev_find_info(NULL, driver);
> >      if (!info) {
> > -        qemu_error("Device \"%s\" not found.  Try -device '?' for a list.\n",
> > -                   driver);
> > +        qemu_error_structed(QERR_QDEV_NFOUND, "{ s: s }", "name", driver);
> >          return NULL;
> >      }
> >      if (info->no_user) {

> Now let's look at errors from the client's perspective.
> 
> Clients need to classify errors to figure out how to respond to them.
> 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).
> 
> Some classical protocols (HTTP, FTP) provide error class (they cleverly
> encode it into the error code).  This gives clients a chance to sanely
> handle errors they don't know.
> 
> Even with error class figured out, clients may still be interested in
> the exact error code, at least in some cases.
> 
> They may also be interested in a human readable description of the
> error, to present to their human users.  Some classical protocols
> provide that in addition to an error code.  This gives clients a chance
> to sanely report errors they don't know to human users.
> 
> I suspect that the additional error data is the error's least
> interesting part most of the time.  When we get QERR_QDEV_NFOUND, I
> figure it's usually clear what device we were trying to find.
> 
> But these are just my educated guesses.  I'd love to hear what folks
> involved with actual clients have to say.  Anyone from libvirt?

I think just returning error codes to the client is far too little
information. I don't think we need the fully normalized structure
that Luiz originally proposed with bus/dev addresses split out, but
we certainly need to include a string description giving as much
detail as possible.  If attaching a host USB device failed, I don't
want a single error code  QERR_OPEN_FAILED, nor a generic message 
like 'could not open device', i want the exact details, eg

  'could not open device: permission denied' 
  'could not open device: no such file or directory' 
  'could not open device: device or resource busy' 

The localization issue is somewhat of a red herring though. This string
description is not something that clients would ultimately expose to the
user. The client apps would likely present a more generic error message
to the user, based off the error code. The error description received
from QEMU will instead be written out to the logs as a record for later
troubleshooting. eg if the user files a bug report, it will include the
full error details, so libvirt/qemu maintainers have a better chance of
figuring out what went wrong.

So ultimately these error messages are for developers, not users benefit.

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 7/9] qdev: Use QError for not found error

[Gerd Hoffmann]

> I think just returning error codes to the client is far too little
> information. I don't think we need the fully normalized structure
> that Luiz originally proposed with bus/dev addresses split out, but
> we certainly need to include a string description giving as much
> detail as possible.  If attaching a host USB device failed, I don't
> want a single error code  QERR_OPEN_FAILED, nor a generic message
> like 'could not open device', i want the exact details, eg
>
>    'could not open device: permission denied'
>    'could not open device: no such file or directory'
>    'could not open device: device or resource busy'

Which makes me wonder whenever it makes sense to re-use errno for the 
error codes instead of inventing our own QERR_* codes?  Not the numbers 
of course because they are not standardized as far I know, but the Ename 
strings.

If you error out because a system call failed you can pass up errno 
straight to the client without loosing information along the way.  And 
for other error conditions it should be possible to find error codes 
describing what happened, i.e. when trying to add a device where no 
backend driver exists in qemu you can return ENOENT (plus freeform text 
message for error logging).

cheers,
   Gerd

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

[Daniel P. Berrange]

On Mon, Oct 19, 2009 at 12:40:08PM +0200, Gerd Hoffmann wrote:
> >I think just returning error codes to the client is far too little
> >information. I don't think we need the fully normalized structure
> >that Luiz originally proposed with bus/dev addresses split out, but
> >we certainly need to include a string description giving as much
> >detail as possible.  If attaching a host USB device failed, I don't
> >want a single error code  QERR_OPEN_FAILED, nor a generic message
> >like 'could not open device', i want the exact details, eg
> >
> >   'could not open device: permission denied'
> >   'could not open device: no such file or directory'
> >   'could not open device: device or resource busy'
> 
> Which makes me wonder whenever it makes sense to re-use errno for the 
> error codes instead of inventing our own QERR_* codes?  Not the numbers 
> of course because they are not standardized as far I know, but the Ename 
> strings.
> 
> If you error out because a system call failed you can pass up errno 
> straight to the client without loosing information along the way.  And 
> for other error conditions it should be possible to find error codes 
> describing what happened, i.e. when trying to add a device where no 
> backend driver exists in qemu you can return ENOENT (plus freeform text 
> message for error logging).

If applicable to the context, I think it is worthwhile including more than
just the generic errno string. eg, if failed opening a disk file, then it
would be fine to just send back the errno string, because the original
drive_add command would already include the filename in question. If
failed opening some random sysfs file relating to a PCI device that is
being attached, then QEMU should back the sysfs filename because it may
not be obvious from just the PCI bus/domain/slot number in the original
command just which file QEMU failed to open.


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 :|

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

[Paolo Bonzini]

On 10/19/2009 12:40 PM, Gerd Hoffmann wrote:
>> 'could not open device: permission denied' 'could not open device:
>> no such file or directory' 'could not open device: device or
>> resource busy'
>
> Which makes me wonder whenever it makes sense to re-use errno for the
> error codes instead of inventing our own QERR_* codes?

The errno would be just one field in the error.  You'd have
QERR_COULD_NOT_OPEN with a string field EACCES/ENOENT/EBUSY/Ewhatever.

> Not the numbers of course because they are not standardized as far I
> know, but the Ename strings.

No, numbers are not standardized.

Paolo

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

[Anthony Liguori]

Daniel P. Berrange wrote:
> hink just returning error codes to the client is far too little
> information. I don't think we need the fully normalized structure
> that Luiz originally proposed with bus/dev addresses split out, but
> we certainly need to include a string description giving as much
> detail as possible.  If attaching a host USB device failed, I don't
> want a single error code  QERR_OPEN_FAILED, nor a generic message 
> like 'could not open device', i want the exact details, eg
>
>   'could not open device: permission denied' 
>   'could not open device: no such file or directory' 
>   'could not open device: device or resource busy' 
>
> The localization issue is somewhat of a red herring though. This string
> description is not something that clients would ultimately expose to the
> user. The client apps would likely present a more generic error message
> to the user, based off the error code. The error description received
> from QEMU will instead be written out to the logs as a record for later
> troubleshooting. eg if the user files a bug report, it will include the
> full error details, so libvirt/qemu maintainers have a better chance of
> figuring out what went wrong.
>   

But then advanced users end up doing silly things like analyzing libvirt 
logs.  Why do that when your own example is obviously "could not open 
device: %s", strerror(errno).   We should pass {"error" : {"type": 
"OpenFailedException", "errno": errno}}

-- 
Regards,

Anthony Liguori

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

[Daniel P. Berrange]

On Mon, Oct 19, 2009 at 09:00:33AM -0500, Anthony Liguori wrote:
> Daniel P. Berrange wrote:
> >hink just returning error codes to the client is far too little
> >information. I don't think we need the fully normalized structure
> >that Luiz originally proposed with bus/dev addresses split out, but
> >we certainly need to include a string description giving as much
> >detail as possible.  If attaching a host USB device failed, I don't
> >want a single error code  QERR_OPEN_FAILED, nor a generic message 
> >like 'could not open device', i want the exact details, eg
> >
> >  'could not open device: permission denied' 
> >  'could not open device: no such file or directory' 
> >  'could not open device: device or resource busy' 
> >
> >The localization issue is somewhat of a red herring though. This string
> >description is not something that clients would ultimately expose to the
> >user. The client apps would likely present a more generic error message
> >to the user, based off the error code. The error description received
> >from QEMU will instead be written out to the logs as a record for later
> >troubleshooting. eg if the user files a bug report, it will include the
> >full error details, so libvirt/qemu maintainers have a better chance of
> >figuring out what went wrong.
> >  
> 
> But then advanced users end up doing silly things like analyzing libvirt 
> logs.  Why do that when your own example is obviously "could not open 
> device: %s", strerror(errno).   We should pass {"error" : {"type": 
> "OpenFailedException", "errno": errno}}

Having a named "exception" instead of an error code is fine, but I think
it is overkill to include fully-structured data fields like 'errno' instead
of just a string. The libvirt application API has a insanely detailed 
error object allowing for passing of structured data back to apps, but 
I'm not aware of any application that has ever used it.  They all just
hook on the error code, and log/print detailed string message field. 

In terms of libvirt using QMP error data, there may be one or two
places where we'd toggle behaviour off an error code / exception type,
but any futher structured data would likely just be converted to a generic
string message. So I don't see much need for anything beyond a error code
or exception type to be used for behaviour decisions, and a detailed string
description for logging.

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 7/9] qdev: Use QError for not found error

[Anthony Liguori]

Daniel P. Berrange wrote:
> Having a named "exception" instead of an error code is fine, but I think
> it is overkill to include fully-structured data fields like 'errno' instead
> of just a string. The libvirt application API has a insanely detailed 
> error object allowing for passing of structured data back to apps, but 
> I'm not aware of any application that has ever used it.  They all just
> hook on the error code, and log/print detailed string message field. 
>   

Just because the current consumers are lazy doesn't mean we should 
restrict the API.

> In terms of libvirt using QMP error data, there may be one or two
> places where we'd toggle behaviour off an error code / exception type,
> but any futher structured data would likely just be converted to a generic
> string message. So I don't see much need for anything beyond a error code
> or exception type to be used for behaviour decisions, and a detailed string
> description for logging.
>   

While it gets complicated when going through things like libvirt, if we 
design the python bindings for libqmp directly, these will appear as 
proper exception objects with members corresponding to the data. I would 
fully expect that users would use the structured exception data in a 
language like Python. For instance, when changing a cdrom iso, being 
able to handle EPERM and ENOENT separately is very valuable.

Regards,

Anthony Liguori

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

[Daniel P. Berrange]

On Mon, Oct 19, 2009 at 10:27:51AM -0500, Anthony Liguori wrote:
> Daniel P. Berrange wrote:
> >Having a named "exception" instead of an error code is fine, but I think
> >it is overkill to include fully-structured data fields like 'errno' instead
> >of just a string. The libvirt application API has a insanely detailed 
> >error object allowing for passing of structured data back to apps, but 
> >I'm not aware of any application that has ever used it.  They all just
> >hook on the error code, and log/print detailed string message field. 
> >  
> 
> Just because the current consumers are lazy doesn't mean we should 
> restrict the API.
> 
> >In terms of libvirt using QMP error data, there may be one or two
> >places where we'd toggle behaviour off an error code / exception type,
> >but any futher structured data would likely just be converted to a generic
> >string message. So I don't see much need for anything beyond a error code
> >or exception type to be used for behaviour decisions, and a detailed string
> >description for logging.
> >  
> 
> While it gets complicated when going through things like libvirt, if we 
> design the python bindings for libqmp directly, these will appear as 
> proper exception objects with members corresponding to the data. I would 
> fully expect that users would use the structured exception data in a 
> language like Python. For instance, when changing a cdrom iso, being 
> able to handle EPERM and ENOENT separately is very valuable.

That would suggest that the example exception type is wrong. Instead of 
naming the exception after the operation that failed, named it after the
cause of the failure. ie, this

  {"error" : {"type": "OpenFailedException", "errno": errno}}

would be better as one of...

  {"error" : {"type": "AccessDeniedException",
              "desc": "unable to open disk /tmp/demo.qcow"}},
  {"error" : {"type": "NoSuchFileException",
              "desc": "file /tmp/demo.qcow does not exist" }}
  ....other specific causes of failure...
  {"error" : {"type": "SystemException",
              "desc": "file /tmp/demo.qcow does: ...some other strerror()..." }}

which is more like the type of exception classification you get in Java,
Python. Explicit named exception types for important problems, and then 
one generic 'SystemException' for less commonly encountered errno's. Of
course Java/Python have inheritance, so AccessDeniedException and 
NoSuchFileException would be inherit from SystemException  anyway. It 
is fairly rare to find exceptions providing more fine grained detail than
just the string error message. 

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 :|

[Qemu-devel] [PATCH 8/9] QError: Add do_info_balloon() errors

[Luiz Capitulino]

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

diff --git a/qerror.c b/qerror.c
index 88a8208..df06f93 100644
--- a/qerror.c
+++ b/qerror.c
@@ -41,6 +41,13 @@ static QErrorTable qerror_table[] = {
         .desc   = "device not found",
         .user_print = qemu_err_qdev_nodev,
     },
+    {
+        .code   = QERR_BAL_MMU,
+        .desc   = "Using KVM without synchronous MMU, ballooning disabled",
+    },
+    {   .code   = QERR_BAL_DIS,
+        .desc   = "Ballooning not activated in VM",
+    },
 };
 
 /**
diff --git a/qerror.h b/qerror.h
index 820f25e..43d8758 100644
--- a/qerror.h
+++ b/qerror.h
@@ -22,6 +22,8 @@
 typedef enum QErrorCode {
     QERR_UNKNOWN,
     QERR_QDEV_NFOUND,
+    QERR_BAL_MMU,
+    QERR_BAL_DIS,
     QERR_MAX,
 } QErrorCode;
 
-- 
1.6.5.rc3.8.g8ba5e

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

[Luiz Capitulino]

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

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

Re: [Qemu-devel] [PATCH v0 0/9] QError

[Anthony Liguori]

Luiz Capitulino wrote:
>  This series adds infrastructure which will be needed by the future monitor
> protocol to handle and emit error information correctly.
>
>  QError is a QObject designed to stored error information in way that it can
> be emitted by the protocol code or printed in an user friendly way.
>
>  To use it in the Monitor and its handlers, I've added a function called
> qemu_error_structed(), which builds on top of Gerd's QemuErrorSink API, this
> means that it is also capable of printing error information to stderr.
>
>  Basically, we have a QError member in the Monitor struct, which will be set
> if an error has occurred. The Monitor's top level code will check it at
> handler exit time and handle the error properly.
>
>  QError represents an error, and it has the following properties:
>
> - Error code
> - Error general description
> - Error specific data
>
>  Code and description are stored in a static table in the QError module itself,
> the error specific data is generated at error time by the function calling
> qemu_error_structed().
>
>  This design is based on the way I plan to emit error information in the
> protocol, which is:
>
> { "error": { "code": json-number,
>              "desc": json-string,
>              "data": json-value } "id": json-value, "timestamp": json-string }	
>   

Why timestamp?
>  Besides introducing QError, this series also has simple usage examples for
> qdev_device_add() and do_info_balloon().
>
>  Hopefully this is the last big piece of infrastructure before having the
> protocol parser itself.
>
>  Please, review carefully.
>
>  Thanks.
>
> PS: Applies on top of my last series.
>
>
>   

Re: [Qemu-devel] [PATCH v0 0/9] QError

[Luiz Capitulino]

On Thu, 15 Oct 2009 14:24:57 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> Luiz Capitulino wrote:
> >  This series adds infrastructure which will be needed by the future monitor
> > protocol to handle and emit error information correctly.
> >
> >  QError is a QObject designed to stored error information in way that it can
> > be emitted by the protocol code or printed in an user friendly way.
> >
> >  To use it in the Monitor and its handlers, I've added a function called
> > qemu_error_structed(), which builds on top of Gerd's QemuErrorSink API, this
> > means that it is also capable of printing error information to stderr.
> >
> >  Basically, we have a QError member in the Monitor struct, which will be set
> > if an error has occurred. The Monitor's top level code will check it at
> > handler exit time and handle the error properly.
> >
> >  QError represents an error, and it has the following properties:
> >
> > - Error code
> > - Error general description
> > - Error specific data
> >
> >  Code and description are stored in a static table in the QError module itself,
> > the error specific data is generated at error time by the function calling
> > qemu_error_structed().
> >
> >  This design is based on the way I plan to emit error information in the
> > protocol, which is:
> >
> > { "error": { "code": json-number,
> >              "desc": json-string,
> >              "data": json-value } "id": json-value, "timestamp": json-string }	
> >   
> 
> Why timestamp?

 I would have to dig the archives to find the thread, but some people have
asked to provide the timestamp on every replies (either, error or regular
output).

Re: [Qemu-devel] [PATCH v0 0/9] QError

[Markus Armbruster]

I just caught up with this topic, and since there's no obvious one
message to reply to, I just reply here.

I agree with Anthony we better get the error reporting protocol
approximately right the first time, and rushing it could lead to tears
later on.  Mind, I said "approximately right", not "perfect".

However, there are more than one way to screw this up.  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, for reasons
that have already been mentioned by me and others:

* 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.

* It's highly desirable for errors to do double duty as documentation.
  Code like this (random pick) doesn't need a comment:

        if (qemu_opt_get(opts, "vlan")) {
            qemu_error("The 'vlan' parameter is not valid with -netdev\n");
            return -1;
        }

  Bury the human-readable description in some far-away error list, and
  we lose that.  Even wrapping it in enough error object construction
  boiler-plate can easily lose it.

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?

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

What if we go with that now, and later realize that it was *too* stupid,
i.e. we need more structure after all?  I believe that'll be only
disastrous if we need an *incompatible* protocol revision to accomodate
it.  Can't we simply add a "data" member to the error object then?

Re: [Qemu-devel] [PATCH v0 0/9] QError

[Anthony Liguori]

Markus Armbruster wrote:
> I just caught up with this topic, and since there's no obvious one
> message to reply to, I just reply here.
>
> I agree with Anthony we better get the error reporting protocol
> approximately right the first time, and rushing it could lead to tears
> later on.  Mind, I said "approximately right", not "perfect".
>
> However, there are more than one way to screw this up.  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, for reasons
> that have already been mentioned by me and others:
>
> * 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.
>
> * It's highly desirable for errors to do double duty as documentation.
>   Code like this (random pick) doesn't need a comment:
>
>         if (qemu_opt_get(opts, "vlan")) {
>             qemu_error("The 'vlan' parameter is not valid with -netdev\n");
>             return -1;
>         }
>   

Or:

if (qemu_opt_get(opts, "vlan")) {
   throw InvalidParameter("The 'vlan' parameter is not valid with -netdev");
}

Which would become:

#define INVALID_PARAMETER "InvalidParameter"

if (qemu_opt_get(opts, "vlan")) {
    qemu_error(INVALID_PARAMETER, "The 'vlan' parameter is not valid 
with -netdev");
    return -1;
}

And if we find that this is a common occurrance, we probably should do:

invalid_parameter("vlan", "-netdev");

Which would expand to:

asprintf(&buf, "The '%s' parameter is not valid with %s", param, option);
qemu_error_full("InvalidParameter", "{'param': %s, 'option': %s, 'desc': 
%s}", param, option, desc);

The real fundamental requirements are:

1) In order to support high level languages, an error class is needed to 
generate proper exceptions.
2) To integrate well into high level languages exception mechanisms, we 
make errors first class objects.
3) Exceptions should have a string representation.  desc is a standard 
string representation.

There's really nothing complex here.

-- 
Regards,

Anthony Liguori

[Qemu-devel] [PATCH 0/9]: QError v3

[Luiz Capitulino]

 Hi,

 This is a new QError version which implements what has been 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);

 Thanks.

[Qemu-devel] [PATCH 9/9] 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 a8fd552..76cb187 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1717,10 +1717,11 @@ static void do_info_balloon(Monitor *mon, QObject **ret_data)
 
     actual = qemu_balloon_status();
     if (kvm_enabled() && !kvm_has_sync_mmu())
-        monitor_printf(mon, "Using KVM without synchronous MMU, "
-                       "ballooning disabled\n");
+        qemu_error_new(QERR_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.155.gbb47