[Qemu-devel] [00/11] QAPI Round 0 (JSON improvements)

[Qemu-devel] [00/11] QAPI Round 0 (JSON improvements)

[Anthony Liguori]

As part of making QAPI usable for virt-agent, Mike Roth and I spent some time
hardening the current JSON infrastructure against malicious input.  I thought
it would be useful to split this out independently of QAPI Round 1.

This does include Error and glib.  To propagate JSON errors, we need Error.

[Qemu-devel] [PATCH 01/11] Add hard build dependency on glib

[Anthony Liguori]

GLib is an extremely common library that has a portable thread implementation
along with tons of other goodies.

GLib and GObject have a fantastic amount of infrastructure we can leverage in
QEMU including an object oriented programming infrastructure.

Short term, it has a very nice thread pool implementation that we could leverage
in something like virtio-9p.  It also has a test harness implementation that
this series will use.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/Makefile b/Makefile
index eca4c76..6b1d716 100644
--- a/Makefile
+++ b/Makefile
@@ -104,6 +104,8 @@ audio/audio.o audio/fmodaudio.o: QEMU_CFLAGS += $(FMOD_CFLAGS)
 
 QEMU_CFLAGS+=$(CURL_CFLAGS)
 
+QEMU_CFLAGS+=$(GLIB_CFLAGS)
+
 ui/cocoa.o: ui/cocoa.m
 
 ui/sdl.o audio/sdlaudio.o ui/sdl_zoom.o baum.o: QEMU_CFLAGS += $(SDL_CFLAGS)
diff --git a/Makefile.objs b/Makefile.objs
index 9e98a66..0ba02c7 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -322,3 +322,5 @@ vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
 
 vl.o: QEMU_CFLAGS+=$(SDL_CFLAGS)
 
+vl.o: QEMU_CFLAGS+=$(GLIB_CFLAGS)
+
diff --git a/Makefile.target b/Makefile.target
index f0df98e..046b0f2 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -204,6 +204,7 @@ QEMU_CFLAGS += $(VNC_TLS_CFLAGS)
 QEMU_CFLAGS += $(VNC_SASL_CFLAGS)
 QEMU_CFLAGS += $(VNC_JPEG_CFLAGS)
 QEMU_CFLAGS += $(VNC_PNG_CFLAGS)
+QEMU_CFLAGS += $(GLIB_CFLAGS)
 
 # xen backend driver support
 obj-$(CONFIG_XEN) += xen_machine_pv.o xen_domainbuild.o
diff --git a/configure b/configure
index 5513d3e..7454159 100755
--- a/configure
+++ b/configure
@@ -1663,6 +1663,18 @@ EOF
 fi
 
 ##########################################
+# glib support probe
+if $pkg_config --modversion gthread-2.0 > /dev/null 2>&1 ; then
+    glib_cflags=`$pkg_config --cflags gthread-2.0 2>/dev/null`
+    glib_libs=`$pkg_config --libs gthread-2.0 2>/dev/null`
+    libs_softmmu="$glib_libs $libs_softmmu"
+    libs_tools="$glib_libs $libs_softmmu"
+else
+    echo "glib-2.0 required to compile QEMU"
+    exit 1
+fi
+
+##########################################
 # kvm probe
 if test "$kvm" != "no" ; then
     cat > $TMPC <<EOF
@@ -2758,6 +2770,7 @@ if test "$bluez" = "yes" ; then
   echo "CONFIG_BLUEZ=y" >> $config_host_mak
   echo "BLUEZ_CFLAGS=$bluez_cflags" >> $config_host_mak
 fi
+echo "GLIB_CFLAGS=$glib_cflags" >> $config_host_mak
 if test "$xen" = "yes" ; then
   echo "CONFIG_XEN=y" >> $config_host_mak
 fi
-- 
1.7.0.4

[Qemu-devel] Re: [PATCH 01/11] Add hard build dependency on glib

[Paolo Bonzini]

On 03/11/2011 10:00 PM, Anthony Liguori wrote:
> GLib is an extremely common library that has a portable thread implementation
> along with tons of other goodies.
>
> GLib and GObject have a fantastic amount of infrastructure we can leverage in
> QEMU including an object oriented programming infrastructure.
>
> Short term, it has a very nice thread pool implementation that we could leverage
> in something like virtio-9p.  It also has a test harness implementation that
> this series will use.
>
> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>

This doesn't need to go in until there is use for it, which is IIRC for 
the QAPI tests?

Paolo

Re: [Qemu-devel] Re: [PATCH 01/11] Add hard build dependency on glib

[Anthony Liguori]

On 03/12/2011 02:09 AM, Paolo Bonzini wrote:
> On 03/11/2011 10:00 PM, Anthony Liguori wrote:
>> GLib is an extremely common library that has a portable thread 
>> implementation
>> along with tons of other goodies.
>>
>> GLib and GObject have a fantastic amount of infrastructure we can 
>> leverage in
>> QEMU including an object oriented programming infrastructure.
>>
>> Short term, it has a very nice thread pool implementation that we 
>> could leverage
>> in something like virtio-9p.  It also has a test harness 
>> implementation that
>> this series will use.
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>
> This doesn't need to go in until there is use for it, which is IIRC 
> for the QAPI tests?

Correct.  I mistakenly thought I still used it in Error but I removed 
that dependency.

Regards,

Anthony Liguori

> Paolo
>

[Qemu-devel] [PATCH 02/11] qerror: expose a function to format an error

[Anthony Liguori]

This will let Error share the QError human formatting.  This is only used for
HMP.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/qerror.c b/qerror.c
index 4855604..13d53c9 100644
--- a/qerror.c
+++ b/qerror.c
@@ -326,12 +326,18 @@ QError *qerror_from_info(const char *file, int linenr, const char *func,
     return qerr;
 }
 
-static void parse_error(const QError *qerror, int c)
+static void parse_error(const QErrorStringTable *entry, int c)
 {
-    qerror_abort(qerror, "expected '%c' in '%s'", c, qerror->entry->desc);
+#if 0
+    qerror_abort(qerror, "expected '%c' in '%s'", c, entry->desc);
+#else
+    fprintf(stderr, "expected '%c' in '%s'", c, entry->desc);
+    abort();
+#endif
 }
 
-static const char *append_field(QString *outstr, const QError *qerror,
+static const char *append_field(QDict *error, QString *outstr,
+                                const QErrorStringTable *entry,
                                 const char *start)
 {
     QObject *obj;
@@ -339,24 +345,31 @@ static const char *append_field(QString *outstr, const QError *qerror,
     QString *key_qs;
     const char *end, *key;
 
-    if (*start != '%')
-        parse_error(qerror, '%');
+    if (*start != '%') {
+        parse_error(entry, '%');
+    }
     start++;
-    if (*start != '(')
-        parse_error(qerror, '(');
+    if (*start != '(') {
+        parse_error(entry, '(');
+    }
     start++;
 
     end = strchr(start, ')');
-    if (!end)
-        parse_error(qerror, ')');
+    if (!end) {
+        parse_error(entry, ')');
+    }
 
     key_qs = qstring_from_substr(start, 0, end - start - 1);
     key = qstring_get_str(key_qs);
 
-    qdict = qobject_to_qdict(qdict_get(qerror->error, "data"));
+    qdict = qobject_to_qdict(qdict_get(error, "data"));
     obj = qdict_get(qdict, key);
     if (!obj) {
+#if 0
         qerror_abort(qerror, "key '%s' not found in QDict", key);
+#else
+        abort();
+#endif
     }
 
     switch (qobject_type(obj)) {
@@ -367,41 +380,66 @@ static const char *append_field(QString *outstr, const QError *qerror,
             qstring_append_int(outstr, qdict_get_int(qdict, key));
             break;
         default:
+#if 0
             qerror_abort(qerror, "invalid type '%c'", qobject_type(obj));
+#else
+            abort();
+#endif
     }
 
     QDECREF(key_qs);
     return ++end;
 }
 
-/**
- * qerror_human(): Format QError data into human-readable string.
- *
- * Formats according to member 'desc' of the specified QError object.
- */
-QString *qerror_human(const QError *qerror)
+static QString *qerror_format_desc(QDict *error,
+                                   const QErrorStringTable *entry)
 {
-    const char *p;
     QString *qstring;
+    const char *p;
 
-    assert(qerror->entry != NULL);
+    assert(entry != NULL);
 
     qstring = qstring_new();
 
-    for (p = qerror->entry->desc; *p != '\0';) {
+    for (p = entry->desc; *p != '\0';) {
         if (*p != '%') {
             qstring_append_chr(qstring, *p++);
         } else if (*(p + 1) == '%') {
             qstring_append_chr(qstring, '%');
             p += 2;
         } else {
-            p = append_field(qstring, qerror, p);
+            p = append_field(error, qstring, entry, p);
         }
     }
 
     return qstring;
 }
 
+QString *qerror_format(const char *fmt, QDict *error)
+{
+    const QErrorStringTable *entry = NULL;
+    int i;
+
+    for (i = 0; qerror_table[i].error_fmt; i++) {
+        if (strcmp(qerror_table[i].error_fmt, fmt) == 0) {
+            entry = &qerror_table[i];
+            break;
+        }
+    }
+
+    return qerror_format_desc(error, entry);
+}
+
+/**
+ * qerror_human(): Format QError data into human-readable string.
+ *
+ * Formats according to member 'desc' of the specified QError object.
+ */
+QString *qerror_human(const QError *qerror)
+{
+    return qerror_format_desc(qerror->error, qerror->entry);
+}
+
 /**
  * qerror_print(): Print QError data
  *
diff --git a/qerror.h b/qerror.h
index f732d45..fd63ee9 100644
--- a/qerror.h
+++ b/qerror.h
@@ -42,6 +42,7 @@ void qerror_report_internal(const char *file, int linenr, const char *func,
 #define qerror_report(fmt, ...) \
     qerror_report_internal(__FILE__, __LINE__, __func__, fmt, ## __VA_ARGS__)
 QError *qobject_to_qerror(const QObject *obj);
+QString *qerror_format(const char *fmt, QDict *error);
 
 /*
  * QError class list
-- 
1.7.0.4

[Qemu-devel] Re: [PATCH 02/11] qerror: expose a function to format an error

[Anthony Liguori]

On 03/11/2011 03:00 PM, Anthony Liguori wrote:
> This will let Error share the QError human formatting.  This is only used for
> HMP.
>
> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>
> diff --git a/qerror.c b/qerror.c
> index 4855604..13d53c9 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -326,12 +326,18 @@ QError *qerror_from_info(const char *file, int linenr, const char *func,
>       return qerr;
>   }
>
> -static void parse_error(const QError *qerror, int c)
> +static void parse_error(const QErrorStringTable *entry, int c)
>   {
> -    qerror_abort(qerror, "expected '%c' in '%s'", c, qerror->entry->desc);
> +#if 0
> +    qerror_abort(qerror, "expected '%c' in '%s'", c, entry->desc);
> +#else
> +    fprintf(stderr, "expected '%c' in '%s'", c, entry->desc);
> +    abort();
> +#endif
>   }

Err, I shouldn't have left these #if 0's in here.  Please ignore them.

Regards,

Anthony Liguori

> -static const char *append_field(QString *outstr, const QError *qerror,
> +static const char *append_field(QDict *error, QString *outstr,
> +                                const QErrorStringTable *entry,
>                                   const char *start)
>   {
>       QObject *obj;
> @@ -339,24 +345,31 @@ static const char *append_field(QString *outstr, const QError *qerror,
>       QString *key_qs;
>       const char *end, *key;
>
> -    if (*start != '%')
> -        parse_error(qerror, '%');
> +    if (*start != '%') {
> +        parse_error(entry, '%');
> +    }
>       start++;
> -    if (*start != '(')
> -        parse_error(qerror, '(');
> +    if (*start != '(') {
> +        parse_error(entry, '(');
> +    }
>       start++;
>
>       end = strchr(start, ')');
> -    if (!end)
> -        parse_error(qerror, ')');
> +    if (!end) {
> +        parse_error(entry, ')');
> +    }
>
>       key_qs = qstring_from_substr(start, 0, end - start - 1);
>       key = qstring_get_str(key_qs);
>
> -    qdict = qobject_to_qdict(qdict_get(qerror->error, "data"));
> +    qdict = qobject_to_qdict(qdict_get(error, "data"));
>       obj = qdict_get(qdict, key);
>       if (!obj) {
> +#if 0
>           qerror_abort(qerror, "key '%s' not found in QDict", key);
> +#else
> +        abort();
> +#endif
>       }
>
>       switch (qobject_type(obj)) {
> @@ -367,41 +380,66 @@ static const char *append_field(QString *outstr, const QError *qerror,
>               qstring_append_int(outstr, qdict_get_int(qdict, key));
>               break;
>           default:
> +#if 0
>               qerror_abort(qerror, "invalid type '%c'", qobject_type(obj));
> +#else
> +            abort();
> +#endif
>       }
>
>       QDECREF(key_qs);
>       return ++end;
>   }
>
> -/**
> - * qerror_human(): Format QError data into human-readable string.
> - *
> - * Formats according to member 'desc' of the specified QError object.
> - */
> -QString *qerror_human(const QError *qerror)
> +static QString *qerror_format_desc(QDict *error,
> +                                   const QErrorStringTable *entry)
>   {
> -    const char *p;
>       QString *qstring;
> +    const char *p;
>
> -    assert(qerror->entry != NULL);
> +    assert(entry != NULL);
>
>       qstring = qstring_new();
>
> -    for (p = qerror->entry->desc; *p != '\0';) {
> +    for (p = entry->desc; *p != '\0';) {
>           if (*p != '%') {
>               qstring_append_chr(qstring, *p++);
>           } else if (*(p + 1) == '%') {
>               qstring_append_chr(qstring, '%');
>               p += 2;
>           } else {
> -            p = append_field(qstring, qerror, p);
> +            p = append_field(error, qstring, entry, p);
>           }
>       }
>
>       return qstring;
>   }
>
> +QString *qerror_format(const char *fmt, QDict *error)
> +{
> +    const QErrorStringTable *entry = NULL;
> +    int i;
> +
> +    for (i = 0; qerror_table[i].error_fmt; i++) {
> +        if (strcmp(qerror_table[i].error_fmt, fmt) == 0) {
> +            entry =&qerror_table[i];
> +            break;
> +        }
> +    }
> +
> +    return qerror_format_desc(error, entry);
> +}
> +
> +/**
> + * qerror_human(): Format QError data into human-readable string.
> + *
> + * Formats according to member 'desc' of the specified QError object.
> + */
> +QString *qerror_human(const QError *qerror)
> +{
> +    return qerror_format_desc(qerror->error, qerror->entry);
> +}
> +
>   /**
>    * qerror_print(): Print QError data
>    *
> diff --git a/qerror.h b/qerror.h
> index f732d45..fd63ee9 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -42,6 +42,7 @@ void qerror_report_internal(const char *file, int linenr, const char *func,
>   #define qerror_report(fmt, ...) \
>       qerror_report_internal(__FILE__, __LINE__, __func__, fmt, ## __VA_ARGS__)
>   QError *qobject_to_qerror(const QObject *obj);
> +QString *qerror_format(const char *fmt, QDict *error);
>
>   /*
>    * QError class list

[Qemu-devel] Re: [PATCH 02/11] qerror: expose a function to format an error

[Luiz Capitulino]

On Fri, 11 Mar 2011 15:08:38 -0600
Anthony Liguori <aliguori@us.ibm.com> wrote:

> On 03/11/2011 03:00 PM, Anthony Liguori wrote:
> > This will let Error share the QError human formatting.  This is only used for
> > HMP.
> >
> > Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
> >
> > diff --git a/qerror.c b/qerror.c
> > index 4855604..13d53c9 100644
> > --- a/qerror.c
> > +++ b/qerror.c
> > @@ -326,12 +326,18 @@ QError *qerror_from_info(const char *file, int linenr, const char *func,
> >       return qerr;
> >   }
> >
> > -static void parse_error(const QError *qerror, int c)
> > +static void parse_error(const QErrorStringTable *entry, int c)
> >   {
> > -    qerror_abort(qerror, "expected '%c' in '%s'", c, qerror->entry->desc);
> > +#if 0
> > +    qerror_abort(qerror, "expected '%c' in '%s'", c, entry->desc);
> > +#else
> > +    fprintf(stderr, "expected '%c' in '%s'", c, entry->desc);
> > +    abort();
> > +#endif
> >   }
> 
> Err, I shouldn't have left these #if 0's in here.  Please ignore them.

But you're going to keep qerror_abort() usage, right?
 
> 
> Regards,
> 
> Anthony Liguori
> 
> > -static const char *append_field(QString *outstr, const QError *qerror,
> > +static const char *append_field(QDict *error, QString *outstr,
> > +                                const QErrorStringTable *entry,
> >                                   const char *start)
> >   {
> >       QObject *obj;
> > @@ -339,24 +345,31 @@ static const char *append_field(QString *outstr, const QError *qerror,
> >       QString *key_qs;
> >       const char *end, *key;
> >
> > -    if (*start != '%')
> > -        parse_error(qerror, '%');
> > +    if (*start != '%') {
> > +        parse_error(entry, '%');
> > +    }
> >       start++;
> > -    if (*start != '(')
> > -        parse_error(qerror, '(');
> > +    if (*start != '(') {
> > +        parse_error(entry, '(');
> > +    }
> >       start++;
> >
> >       end = strchr(start, ')');
> > -    if (!end)
> > -        parse_error(qerror, ')');
> > +    if (!end) {
> > +        parse_error(entry, ')');
> > +    }
> >
> >       key_qs = qstring_from_substr(start, 0, end - start - 1);
> >       key = qstring_get_str(key_qs);
> >
> > -    qdict = qobject_to_qdict(qdict_get(qerror->error, "data"));
> > +    qdict = qobject_to_qdict(qdict_get(error, "data"));
> >       obj = qdict_get(qdict, key);
> >       if (!obj) {
> > +#if 0
> >           qerror_abort(qerror, "key '%s' not found in QDict", key);
> > +#else
> > +        abort();
> > +#endif
> >       }
> >
> >       switch (qobject_type(obj)) {
> > @@ -367,41 +380,66 @@ static const char *append_field(QString *outstr, const QError *qerror,
> >               qstring_append_int(outstr, qdict_get_int(qdict, key));
> >               break;
> >           default:
> > +#if 0
> >               qerror_abort(qerror, "invalid type '%c'", qobject_type(obj));
> > +#else
> > +            abort();
> > +#endif
> >       }
> >
> >       QDECREF(key_qs);
> >       return ++end;
> >   }
> >
> > -/**
> > - * qerror_human(): Format QError data into human-readable string.
> > - *
> > - * Formats according to member 'desc' of the specified QError object.
> > - */
> > -QString *qerror_human(const QError *qerror)
> > +static QString *qerror_format_desc(QDict *error,
> > +                                   const QErrorStringTable *entry)
> >   {
> > -    const char *p;
> >       QString *qstring;
> > +    const char *p;
> >
> > -    assert(qerror->entry != NULL);
> > +    assert(entry != NULL);
> >
> >       qstring = qstring_new();
> >
> > -    for (p = qerror->entry->desc; *p != '\0';) {
> > +    for (p = entry->desc; *p != '\0';) {
> >           if (*p != '%') {
> >               qstring_append_chr(qstring, *p++);
> >           } else if (*(p + 1) == '%') {
> >               qstring_append_chr(qstring, '%');
> >               p += 2;
> >           } else {
> > -            p = append_field(qstring, qerror, p);
> > +            p = append_field(error, qstring, entry, p);
> >           }
> >       }
> >
> >       return qstring;
> >   }
> >
> > +QString *qerror_format(const char *fmt, QDict *error)
> > +{
> > +    const QErrorStringTable *entry = NULL;
> > +    int i;
> > +
> > +    for (i = 0; qerror_table[i].error_fmt; i++) {
> > +        if (strcmp(qerror_table[i].error_fmt, fmt) == 0) {
> > +            entry =&qerror_table[i];
> > +            break;
> > +        }
> > +    }
> > +
> > +    return qerror_format_desc(error, entry);
> > +}
> > +
> > +/**
> > + * qerror_human(): Format QError data into human-readable string.
> > + *
> > + * Formats according to member 'desc' of the specified QError object.
> > + */
> > +QString *qerror_human(const QError *qerror)
> > +{
> > +    return qerror_format_desc(qerror->error, qerror->entry);
> > +}
> > +
> >   /**
> >    * qerror_print(): Print QError data
> >    *
> > diff --git a/qerror.h b/qerror.h
> > index f732d45..fd63ee9 100644
> > --- a/qerror.h
> > +++ b/qerror.h
> > @@ -42,6 +42,7 @@ void qerror_report_internal(const char *file, int linenr, const char *func,
> >   #define qerror_report(fmt, ...) \
> >       qerror_report_internal(__FILE__, __LINE__, __func__, fmt, ## __VA_ARGS__)
> >   QError *qobject_to_qerror(const QObject *obj);
> > +QString *qerror_format(const char *fmt, QDict *error);
> >
> >   /*
> >    * QError class list
> 

Re: [Qemu-devel] Re: [PATCH 02/11] qerror: expose a function to format an error

[Anthony Liguori]

On 03/14/2011 02:17 PM, Luiz Capitulino wrote:
> On Fri, 11 Mar 2011 15:08:38 -0600
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>
>> On 03/11/2011 03:00 PM, Anthony Liguori wrote:
>>> This will let Error share the QError human formatting.  This is only used for
>>> HMP.
>>>
>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>>
>>> diff --git a/qerror.c b/qerror.c
>>> index 4855604..13d53c9 100644
>>> --- a/qerror.c
>>> +++ b/qerror.c
>>> @@ -326,12 +326,18 @@ QError *qerror_from_info(const char *file, int linenr, const char *func,
>>>        return qerr;
>>>    }
>>>
>>> -static void parse_error(const QError *qerror, int c)
>>> +static void parse_error(const QErrorStringTable *entry, int c)
>>>    {
>>> -    qerror_abort(qerror, "expected '%c' in '%s'", c, qerror->entry->desc);
>>> +#if 0
>>> +    qerror_abort(qerror, "expected '%c' in '%s'", c, entry->desc);
>>> +#else
>>> +    fprintf(stderr, "expected '%c' in '%s'", c, entry->desc);
>>> +    abort();
>>> +#endif
>>>    }
>> Err, I shouldn't have left these #if 0's in here.  Please ignore them.
> But you're going to keep qerror_abort() usage, right?

No, qerror_abort() needs to go away.

It's too tied to QError and this patch is making the formatting code 
work outside of of QEMU.

Once this whole series is completely merged, QError goes away entirely 
and this pretty formatting is replaced with something much simpler.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 02/11] qerror: expose a function to format an error

[Luiz Capitulino]

On Mon, 14 Mar 2011 14:27:30 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 03/14/2011 02:17 PM, Luiz Capitulino wrote:
> > On Fri, 11 Mar 2011 15:08:38 -0600
> > Anthony Liguori<aliguori@us.ibm.com>  wrote:
> >
> >> On 03/11/2011 03:00 PM, Anthony Liguori wrote:
> >>> This will let Error share the QError human formatting.  This is only used for
> >>> HMP.
> >>>
> >>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
> >>>
> >>> diff --git a/qerror.c b/qerror.c
> >>> index 4855604..13d53c9 100644
> >>> --- a/qerror.c
> >>> +++ b/qerror.c
> >>> @@ -326,12 +326,18 @@ QError *qerror_from_info(const char *file, int linenr, const char *func,
> >>>        return qerr;
> >>>    }
> >>>
> >>> -static void parse_error(const QError *qerror, int c)
> >>> +static void parse_error(const QErrorStringTable *entry, int c)
> >>>    {
> >>> -    qerror_abort(qerror, "expected '%c' in '%s'", c, qerror->entry->desc);
> >>> +#if 0
> >>> +    qerror_abort(qerror, "expected '%c' in '%s'", c, entry->desc);
> >>> +#else
> >>> +    fprintf(stderr, "expected '%c' in '%s'", c, entry->desc);
> >>> +    abort();
> >>> +#endif
> >>>    }
> >> Err, I shouldn't have left these #if 0's in here.  Please ignore them.
> > But you're going to keep qerror_abort() usage, right?
> 
> No, qerror_abort() needs to go away.
> 
> It's too tied to QError and this patch is making the formatting code 
> work outside of of QEMU.

qerror_abort() only exists for debugging purposes. I won't say its perfect,
but it's better than nothing and has already saved some time when writing
new errors.

I'm fine dropping it as long as there's a better replacement, which is
not the case here. There's even a hunk that replaces qerror_abort() for
a plain abort().

> Once this whole series is completely merged, QError goes away entirely 
> and this pretty formatting is replaced with something much simpler.
> 
> Regards,
> 
> Anthony Liguori
> 

Re: [Qemu-devel] Re: [PATCH 02/11] qerror: expose a function to format an error

[Anthony Liguori]

On 03/14/2011 02:37 PM, Luiz Capitulino wrote:
>
> qerror_abort() only exists for debugging purposes. I won't say its perfect,
> but it's better than nothing and has already saved some time when writing
> new errors.
>
> I'm fine dropping it as long as there's a better replacement, which is
> not the case here. There's even a hunk that replaces qerror_abort() for
> a plain abort().

Yes, that's the replacement.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 02/11] qerror: expose a function to format an error

[Luiz Capitulino]

On Mon, 14 Mar 2011 14:45:13 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> On 03/14/2011 02:37 PM, Luiz Capitulino wrote:
> >
> > qerror_abort() only exists for debugging purposes. I won't say its perfect,
> > but it's better than nothing and has already saved some time when writing
> > new errors.
> >
> > I'm fine dropping it as long as there's a better replacement, which is
> > not the case here. There's even a hunk that replaces qerror_abort() for
> > a plain abort().
> 
> Yes, that's the replacement.

It's not a good one: it makes the current code a bit worse and we don't know
how and when the error classes are going to be replaced.

Maybe a better merge plan would be to work on errors first. Completely drop
qerror according to qapi needs, and then put the rest of the stuff on top.

Re: [Qemu-devel] Re: [PATCH 02/11] qerror: expose a function to format an error

[Anthony Liguori]

On 03/14/2011 03:22 PM, Luiz Capitulino wrote:
> On Mon, 14 Mar 2011 14:45:13 -0500
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>
>> On 03/14/2011 02:37 PM, Luiz Capitulino wrote:
>>> qerror_abort() only exists for debugging purposes. I won't say its perfect,
>>> but it's better than nothing and has already saved some time when writing
>>> new errors.
>>>
>>> I'm fine dropping it as long as there's a better replacement, which is
>>> not the case here. There's even a hunk that replaces qerror_abort() for
>>> a plain abort().
>> Yes, that's the replacement.
> It's not a good one: it makes the current code a bit worse and we don't know
> how and when the error classes are going to be replaced.

Yes, we do, before 0.15.0.

> Maybe a better merge plan would be to work on errors first. Completely drop
> qerror according to qapi needs, and then put the rest of the stuff on top.

Can't be done until we introduce new QMP commands to get rid of the old 
HMP commands because there are HMP commands without equivalents that 
make use of qerror_report().

There are just a handful of these left in my QAPI branch so once I can 
start adding these QMP commands, they'll be gone quickly.  I don't want 
to introduce a bunch of new QMP commands without this stuff getting 
merged upstream first though.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 02/11] qerror: expose a function to format an error

[Luiz Capitulino]

On Mon, 14 Mar 2011 15:41:49 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> On 03/14/2011 03:22 PM, Luiz Capitulino wrote:
> > On Mon, 14 Mar 2011 14:45:13 -0500
> > Anthony Liguori<aliguori@us.ibm.com>  wrote:
> >
> >> On 03/14/2011 02:37 PM, Luiz Capitulino wrote:
> >>> qerror_abort() only exists for debugging purposes. I won't say its perfect,
> >>> but it's better than nothing and has already saved some time when writing
> >>> new errors.
> >>>
> >>> I'm fine dropping it as long as there's a better replacement, which is
> >>> not the case here. There's even a hunk that replaces qerror_abort() for
> >>> a plain abort().
> >> Yes, that's the replacement.
> > It's not a good one: it makes the current code a bit worse and we don't know
> > how and when the error classes are going to be replaced.
> 
> Yes, we do, before 0.15.0.

Very optimistic :) I don't doubt you can post patches quickly, but we're
likely going to have fun discussions, respins, tests etc. And all the QAPI
stuff in parallel.

> > Maybe a better merge plan would be to work on errors first. Completely drop
> > qerror according to qapi needs, and then put the rest of the stuff on top.
> 
> Can't be done until we introduce new QMP commands to get rid of the old 
> HMP commands because there are HMP commands without equivalents that 
> make use of qerror_report().
> 
> There are just a handful of these left in my QAPI branch so once I can 
> start adding these QMP commands, they'll be gone quickly.  I don't want 
> to introduce a bunch of new QMP commands without this stuff getting 
> merged upstream first though.
> 
> Regards,
> 
> Anthony Liguori
> 

Re: [Qemu-devel] Re: [PATCH 02/11] qerror: expose a function to format an error

[Anthony Liguori]

On 03/14/2011 03:48 PM, Luiz Capitulino wrote:
> On Mon, 14 Mar 2011 15:41:49 -0500
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>
>> On 03/14/2011 03:22 PM, Luiz Capitulino wrote:
>>> On Mon, 14 Mar 2011 14:45:13 -0500
>>> Anthony Liguori<aliguori@us.ibm.com>   wrote:
>>>
>>>> On 03/14/2011 02:37 PM, Luiz Capitulino wrote:
>>>>> qerror_abort() only exists for debugging purposes. I won't say its perfect,
>>>>> but it's better than nothing and has already saved some time when writing
>>>>> new errors.
>>>>>
>>>>> I'm fine dropping it as long as there's a better replacement, which is
>>>>> not the case here. There's even a hunk that replaces qerror_abort() for
>>>>> a plain abort().
>>>> Yes, that's the replacement.
>>> It's not a good one: it makes the current code a bit worse and we don't know
>>> how and when the error classes are going to be replaced.
>> Yes, we do, before 0.15.0.
> Very optimistic :) I don't doubt you can post patches quickly, but we're
> likely going to have fun discussions, respins, tests etc. And all the QAPI
> stuff in parallel.

Let's have those discussions then because that's what's important.

I split this out because I didn't want to have a 40 patch series so I 
tried to split into two logical series.

But the goal is here QAPI.  That's what's important to get merged.  I'm 
not terribly interested in merging these changes until we're ready to 
merge the first round of QAPI.

It's a big change, and it's optimistic, but that's what makes it worth 
trying to do.

Regards,

Anthony Liguori

[Qemu-devel] [PATCH 03/11] add a generic Error object

[Anthony Liguori]

The Error class is similar to QError (now deprecated) except that it supports
propagation.  This allows for higher quality error handling.  It's losely
modeled after glib style GErrors.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/Makefile.objs b/Makefile.objs
index 0ba02c7..da31530 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -15,6 +15,7 @@ oslib-obj-$(CONFIG_POSIX) += oslib-posix.o
 
 block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o
 block-obj-y += nbd.o block.o aio.o aes.o qemu-config.o
+block-obj-y += error.o
 block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
 block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
 
diff --git a/error.c b/error.c
new file mode 100644
index 0000000..5d84106
--- /dev/null
+++ b/error.c
@@ -0,0 +1,122 @@
+/*
+ * QEMU Error Objects
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.  See
+ * the COPYING.LIB file in the top-level directory.
+ */
+#include "error.h"
+#include "error_int.h"
+#include "qemu-objects.h"
+#include "qerror.h"
+#include <assert.h>
+
+struct Error
+{
+    QDict *obj;
+    const char *fmt;
+    char *msg;
+};
+
+void error_set(Error **errp, const char *fmt, ...)
+{
+    Error *err;
+    va_list ap;
+
+    if (errp == NULL) {
+        return;
+    }
+
+    err = qemu_mallocz(sizeof(*err));
+
+    va_start(ap, fmt);
+    err->obj = qobject_to_qdict(qobject_from_jsonv(fmt, &ap));
+    va_end(ap);
+    err->fmt = fmt;
+
+    *errp = err;
+}
+
+bool error_is_set(Error **errp)
+{
+    return (errp && *errp);
+}
+
+const char *error_get_pretty(Error *err)
+{
+    if (err->msg == NULL) {
+        QString *str;
+        str = qerror_format(err->fmt, err->obj);
+        err->msg = qemu_strdup(qstring_get_str(str));
+    }
+
+    return err->msg;
+}
+
+const char *error_get_field(Error *err, const char *field)
+{
+    if (strcmp(field, "class") == 0) {
+        return qdict_get_str(err->obj, field);
+    } else {
+        QDict *dict = qdict_get_qdict(err->obj, "data");
+        return qdict_get_str(dict, field);
+    }
+}
+
+void error_free(Error *err)
+{
+    QDECREF(err->obj);
+    qemu_free(err->msg);
+    qemu_free(err);
+}
+
+bool error_is_type(Error *err, const char *fmt)
+{
+    char *ptr;
+    char *end;
+    char classname[1024];
+
+    ptr = strstr(fmt, "'class': '");
+    assert(ptr != NULL);
+    ptr += strlen("'class': '");
+
+    end = strchr(ptr, '\'');
+    assert(end != NULL);
+    
+    memcpy(classname, ptr, (end - ptr));
+    classname[(end - ptr)] = 0;
+
+    return strcmp(classname, error_get_field(err, "class")) == 0;
+}
+
+void error_propagate(Error **dst_err, Error *local_err)
+{
+    if (dst_err) {
+        *dst_err = local_err;
+    } else if (local_err) {
+        error_free(local_err);
+    }
+}
+
+QObject *error_get_qobject(Error *err)
+{
+    QINCREF(err->obj);
+    return QOBJECT(err->obj);
+}
+
+void error_set_qobject(Error **errp, QObject *obj)
+{
+    Error *err;
+    if (errp == NULL) {
+        return;
+    }
+    err = qemu_mallocz(sizeof(*err));
+    err->obj = qobject_to_qdict(obj);
+    qobject_incref(obj);
+
+    *errp = err;
+}
diff --git a/error.h b/error.h
new file mode 100644
index 0000000..317d487
--- /dev/null
+++ b/error.h
@@ -0,0 +1,65 @@
+/*
+ * QEMU Error Objects
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.  See
+ * the COPYING.LIB file in the top-level directory.
+ */
+#ifndef ERROR_H
+#define ERROR_H
+
+#include <stdbool.h>
+
+/**
+ * A class representing internal errors within QEMU.  An error has a string
+ * typename and optionally a set of named string parameters.
+ */
+typedef struct Error Error;
+
+/**
+ * Set an indirect pointer to an error given a printf-style format parameter.
+ * Currently, qerror.h defines these error formats.  This function is not
+ * meant to be used outside of QEMU.
+ */
+void error_set(Error **err, const char *fmt, ...)
+    __attribute__((format(printf, 2, 3)));
+
+/**
+ * Returns true if an indirect pointer to an error is pointing to a valid
+ * error object.
+ */
+bool error_is_set(Error **err);
+
+/**
+ * Get a human readable representation of an error object.
+ */
+const char *error_get_pretty(Error *err);
+
+/**
+ * Get an individual named error field.
+ */
+const char *error_get_field(Error *err, const char *field);
+
+/**
+ * Propagate an error to an indirect pointer to an error.  This function will
+ * always transfer ownership of the error reference and handles the case where
+ * dst_err is NULL correctly.
+ */
+void error_propagate(Error **dst_err, Error *local_err);
+
+/**
+ * Free an error object.
+ */
+void error_free(Error *err);
+
+/**
+ * Determine if an error is of a speific type (based on the qerror format).
+ * Non-QEMU users should get the `class' field to identify the error type.
+ */
+bool error_is_type(Error *err, const char *fmt);
+
+#endif
diff --git a/error_int.h b/error_int.h
new file mode 100644
index 0000000..eaba65e
--- /dev/null
+++ b/error_int.h
@@ -0,0 +1,27 @@
+/*
+ * QEMU Error Objects
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.  See
+ * the COPYING.LIB file in the top-level directory.
+ */
+#ifndef QEMU_ERROR_INT_H
+#define QEMU_ERROR_INT_H
+
+#include "qemu-common.h"
+#include "qobject.h"
+#include "error.h"
+
+/**
+ * Internal QEMU functions for working with Error.
+ *
+ * These are used to convert QErrors to Errors
+ */
+QObject *error_get_qobject(Error *err);
+void error_set_qobject(Error **errp, QObject *obj);
+  
+#endif
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH 03/11] add a generic Error object

[Blue Swirl]

On Fri, Mar 11, 2011 at 11:00 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> The Error class is similar to QError (now deprecated) except that it supports
> propagation.  This allows for higher quality error handling.  It's losely
> modeled after glib style GErrors.
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 0ba02c7..da31530 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -15,6 +15,7 @@ oslib-obj-$(CONFIG_POSIX) += oslib-posix.o
>
>  block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o
>  block-obj-y += nbd.o block.o aio.o aes.o qemu-config.o
> +block-obj-y += error.o
>  block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
>  block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
>
> diff --git a/error.c b/error.c
> new file mode 100644
> index 0000000..5d84106
> --- /dev/null
> +++ b/error.c
> @@ -0,0 +1,122 @@
> +/*
> + * QEMU Error Objects
> + *
> + * Copyright IBM, Corp. 2011
> + *
> + * Authors:
> + *  Anthony Liguori   <aliguori@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
> + * the COPYING.LIB file in the top-level directory.
> + */
> +#include "error.h"
> +#include "error_int.h"
> +#include "qemu-objects.h"
> +#include "qerror.h"
> +#include <assert.h>
> +
> +struct Error
> +{
> +    QDict *obj;
> +    const char *fmt;
> +    char *msg;
> +};
> +
> +void error_set(Error **errp, const char *fmt, ...)
> +{
> +    Error *err;
> +    va_list ap;
> +
> +    if (errp == NULL) {
> +        return;
> +    }
> +
> +    err = qemu_mallocz(sizeof(*err));
> +
> +    va_start(ap, fmt);
> +    err->obj = qobject_to_qdict(qobject_from_jsonv(fmt, &ap));
> +    va_end(ap);
> +    err->fmt = fmt;
> +
> +    *errp = err;
> +}
> +
> +bool error_is_set(Error **errp)
> +{
> +    return (errp && *errp);
> +}
> +
> +const char *error_get_pretty(Error *err)
> +{
> +    if (err->msg == NULL) {
> +        QString *str;
> +        str = qerror_format(err->fmt, err->obj);
> +        err->msg = qemu_strdup(qstring_get_str(str));
> +    }
> +
> +    return err->msg;
> +}
> +
> +const char *error_get_field(Error *err, const char *field)
> +{
> +    if (strcmp(field, "class") == 0) {
> +        return qdict_get_str(err->obj, field);
> +    } else {
> +        QDict *dict = qdict_get_qdict(err->obj, "data");
> +        return qdict_get_str(dict, field);
> +    }
> +}
> +
> +void error_free(Error *err)
> +{
> +    QDECREF(err->obj);
> +    qemu_free(err->msg);
> +    qemu_free(err);
> +}
> +
> +bool error_is_type(Error *err, const char *fmt)
> +{
> +    char *ptr;
> +    char *end;
> +    char classname[1024];
> +
> +    ptr = strstr(fmt, "'class': '");
> +    assert(ptr != NULL);
> +    ptr += strlen("'class': '");
> +
> +    end = strchr(ptr, '\'');
> +    assert(end != NULL);
> +
> +    memcpy(classname, ptr, (end - ptr));
> +    classname[(end - ptr)] = 0;
> +
> +    return strcmp(classname, error_get_field(err, "class")) == 0;
> +}
> +
> +void error_propagate(Error **dst_err, Error *local_err)
> +{
> +    if (dst_err) {
> +        *dst_err = local_err;
> +    } else if (local_err) {
> +        error_free(local_err);
> +    }
> +}
> +
> +QObject *error_get_qobject(Error *err)
> +{
> +    QINCREF(err->obj);
> +    return QOBJECT(err->obj);
> +}
> +
> +void error_set_qobject(Error **errp, QObject *obj)
> +{
> +    Error *err;
> +    if (errp == NULL) {
> +        return;
> +    }
> +    err = qemu_mallocz(sizeof(*err));
> +    err->obj = qobject_to_qdict(obj);
> +    qobject_incref(obj);
> +
> +    *errp = err;
> +}
> diff --git a/error.h b/error.h
> new file mode 100644

The name is too generic, it could conflict with system headers. At
least I have /usr/include/error.h.

> index 0000000..317d487
> --- /dev/null
> +++ b/error.h
> @@ -0,0 +1,65 @@
> +/*
> + * QEMU Error Objects
> + *
> + * Copyright IBM, Corp. 2011
> + *
> + * Authors:
> + *  Anthony Liguori   <aliguori@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
> + * the COPYING.LIB file in the top-level directory.
> + */
> +#ifndef ERROR_H
> +#define ERROR_H

This #define could also conflict with system headers. In my system,
_ERROR_H is used, but who knows all error.h files out there?

Re: [Qemu-devel] [PATCH 03/11] add a generic Error object

[Anthony Liguori]

On 03/12/2011 05:05 AM, Blue Swirl wrote:
> On Fri, Mar 11, 2011 at 11:00 PM, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>> The Error class is similar to QError (now deprecated) except that it supports
>> propagation.  This allows for higher quality error handling.  It's losely
>> modeled after glib style GErrors.
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 0ba02c7..da31530 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -15,6 +15,7 @@ oslib-obj-$(CONFIG_POSIX) += oslib-posix.o
>>
>>   block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o
>>   block-obj-y += nbd.o block.o aio.o aes.o qemu-config.o
>> +block-obj-y += error.o
>>   block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
>>   block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
>>
>> diff --git a/error.c b/error.c
>> new file mode 100644
>> index 0000000..5d84106
>> --- /dev/null
>> +++ b/error.c
>> @@ -0,0 +1,122 @@
>> +/*
>> + * QEMU Error Objects
>> + *
>> + * Copyright IBM, Corp. 2011
>> + *
>> + * Authors:
>> + *  Anthony Liguori<aliguori@us.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
>> + * the COPYING.LIB file in the top-level directory.
>> + */
>> +#include "error.h"
>> +#include "error_int.h"
>> +#include "qemu-objects.h"
>> +#include "qerror.h"
>> +#include<assert.h>
>> +
>> +struct Error
>> +{
>> +    QDict *obj;
>> +    const char *fmt;
>> +    char *msg;
>> +};
>> +
>> +void error_set(Error **errp, const char *fmt, ...)
>> +{
>> +    Error *err;
>> +    va_list ap;
>> +
>> +    if (errp == NULL) {
>> +        return;
>> +    }
>> +
>> +    err = qemu_mallocz(sizeof(*err));
>> +
>> +    va_start(ap, fmt);
>> +    err->obj = qobject_to_qdict(qobject_from_jsonv(fmt,&ap));
>> +    va_end(ap);
>> +    err->fmt = fmt;
>> +
>> +    *errp = err;
>> +}
>> +
>> +bool error_is_set(Error **errp)
>> +{
>> +    return (errp&&  *errp);
>> +}
>> +
>> +const char *error_get_pretty(Error *err)
>> +{
>> +    if (err->msg == NULL) {
>> +        QString *str;
>> +        str = qerror_format(err->fmt, err->obj);
>> +        err->msg = qemu_strdup(qstring_get_str(str));
>> +    }
>> +
>> +    return err->msg;
>> +}
>> +
>> +const char *error_get_field(Error *err, const char *field)
>> +{
>> +    if (strcmp(field, "class") == 0) {
>> +        return qdict_get_str(err->obj, field);
>> +    } else {
>> +        QDict *dict = qdict_get_qdict(err->obj, "data");
>> +        return qdict_get_str(dict, field);
>> +    }
>> +}
>> +
>> +void error_free(Error *err)
>> +{
>> +    QDECREF(err->obj);
>> +    qemu_free(err->msg);
>> +    qemu_free(err);
>> +}
>> +
>> +bool error_is_type(Error *err, const char *fmt)
>> +{
>> +    char *ptr;
>> +    char *end;
>> +    char classname[1024];
>> +
>> +    ptr = strstr(fmt, "'class': '");
>> +    assert(ptr != NULL);
>> +    ptr += strlen("'class': '");
>> +
>> +    end = strchr(ptr, '\'');
>> +    assert(end != NULL);
>> +
>> +    memcpy(classname, ptr, (end - ptr));
>> +    classname[(end - ptr)] = 0;
>> +
>> +    return strcmp(classname, error_get_field(err, "class")) == 0;
>> +}
>> +
>> +void error_propagate(Error **dst_err, Error *local_err)
>> +{
>> +    if (dst_err) {
>> +        *dst_err = local_err;
>> +    } else if (local_err) {
>> +        error_free(local_err);
>> +    }
>> +}
>> +
>> +QObject *error_get_qobject(Error *err)
>> +{
>> +    QINCREF(err->obj);
>> +    return QOBJECT(err->obj);
>> +}
>> +
>> +void error_set_qobject(Error **errp, QObject *obj)
>> +{
>> +    Error *err;
>> +    if (errp == NULL) {
>> +        return;
>> +    }
>> +    err = qemu_mallocz(sizeof(*err));
>> +    err->obj = qobject_to_qdict(obj);
>> +    qobject_incref(obj);
>> +
>> +    *errp = err;
>> +}
>> diff --git a/error.h b/error.h
>> new file mode 100644
> The name is too generic, it could conflict with system headers. At
> least I have /usr/include/error.h.
>
>> index 0000000..317d487
>> --- /dev/null
>> +++ b/error.h
>> @@ -0,0 +1,65 @@
>> +/*
>> + * QEMU Error Objects
>> + *
>> + * Copyright IBM, Corp. 2011
>> + *
>> + * Authors:
>> + *  Anthony Liguori<aliguori@us.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
>> + * the COPYING.LIB file in the top-level directory.
>> + */
>> +#ifndef ERROR_H
>> +#define ERROR_H
> This #define could also conflict with system headers. In my system,
> _ERROR_H is used, but who knows all error.h files out there?

Certainly a good point.  I'll use a different name.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 03/11] add a generic Error object

[Luiz Capitulino]

On Fri, 11 Mar 2011 15:00:41 -0600
Anthony Liguori <aliguori@us.ibm.com> wrote:

> The Error class is similar to QError (now deprecated) except that it supports
> propagation.  This allows for higher quality error handling.  It's losely
> modeled after glib style GErrors.

I think Daniel asked this, but I can't remember your answer: why don't we
use GError then?

Also, I think this patch needs more description regarding how this is going
to replace QError. I mean, we want to deprecate QError but it seems to me
that you're going to maintain the error declaration format, like:

 "{ 'class': 'CommandNotFound', 'data': { 'name': %s } }"

And the current QError class list in qerror.h. Did I get it right?

More comments below.

> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 0ba02c7..da31530 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -15,6 +15,7 @@ oslib-obj-$(CONFIG_POSIX) += oslib-posix.o
>  
>  block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o
>  block-obj-y += nbd.o block.o aio.o aes.o qemu-config.o
> +block-obj-y += error.o
>  block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
>  block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o

You also have to do this:

-CHECK_PROG_DEPS = qemu-malloc.o $(oslib-obj-y) $(trace-obj-y)
+CHECK_PROG_DEPS = qemu-malloc.o error.o qerror.o $(oslib-obj-y) $(trace-obj-y)

Otherwise you break check build.

> diff --git a/error.c b/error.c
> new file mode 100644
> index 0000000..5d84106
> --- /dev/null
> +++ b/error.c
> @@ -0,0 +1,122 @@
> +/*
> + * QEMU Error Objects
> + *
> + * Copyright IBM, Corp. 2011
> + *
> + * Authors:
> + *  Anthony Liguori   <aliguori@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
> + * the COPYING.LIB file in the top-level directory.
> + */
> +#include "error.h"
> +#include "error_int.h"
> +#include "qemu-objects.h"
> +#include "qerror.h"
> +#include <assert.h>
> +
> +struct Error
> +{
> +    QDict *obj;

'obj' is a bit generic and sometimes it's used to denote QObjects in the
code, I suggest 'error_dict' or something that communicates its purpose.

> +    const char *fmt;
> +    char *msg;

I don't think we should store 'msg', more about this in error_get_pretty().

> +};
> +
> +void error_set(Error **errp, const char *fmt, ...)
> +{
> +    Error *err;
> +    va_list ap;
> +
> +    if (errp == NULL) {
> +        return;
> +    }
> +
> +    err = qemu_mallocz(sizeof(*err));
> +
> +    va_start(ap, fmt);
> +    err->obj = qobject_to_qdict(qobject_from_jsonv(fmt, &ap));
> +    va_end(ap);
> +    err->fmt = fmt;
> +
> +    *errp = err;
> +}
> +
> +bool error_is_set(Error **errp)
> +{
> +    return (errp && *errp);
> +}
> +
> +const char *error_get_pretty(Error *err)
> +{
> +    if (err->msg == NULL) {
> +        QString *str;
> +        str = qerror_format(err->fmt, err->obj);
> +        err->msg = qemu_strdup(qstring_get_str(str));

Four comments here:

 1. This is missing a QDECREF(str);

 2. Storing 'msg' looks like an unnecessary optimization to me, why don't
    we just re-create it when error_get_pretty() is called?

 3. This function is not used by this series

 4. I think it's a good idea to assert on Error == NULL, specially
    because some functions accept it

> +    }
> +
> +    return err->msg;
> +}
> +
> +const char *error_get_field(Error *err, const char *field)
> +{
> +    if (strcmp(field, "class") == 0) {
> +        return qdict_get_str(err->obj, field);
> +    } else {
> +        QDict *dict = qdict_get_qdict(err->obj, "data");
> +        return qdict_get_str(dict, field);
> +    }
> +}
> +
> +void error_free(Error *err)
> +{
> +    QDECREF(err->obj);
> +    qemu_free(err->msg);
> +    qemu_free(err);
> +}
> +
> +bool error_is_type(Error *err, const char *fmt)
> +{
> +    char *ptr;
> +    char *end;
> +    char classname[1024];
> +
> +    ptr = strstr(fmt, "'class': '");
> +    assert(ptr != NULL);
> +    ptr += strlen("'class': '");
> +
> +    end = strchr(ptr, '\'');
> +    assert(end != NULL);
> +    
> +    memcpy(classname, ptr, (end - ptr));
> +    classname[(end - ptr)] = 0;
> +
> +    return strcmp(classname, error_get_field(err, "class")) == 0;
> +}

Not used by this series. Except for obvious stuff, I prefer to only add
code that's going to be used right away.

> +
> +void error_propagate(Error **dst_err, Error *local_err)
> +{
> +    if (dst_err) {
> +        *dst_err = local_err;
> +    } else if (local_err) {
> +        error_free(local_err);
> +    }
> +}
> +
> +QObject *error_get_qobject(Error *err)
> +{
> +    QINCREF(err->obj);
> +    return QOBJECT(err->obj);
> +}
> +
> +void error_set_qobject(Error **errp, QObject *obj)
> +{
> +    Error *err;
> +    if (errp == NULL) {
> +        return;
> +    }
> +    err = qemu_mallocz(sizeof(*err));
> +    err->obj = qobject_to_qdict(obj);
> +    qobject_incref(obj);
> +
> +    *errp = err;
> +}

This is not documented. Also, I prefer the documentation & code next to each
other in the same file.

> diff --git a/error.h b/error.h
> new file mode 100644
> index 0000000..317d487
> --- /dev/null
> +++ b/error.h
> @@ -0,0 +1,65 @@
> +/*
> + * QEMU Error Objects
> + *
> + * Copyright IBM, Corp. 2011
> + *
> + * Authors:
> + *  Anthony Liguori   <aliguori@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
> + * the COPYING.LIB file in the top-level directory.
> + */
> +#ifndef ERROR_H
> +#define ERROR_H
> +
> +#include <stdbool.h>
> +
> +/**
> + * A class representing internal errors within QEMU.  An error has a string
> + * typename and optionally a set of named string parameters.
> + */
> +typedef struct Error Error;
> +
> +/**
> + * Set an indirect pointer to an error given a printf-style format parameter.
> + * Currently, qerror.h defines these error formats.  This function is not
> + * meant to be used outside of QEMU.
> + */
> +void error_set(Error **err, const char *fmt, ...)
> +    __attribute__((format(printf, 2, 3)));
> +
> +/**
> + * Returns true if an indirect pointer to an error is pointing to a valid
> + * error object.
> + */
> +bool error_is_set(Error **err);
> +
> +/**
> + * Get a human readable representation of an error object.
> + */
> +const char *error_get_pretty(Error *err);
> +
> +/**
> + * Get an individual named error field.
> + */
> +const char *error_get_field(Error *err, const char *field);
> +
> +/**
> + * Propagate an error to an indirect pointer to an error.  This function will
> + * always transfer ownership of the error reference and handles the case where
> + * dst_err is NULL correctly.
> + */
> +void error_propagate(Error **dst_err, Error *local_err);
> +
> +/**
> + * Free an error object.
> + */
> +void error_free(Error *err);
> +
> +/**
> + * Determine if an error is of a speific type (based on the qerror format).
> + * Non-QEMU users should get the `class' field to identify the error type.
> + */
> +bool error_is_type(Error *err, const char *fmt);
> +
> +#endif
> diff --git a/error_int.h b/error_int.h
> new file mode 100644
> index 0000000..eaba65e
> --- /dev/null
> +++ b/error_int.h
> @@ -0,0 +1,27 @@
> +/*
> + * QEMU Error Objects
> + *
> + * Copyright IBM, Corp. 2011
> + *
> + * Authors:
> + *  Anthony Liguori   <aliguori@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
> + * the COPYING.LIB file in the top-level directory.
> + */
> +#ifndef QEMU_ERROR_INT_H
> +#define QEMU_ERROR_INT_H
> +
> +#include "qemu-common.h"
> +#include "qobject.h"
> +#include "error.h"
> +
> +/**
> + * Internal QEMU functions for working with Error.
> + *
> + * These are used to convert QErrors to Errors
> + */
> +QObject *error_get_qobject(Error *err);
> +void error_set_qobject(Error **errp, QObject *obj);
> +  
> +#endif

[Qemu-devel] Re: [PATCH 03/11] add a generic Error object

[Anthony Liguori]

On 03/14/2011 02:18 PM, Luiz Capitulino wrote:
> On Fri, 11 Mar 2011 15:00:41 -0600
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>
>> The Error class is similar to QError (now deprecated) except that it supports
>> propagation.  This allows for higher quality error handling.  It's losely
>> modeled after glib style GErrors.
> I think Daniel asked this, but I can't remember your answer: why don't we
> use GError then?

Because GError just uses strings and doesn't store key/values.

> Also, I think this patch needs more description regarding how this is going
> to replace QError.

Once there is no more qerror usage (we need to converting remaining HMP 
commands to QMP), qerror goes away.  This is scoped for the 0.15 release.

>   I mean, we want to deprecate QError but it seems to me
> that you're going to maintain the error declaration format, like:
>
>   "{ 'class': 'CommandNotFound', 'data': { 'name': %s } }"
>
> And the current QError class list in qerror.h. Did I get it right?

No, it will be switched to something simpler.  The QERR JSON is just an 
implementation detail.

> More comments below.
>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 0ba02c7..da31530 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -15,6 +15,7 @@ oslib-obj-$(CONFIG_POSIX) += oslib-posix.o
>>
>>   block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o
>>   block-obj-y += nbd.o block.o aio.o aes.o qemu-config.o
>> +block-obj-y += error.o
>>   block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
>>   block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
> You also have to do this:
>
> -CHECK_PROG_DEPS = qemu-malloc.o $(oslib-obj-y) $(trace-obj-y)
> +CHECK_PROG_DEPS = qemu-malloc.o error.o qerror.o $(oslib-obj-y) $(trace-obj-y)
>
> Otherwise you break check build.

Ah, okay.  Maybe I'll convert those over to gtester while I'm there.

>> diff --git a/error.c b/error.c
>> new file mode 100644
>> index 0000000..5d84106
>> --- /dev/null
>> +++ b/error.c
>> @@ -0,0 +1,122 @@
>> +/*
>> + * QEMU Error Objects
>> + *
>> + * Copyright IBM, Corp. 2011
>> + *
>> + * Authors:
>> + *  Anthony Liguori<aliguori@us.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
>> + * the COPYING.LIB file in the top-level directory.
>> + */
>> +#include "error.h"
>> +#include "error_int.h"
>> +#include "qemu-objects.h"
>> +#include "qerror.h"
>> +#include<assert.h>
>> +
>> +struct Error
>> +{
>> +    QDict *obj;
> 'obj' is a bit generic and sometimes it's used to denote QObjects in the
> code, I suggest 'error_dict' or something that communicates its purpose.

Sure.

>> +const char *error_get_pretty(Error *err)
>> +{
>> +    if (err->msg == NULL) {
>> +        QString *str;
>> +        str = qerror_format(err->fmt, err->obj);
>> +        err->msg = qemu_strdup(qstring_get_str(str));
> Four comments here:
>
>   1. This is missing a QDECREF(str);

Yes, I caught this a few days ago in my tree thanks to valgrind.

>   2. Storing 'msg' looks like an unnecessary optimization to me, why don't
>      we just re-create it when error_get_pretty() is called?

Because we return a 'const char *' here so by storing msg, we can tie 
the string's life cycle to the Error object.  That means callers don't 
have to worry about freeing it.

>   3. This function is not used by this series

Yeah, it's infrastructure that needs to be here for subsequent series.

>   4. I think it's a good idea to assert on Error == NULL, specially
>      because some functions accept it

Only functions that take a double pointer, but not a bad thing to do.

>> +bool error_is_type(Error *err, const char *fmt)
>> +{
>> +    char *ptr;
>> +    char *end;
>> +    char classname[1024];
>> +
>> +    ptr = strstr(fmt, "'class': '");
>> +    assert(ptr != NULL);
>> +    ptr += strlen("'class': '");
>> +
>> +    end = strchr(ptr, '\'');
>> +    assert(end != NULL);
>> +
>> +    memcpy(classname, ptr, (end - ptr));
>> +    classname[(end - ptr)] = 0;
>> +
>> +    return strcmp(classname, error_get_field(err, "class")) == 0;
>> +}
> Not used by this series. Except for obvious stuff, I prefer to only add
> code that's going to be used right away.

That means adding a ton of stuff all at once.  Splitting it up like this 
is pretty reasonable IMHO.   Think of Error as an interface and not just 
a code dependency.

>> +
>> +void error_propagate(Error **dst_err, Error *local_err)
>> +{
>> +    if (dst_err) {
>> +        *dst_err = local_err;
>> +    } else if (local_err) {
>> +        error_free(local_err);
>> +    }
>> +}
>> +
>> +QObject *error_get_qobject(Error *err)
>> +{
>> +    QINCREF(err->obj);
>> +    return QOBJECT(err->obj);
>> +}
>> +
>> +void error_set_qobject(Error **errp, QObject *obj)
>> +{
>> +    Error *err;
>> +    if (errp == NULL) {
>> +        return;
>> +    }
>> +    err = qemu_mallocz(sizeof(*err));
>> +    err->obj = qobject_to_qdict(obj);
>> +    qobject_incref(obj);
>> +
>> +    *errp = err;
>> +}
> This is not documented. Also, I prefer the documentation&  code next to each
> other in the same file.

These are internal functions to QEMU and are documented as such.

>> diff --git a/error_int.h b/error_int.h
>> new file mode 100644
>> index 0000000..eaba65e
>> --- /dev/null
>> +++ b/error_int.h
>> @@ -0,0 +1,27 @@
>> +/*
>> + * QEMU Error Objects
>> + *
>> + * Copyright IBM, Corp. 2011
>> + *
>> + * Authors:
>> + *  Anthony Liguori<aliguori@us.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
>> + * the COPYING.LIB file in the top-level directory.
>> + */
>> +#ifndef QEMU_ERROR_INT_H
>> +#define QEMU_ERROR_INT_H
>> +
>> +#include "qemu-common.h"
>> +#include "qobject.h"
>> +#include "error.h"
>> +
>> +/**
>> + * Internal QEMU functions for working with Error.
>> + *
>> + * These are used to convert QErrors to Errors
>> + */
>> +QObject *error_get_qobject(Error *err);
>> +void error_set_qobject(Error **errp, QObject *obj);
>> +
>> +#endif

Right here.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 03/11] add a generic Error object

[Luiz Capitulino]

On Mon, 14 Mar 2011 14:34:55 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> On 03/14/2011 02:18 PM, Luiz Capitulino wrote:
> > On Fri, 11 Mar 2011 15:00:41 -0600
> > Anthony Liguori<aliguori@us.ibm.com>  wrote:
> >
> >> The Error class is similar to QError (now deprecated) except that it supports
> >> propagation.  This allows for higher quality error handling.  It's losely
> >> modeled after glib style GErrors.
> > I think Daniel asked this, but I can't remember your answer: why don't we
> > use GError then?
> 
> Because GError just uses strings and doesn't store key/values.
> 
> > Also, I think this patch needs more description regarding how this is going
> > to replace QError.
> 
> Once there is no more qerror usage (we need to converting remaining HMP 
> commands to QMP), qerror goes away.  This is scoped for the 0.15 release.
> 
> >   I mean, we want to deprecate QError but it seems to me
> > that you're going to maintain the error declaration format, like:
> >
> >   "{ 'class': 'CommandNotFound', 'data': { 'name': %s } }"
> >
> > And the current QError class list in qerror.h. Did I get it right?
> 
> No, it will be switched to something simpler.  The QERR JSON is just an 
> implementation detail.
> 
> > More comments below.
> >
> >> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
> >>
> >> diff --git a/Makefile.objs b/Makefile.objs
> >> index 0ba02c7..da31530 100644
> >> --- a/Makefile.objs
> >> +++ b/Makefile.objs
> >> @@ -15,6 +15,7 @@ oslib-obj-$(CONFIG_POSIX) += oslib-posix.o
> >>
> >>   block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o
> >>   block-obj-y += nbd.o block.o aio.o aes.o qemu-config.o
> >> +block-obj-y += error.o
> >>   block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
> >>   block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
> > You also have to do this:
> >
> > -CHECK_PROG_DEPS = qemu-malloc.o $(oslib-obj-y) $(trace-obj-y)
> > +CHECK_PROG_DEPS = qemu-malloc.o error.o qerror.o $(oslib-obj-y) $(trace-obj-y)
> >
> > Otherwise you break check build.
> 
> Ah, okay.  Maybe I'll convert those over to gtester while I'm there.
> 
> >> diff --git a/error.c b/error.c
> >> new file mode 100644
> >> index 0000000..5d84106
> >> --- /dev/null
> >> +++ b/error.c
> >> @@ -0,0 +1,122 @@
> >> +/*
> >> + * QEMU Error Objects
> >> + *
> >> + * Copyright IBM, Corp. 2011
> >> + *
> >> + * Authors:
> >> + *  Anthony Liguori<aliguori@us.ibm.com>
> >> + *
> >> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
> >> + * the COPYING.LIB file in the top-level directory.
> >> + */
> >> +#include "error.h"
> >> +#include "error_int.h"
> >> +#include "qemu-objects.h"
> >> +#include "qerror.h"
> >> +#include<assert.h>
> >> +
> >> +struct Error
> >> +{
> >> +    QDict *obj;
> > 'obj' is a bit generic and sometimes it's used to denote QObjects in the
> > code, I suggest 'error_dict' or something that communicates its purpose.
> 
> Sure.
> 
> >> +const char *error_get_pretty(Error *err)
> >> +{
> >> +    if (err->msg == NULL) {
> >> +        QString *str;
> >> +        str = qerror_format(err->fmt, err->obj);
> >> +        err->msg = qemu_strdup(qstring_get_str(str));
> > Four comments here:
> >
> >   1. This is missing a QDECREF(str);
> 
> Yes, I caught this a few days ago in my tree thanks to valgrind.
> 
> >   2. Storing 'msg' looks like an unnecessary optimization to me, why don't
> >      we just re-create it when error_get_pretty() is called?
> 
> Because we return a 'const char *' here so by storing msg, we can tie 
> the string's life cycle to the Error object.  That means callers don't 
> have to worry about freeing it.

Makes sense.

> 
> >   3. This function is not used by this series
> 
> Yeah, it's infrastructure that needs to be here for subsequent series.
> 
> >   4. I think it's a good idea to assert on Error == NULL, specially
> >      because some functions accept it
> 
> Only functions that take a double pointer, but not a bad thing to do.
> 
> >> +bool error_is_type(Error *err, const char *fmt)
> >> +{
> >> +    char *ptr;
> >> +    char *end;
> >> +    char classname[1024];
> >> +
> >> +    ptr = strstr(fmt, "'class': '");
> >> +    assert(ptr != NULL);
> >> +    ptr += strlen("'class': '");
> >> +
> >> +    end = strchr(ptr, '\'');
> >> +    assert(end != NULL);
> >> +
> >> +    memcpy(classname, ptr, (end - ptr));
> >> +    classname[(end - ptr)] = 0;
> >> +
> >> +    return strcmp(classname, error_get_field(err, "class")) == 0;
> >> +}
> > Not used by this series. Except for obvious stuff, I prefer to only add
> > code that's going to be used right away.
> 
> That means adding a ton of stuff all at once.  Splitting it up like this 
> is pretty reasonable IMHO.   Think of Error as an interface and not just 
> a code dependency.

But it's harder to review and to test.

> 
> >> +
> >> +void error_propagate(Error **dst_err, Error *local_err)
> >> +{
> >> +    if (dst_err) {
> >> +        *dst_err = local_err;
> >> +    } else if (local_err) {
> >> +        error_free(local_err);
> >> +    }
> >> +}
> >> +
> >> +QObject *error_get_qobject(Error *err)
> >> +{
> >> +    QINCREF(err->obj);
> >> +    return QOBJECT(err->obj);
> >> +}
> >> +
> >> +void error_set_qobject(Error **errp, QObject *obj)
> >> +{
> >> +    Error *err;
> >> +    if (errp == NULL) {
> >> +        return;
> >> +    }
> >> +    err = qemu_mallocz(sizeof(*err));
> >> +    err->obj = qobject_to_qdict(obj);
> >> +    qobject_incref(obj);
> >> +
> >> +    *errp = err;
> >> +}
> > This is not documented. Also, I prefer the documentation&  code next to each
> > other in the same file.
> 
> These are internal functions to QEMU and are documented as such.
> 
> >> diff --git a/error_int.h b/error_int.h
> >> new file mode 100644
> >> index 0000000..eaba65e
> >> --- /dev/null
> >> +++ b/error_int.h
> >> @@ -0,0 +1,27 @@
> >> +/*
> >> + * QEMU Error Objects
> >> + *
> >> + * Copyright IBM, Corp. 2011
> >> + *
> >> + * Authors:
> >> + *  Anthony Liguori<aliguori@us.ibm.com>
> >> + *
> >> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
> >> + * the COPYING.LIB file in the top-level directory.
> >> + */
> >> +#ifndef QEMU_ERROR_INT_H
> >> +#define QEMU_ERROR_INT_H
> >> +
> >> +#include "qemu-common.h"
> >> +#include "qobject.h"
> >> +#include "error.h"
> >> +
> >> +/**
> >> + * Internal QEMU functions for working with Error.
> >> + *
> >> + * These are used to convert QErrors to Errors
> >> + */
> >> +QObject *error_get_qobject(Error *err);
> >> +void error_set_qobject(Error **errp, QObject *obj);
> >> +
> >> +#endif
> 
> Right here.
> 
> Regards,
> 
> Anthony Liguori
> 

[Qemu-devel] [PATCH 04/11] qerror: split out the reporting bits of QError

[Anthony Liguori]

These make it very hard to compile QError outside of QEMU.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/Makefile.objs b/Makefile.objs
index da31530..69f0383 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -2,7 +2,7 @@
 # QObject
 qobject-obj-y = qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o
 qobject-obj-y += qjson.o json-lexer.o json-streamer.o json-parser.o
-qobject-obj-y += qerror.o
+qobject-obj-y += qerror.o qerror-report.o
 
 #######################################################################
 # oslib-obj-y is code depending on the OS (win32 vs posix)
diff --git a/qerror-report.c b/qerror-report.c
new file mode 100644
index 0000000..1ebb111
--- /dev/null
+++ b/qerror-report.c
@@ -0,0 +1,131 @@
+/*
+ * QError Module
+ *
+ * Copyright (C) 2009 Red Hat Inc.
+ *
+ * Authors:
+ *  Luiz Capitulino <lcapitulino@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+
+#include "qemu-common.h"
+#include "qerror.h"
+#include "monitor.h"
+#include "qjson.h"
+
+static void GCC_FMT_ATTR(2, 3) qerror_abort(const QError *qerr,
+                                            const char *fmt, ...)
+{
+    va_list ap;
+
+    fprintf(stderr, "qerror: bad call in function '%s':\n", qerr->func);
+    fprintf(stderr, "qerror: -> ");
+
+    va_start(ap, fmt);
+    vfprintf(stderr, fmt, ap);
+    va_end(ap);
+
+    fprintf(stderr, "\nqerror: call at %s:%d\n", qerr->file, qerr->linenr);
+    abort();
+}
+
+static void GCC_FMT_ATTR(2, 0) qerror_set_data(QError *qerr,
+                                               const char *fmt, va_list *va)
+{
+    QObject *obj;
+
+    obj = qobject_from_jsonv(fmt, va);
+    if (!obj) {
+        qerror_abort(qerr, "invalid format '%s'", fmt);
+    }
+    if (qobject_type(obj) != QTYPE_QDICT) {
+        qerror_abort(qerr, "error format is not a QDict '%s'", fmt);
+    }
+
+    qerr->error = qobject_to_qdict(obj);
+
+    obj = qdict_get(qerr->error, "class");
+    if (!obj) {
+        qerror_abort(qerr, "missing 'class' key in '%s'", fmt);
+    }
+    if (qobject_type(obj) != QTYPE_QSTRING) {
+        qerror_abort(qerr, "'class' key value should be a QString");
+    }
+    
+    obj = qdict_get(qerr->error, "data");
+    if (!obj) {
+        qerror_abort(qerr, "missing 'data' key in '%s'", fmt);
+    }
+    if (qobject_type(obj) != QTYPE_QDICT) {
+        qerror_abort(qerr, "'data' key value should be a QDICT");
+    }
+}
+
+/**
+ * qerror_from_info(): Create a new QError from error information
+ *
+ * The information consists of:
+ *
+ * - file   the file name of where the error occurred
+ * - linenr the line number of where the error occurred
+ * - func   the function name of where the error occurred
+ * - fmt    JSON printf-like dictionary, there must exist keys 'class' and
+ *          'data'
+ * - va     va_list of all arguments specified by fmt
+ *
+ * Return strong reference.
+ */
+QError *qerror_from_info(const char *file, int linenr, const char *func,
+                         const char *fmt, va_list *va)
+{
+    QError *qerr;
+
+    qerr = qerror_new();
+    loc_save(&qerr->loc);
+    qerr->linenr = linenr;
+    qerr->file = file;
+    qerr->func = func;
+
+    if (!fmt) {
+        qerror_abort(qerr, "QDict not specified");
+    }
+
+    qerror_set_data(qerr, fmt, va);
+    qerror_set_desc(qerr, fmt);
+
+    return qerr;
+}
+
+/**
+ * qerror_print(): Print QError data
+ *
+ * This function will print the member 'desc' of the specified QError object,
+ * it uses error_report() for this, so that the output is routed to the right
+ * place (ie. stderr or Monitor's device).
+ */
+void qerror_print(QError *qerror)
+{
+    QString *qstring = qerror_human(qerror);
+    loc_push_restore(&qerror->loc);
+    error_report("%s", qstring_get_str(qstring));
+    loc_pop(&qerror->loc);
+    QDECREF(qstring);
+}
+
+void qerror_report_internal(const char *file, int linenr, const char *func,
+                            const char *fmt, ...)
+{
+    va_list va;
+    QError *qerror;
+
+    va_start(va, fmt);
+    qerror = qerror_from_info(file, linenr, func, fmt, &va);
+    va_end(va);
+
+    qerror_print(qerror);
+    QDECREF(qerror);
+}
+
+
diff --git a/qerror.c b/qerror.c
index 13d53c9..78d3884 100644
--- a/qerror.c
+++ b/qerror.c
@@ -243,39 +243,7 @@ static void GCC_FMT_ATTR(2, 3) qerror_abort(const QError *qerr,
     abort();
 }
 
-static void GCC_FMT_ATTR(2, 0) qerror_set_data(QError *qerr,
-                                               const char *fmt, va_list *va)
-{
-    QObject *obj;
-
-    obj = qobject_from_jsonv(fmt, va);
-    if (!obj) {
-        qerror_abort(qerr, "invalid format '%s'", fmt);
-    }
-    if (qobject_type(obj) != QTYPE_QDICT) {
-        qerror_abort(qerr, "error format is not a QDict '%s'", fmt);
-    }
-
-    qerr->error = qobject_to_qdict(obj);
-
-    obj = qdict_get(qerr->error, "class");
-    if (!obj) {
-        qerror_abort(qerr, "missing 'class' key in '%s'", fmt);
-    }
-    if (qobject_type(obj) != QTYPE_QSTRING) {
-        qerror_abort(qerr, "'class' key value should be a QString");
-    }
-    
-    obj = qdict_get(qerr->error, "data");
-    if (!obj) {
-        qerror_abort(qerr, "missing 'data' key in '%s'", fmt);
-    }
-    if (qobject_type(obj) != QTYPE_QDICT) {
-        qerror_abort(qerr, "'data' key value should be a QDICT");
-    }
-}
-
-static void qerror_set_desc(QError *qerr, const char *fmt)
+void qerror_set_desc(QError *qerr, const char *fmt)
 {
     int i;
 
@@ -291,41 +259,6 @@ static void qerror_set_desc(QError *qerr, const char *fmt)
     qerror_abort(qerr, "error format '%s' not found", fmt);
 }
 
-/**
- * qerror_from_info(): Create a new QError from error information
- *
- * The information consists of:
- *
- * - file   the file name of where the error occurred
- * - linenr the line number of where the error occurred
- * - func   the function name of where the error occurred
- * - fmt    JSON printf-like dictionary, there must exist keys 'class' and
- *          'data'
- * - va     va_list of all arguments specified by fmt
- *
- * Return strong reference.
- */
-QError *qerror_from_info(const char *file, int linenr, const char *func,
-                         const char *fmt, va_list *va)
-{
-    QError *qerr;
-
-    qerr = qerror_new();
-    loc_save(&qerr->loc);
-    qerr->linenr = linenr;
-    qerr->file = file;
-    qerr->func = func;
-
-    if (!fmt) {
-        qerror_abort(qerr, "QDict not specified");
-    }
-
-    qerror_set_data(qerr, fmt, va);
-    qerror_set_desc(qerr, fmt);
-
-    return qerr;
-}
-
 static void parse_error(const QErrorStringTable *entry, int c)
 {
 #if 0
@@ -441,40 +374,6 @@ QString *qerror_human(const QError *qerror)
 }
 
 /**
- * qerror_print(): Print QError data
- *
- * This function will print the member 'desc' of the specified QError object,
- * it uses error_report() for this, so that the output is routed to the right
- * place (ie. stderr or Monitor's device).
- */
-void qerror_print(QError *qerror)
-{
-    QString *qstring = qerror_human(qerror);
-    loc_push_restore(&qerror->loc);
-    error_report("%s", qstring_get_str(qstring));
-    loc_pop(&qerror->loc);
-    QDECREF(qstring);
-}
-
-void qerror_report_internal(const char *file, int linenr, const char *func,
-                            const char *fmt, ...)
-{
-    va_list va;
-    QError *qerror;
-
-    va_start(va, fmt);
-    qerror = qerror_from_info(file, linenr, func, fmt, &va);
-    va_end(va);
-
-    if (monitor_cur_is_qmp()) {
-        monitor_set_error(cur_mon, qerror);
-    } else {
-        qerror_print(qerror);
-        QDECREF(qerror);
-    }
-}
-
-/**
  * qobject_to_qerror(): Convert a QObject into a QError
  */
 QError *qobject_to_qerror(const QObject *obj)
diff --git a/qerror.h b/qerror.h
index fd63ee9..495d0a4 100644
--- a/qerror.h
+++ b/qerror.h
@@ -43,6 +43,7 @@ void qerror_report_internal(const char *file, int linenr, const char *func,
     qerror_report_internal(__FILE__, __LINE__, __func__, fmt, ## __VA_ARGS__)
 QError *qobject_to_qerror(const QObject *obj);
 QString *qerror_format(const char *fmt, QDict *error);
+void qerror_set_desc(QError *qerr, const char *fmt);
 
 /*
  * QError class list
-- 
1.7.0.4

[Qemu-devel] Re: [PATCH 04/11] qerror: split out the reporting bits of QError

[Luiz Capitulino]

On Fri, 11 Mar 2011 15:00:42 -0600
Anthony Liguori <aliguori@us.ibm.com> wrote:

> These make it very hard to compile QError outside of QEMU.

Why would someone do this?

> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index da31530..69f0383 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -2,7 +2,7 @@
>  # QObject
>  qobject-obj-y = qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o
>  qobject-obj-y += qjson.o json-lexer.o json-streamer.o json-parser.o
> -qobject-obj-y += qerror.o
> +qobject-obj-y += qerror.o qerror-report.o
>  
>  #######################################################################
>  # oslib-obj-y is code depending on the OS (win32 vs posix)
> diff --git a/qerror-report.c b/qerror-report.c
> new file mode 100644
> index 0000000..1ebb111
> --- /dev/null
> +++ b/qerror-report.c
> @@ -0,0 +1,131 @@
> +/*
> + * QError Module
> + *
> + * Copyright (C) 2009 Red Hat Inc.
> + *
> + * Authors:
> + *  Luiz Capitulino <lcapitulino@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + */
> +
> +#include "qemu-common.h"
> +#include "qerror.h"
> +#include "monitor.h"
> +#include "qjson.h"
> +
> +static void GCC_FMT_ATTR(2, 3) qerror_abort(const QError *qerr,
> +                                            const char *fmt, ...)
> +{
> +    va_list ap;
> +
> +    fprintf(stderr, "qerror: bad call in function '%s':\n", qerr->func);
> +    fprintf(stderr, "qerror: -> ");
> +
> +    va_start(ap, fmt);
> +    vfprintf(stderr, fmt, ap);
> +    va_end(ap);
> +
> +    fprintf(stderr, "\nqerror: call at %s:%d\n", qerr->file, qerr->linenr);
> +    abort();
> +}
> +
> +static void GCC_FMT_ATTR(2, 0) qerror_set_data(QError *qerr,
> +                                               const char *fmt, va_list *va)
> +{
> +    QObject *obj;
> +
> +    obj = qobject_from_jsonv(fmt, va);
> +    if (!obj) {
> +        qerror_abort(qerr, "invalid format '%s'", fmt);
> +    }
> +    if (qobject_type(obj) != QTYPE_QDICT) {
> +        qerror_abort(qerr, "error format is not a QDict '%s'", fmt);
> +    }
> +
> +    qerr->error = qobject_to_qdict(obj);
> +
> +    obj = qdict_get(qerr->error, "class");
> +    if (!obj) {
> +        qerror_abort(qerr, "missing 'class' key in '%s'", fmt);
> +    }
> +    if (qobject_type(obj) != QTYPE_QSTRING) {
> +        qerror_abort(qerr, "'class' key value should be a QString");
> +    }
> +    
> +    obj = qdict_get(qerr->error, "data");
> +    if (!obj) {
> +        qerror_abort(qerr, "missing 'data' key in '%s'", fmt);
> +    }
> +    if (qobject_type(obj) != QTYPE_QDICT) {
> +        qerror_abort(qerr, "'data' key value should be a QDICT");
> +    }
> +}
> +
> +/**
> + * qerror_from_info(): Create a new QError from error information
> + *
> + * The information consists of:
> + *
> + * - file   the file name of where the error occurred
> + * - linenr the line number of where the error occurred
> + * - func   the function name of where the error occurred
> + * - fmt    JSON printf-like dictionary, there must exist keys 'class' and
> + *          'data'
> + * - va     va_list of all arguments specified by fmt
> + *
> + * Return strong reference.
> + */
> +QError *qerror_from_info(const char *file, int linenr, const char *func,
> +                         const char *fmt, va_list *va)
> +{
> +    QError *qerr;
> +
> +    qerr = qerror_new();
> +    loc_save(&qerr->loc);
> +    qerr->linenr = linenr;
> +    qerr->file = file;
> +    qerr->func = func;
> +
> +    if (!fmt) {
> +        qerror_abort(qerr, "QDict not specified");
> +    }
> +
> +    qerror_set_data(qerr, fmt, va);
> +    qerror_set_desc(qerr, fmt);
> +
> +    return qerr;
> +}
> +
> +/**
> + * qerror_print(): Print QError data
> + *
> + * This function will print the member 'desc' of the specified QError object,
> + * it uses error_report() for this, so that the output is routed to the right
> + * place (ie. stderr or Monitor's device).
> + */
> +void qerror_print(QError *qerror)
> +{
> +    QString *qstring = qerror_human(qerror);
> +    loc_push_restore(&qerror->loc);
> +    error_report("%s", qstring_get_str(qstring));
> +    loc_pop(&qerror->loc);
> +    QDECREF(qstring);
> +}
> +
> +void qerror_report_internal(const char *file, int linenr, const char *func,
> +                            const char *fmt, ...)
> +{
> +    va_list va;
> +    QError *qerror;
> +
> +    va_start(va, fmt);
> +    qerror = qerror_from_info(file, linenr, func, fmt, &va);
> +    va_end(va);
> +
> +    qerror_print(qerror);
> +    QDECREF(qerror);
> +}
> +
> +
> diff --git a/qerror.c b/qerror.c
> index 13d53c9..78d3884 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -243,39 +243,7 @@ static void GCC_FMT_ATTR(2, 3) qerror_abort(const QError *qerr,
>      abort();
>  }
>  
> -static void GCC_FMT_ATTR(2, 0) qerror_set_data(QError *qerr,
> -                                               const char *fmt, va_list *va)
> -{
> -    QObject *obj;
> -
> -    obj = qobject_from_jsonv(fmt, va);
> -    if (!obj) {
> -        qerror_abort(qerr, "invalid format '%s'", fmt);
> -    }
> -    if (qobject_type(obj) != QTYPE_QDICT) {
> -        qerror_abort(qerr, "error format is not a QDict '%s'", fmt);
> -    }
> -
> -    qerr->error = qobject_to_qdict(obj);
> -
> -    obj = qdict_get(qerr->error, "class");
> -    if (!obj) {
> -        qerror_abort(qerr, "missing 'class' key in '%s'", fmt);
> -    }
> -    if (qobject_type(obj) != QTYPE_QSTRING) {
> -        qerror_abort(qerr, "'class' key value should be a QString");
> -    }
> -    
> -    obj = qdict_get(qerr->error, "data");
> -    if (!obj) {
> -        qerror_abort(qerr, "missing 'data' key in '%s'", fmt);
> -    }
> -    if (qobject_type(obj) != QTYPE_QDICT) {
> -        qerror_abort(qerr, "'data' key value should be a QDICT");
> -    }
> -}
> -
> -static void qerror_set_desc(QError *qerr, const char *fmt)
> +void qerror_set_desc(QError *qerr, const char *fmt)
>  {
>      int i;
>  
> @@ -291,41 +259,6 @@ static void qerror_set_desc(QError *qerr, const char *fmt)
>      qerror_abort(qerr, "error format '%s' not found", fmt);
>  }
>  
> -/**
> - * qerror_from_info(): Create a new QError from error information
> - *
> - * The information consists of:
> - *
> - * - file   the file name of where the error occurred
> - * - linenr the line number of where the error occurred
> - * - func   the function name of where the error occurred
> - * - fmt    JSON printf-like dictionary, there must exist keys 'class' and
> - *          'data'
> - * - va     va_list of all arguments specified by fmt
> - *
> - * Return strong reference.
> - */
> -QError *qerror_from_info(const char *file, int linenr, const char *func,
> -                         const char *fmt, va_list *va)
> -{
> -    QError *qerr;
> -
> -    qerr = qerror_new();
> -    loc_save(&qerr->loc);
> -    qerr->linenr = linenr;
> -    qerr->file = file;
> -    qerr->func = func;
> -
> -    if (!fmt) {
> -        qerror_abort(qerr, "QDict not specified");
> -    }
> -
> -    qerror_set_data(qerr, fmt, va);
> -    qerror_set_desc(qerr, fmt);
> -
> -    return qerr;
> -}
> -
>  static void parse_error(const QErrorStringTable *entry, int c)
>  {
>  #if 0
> @@ -441,40 +374,6 @@ QString *qerror_human(const QError *qerror)
>  }
>  
>  /**
> - * qerror_print(): Print QError data
> - *
> - * This function will print the member 'desc' of the specified QError object,
> - * it uses error_report() for this, so that the output is routed to the right
> - * place (ie. stderr or Monitor's device).
> - */
> -void qerror_print(QError *qerror)
> -{
> -    QString *qstring = qerror_human(qerror);
> -    loc_push_restore(&qerror->loc);
> -    error_report("%s", qstring_get_str(qstring));
> -    loc_pop(&qerror->loc);
> -    QDECREF(qstring);
> -}
> -
> -void qerror_report_internal(const char *file, int linenr, const char *func,
> -                            const char *fmt, ...)
> -{
> -    va_list va;
> -    QError *qerror;
> -
> -    va_start(va, fmt);
> -    qerror = qerror_from_info(file, linenr, func, fmt, &va);
> -    va_end(va);
> -
> -    if (monitor_cur_is_qmp()) {
> -        monitor_set_error(cur_mon, qerror);
> -    } else {
> -        qerror_print(qerror);
> -        QDECREF(qerror);
> -    }
> -}
> -
> -/**
>   * qobject_to_qerror(): Convert a QObject into a QError
>   */
>  QError *qobject_to_qerror(const QObject *obj)
> diff --git a/qerror.h b/qerror.h
> index fd63ee9..495d0a4 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -43,6 +43,7 @@ void qerror_report_internal(const char *file, int linenr, const char *func,
>      qerror_report_internal(__FILE__, __LINE__, __func__, fmt, ## __VA_ARGS__)
>  QError *qobject_to_qerror(const QObject *obj);
>  QString *qerror_format(const char *fmt, QDict *error);
> +void qerror_set_desc(QError *qerr, const char *fmt);
>  
>  /*
>   * QError class list

[Qemu-devel] Re: [PATCH 04/11] qerror: split out the reporting bits of QError

[Anthony Liguori]

On 03/14/2011 02:18 PM, Luiz Capitulino wrote:
> On Fri, 11 Mar 2011 15:00:42 -0600
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>
>> These make it very hard to compile QError outside of QEMU.
> Why would someone do this?

libqmp

It's very nice to have client code that convert QMP errors to human 
readable strings.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 04/11] qerror: split out the reporting bits of QError

[Luiz Capitulino]

On Mon, 14 Mar 2011 14:24:37 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> On 03/14/2011 02:18 PM, Luiz Capitulino wrote:
> > On Fri, 11 Mar 2011 15:00:42 -0600
> > Anthony Liguori<aliguori@us.ibm.com>  wrote:
> >
> >> These make it very hard to compile QError outside of QEMU.
> > Why would someone do this?
> 
> libqmp
> 
> It's very nice to have client code that convert QMP errors to human 
> readable strings.

Ok. I'm not sure if I would bother doing this kind of change in this
series (as it's mainly re-working internal stuff) but,  this has to be
explained in the log message then.

[Qemu-devel] Re: [PATCH 04/11] qerror: split out the reporting bits of QError

[Anthony Liguori]

On 03/14/2011 02:30 PM, Luiz Capitulino wrote:
>> libqmp
>>
>> It's very nice to have client code that convert QMP errors to human
>> readable strings.
> Ok. I'm not sure if I would bother doing this kind of change in this
> series (as it's mainly re-working internal stuff) but,  this has to be
> explained in the log message then.

I'll clarify in the log.

There's a lot of code in this series to merge.  I'll do my best to make 
review as easy as possible but I need a bit of flexibility in terms of 
merging some infrastructure bits first followed by the bits that use it.

Everything is already written.  I just didn't want to bombard the list 
with a 200 patch series.

Regards,

Anthony Liguori

[Qemu-devel] [PATCH 05/11] qerror: add new error message for invalid enum values

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/qerror.c b/qerror.c
index 78d3884..5a1e637 100644
--- a/qerror.c
+++ b/qerror.c
@@ -109,6 +109,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Duplicate ID '%(id)' for %(object)",
     },
     {
+        .error_fmt = QERR_ENUM_VALUE_INVALID,
+        .desc      = "Enum value '%(value)' is invalid for type '%(type)'",
+    },
+    {
         .error_fmt = QERR_FD_NOT_FOUND,
         .desc      = "File descriptor named '%(name)' not found",
     },
diff --git a/qerror.h b/qerror.h
index 495d0a4..35e7253 100644
--- a/qerror.h
+++ b/qerror.h
@@ -98,6 +98,9 @@ void qerror_set_desc(QError *qerr, const char *fmt);
 #define QERR_DUPLICATE_ID \
     "{ 'class': 'DuplicateId', 'data': { 'id': %s, 'object': %s } }"
 
+#define QERR_ENUM_VALUE_INVALID \
+    "{ 'class': 'EnumValueInvalid', 'data': { 'type': %s, 'value': %s } }"
+
 #define QERR_FD_NOT_FOUND \
     "{ 'class': 'FdNotFound', 'data': { 'name': %s } }"
 
-- 
1.7.0.4

[Qemu-devel] Re: [PATCH 05/11] qerror: add new error message for invalid enum values

[Luiz Capitulino]

On Fri, 11 Mar 2011 15:00:43 -0600
Anthony Liguori <aliguori@us.ibm.com> wrote:

> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> diff --git a/qerror.c b/qerror.c
> index 78d3884..5a1e637 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -109,6 +109,10 @@ static const QErrorStringTable qerror_table[] = {
>          .desc      = "Duplicate ID '%(id)' for %(object)",
>      },
>      {
> +        .error_fmt = QERR_ENUM_VALUE_INVALID,
> +        .desc      = "Enum value '%(value)' is invalid for type '%(type)'",
> +    },

This doesn't seem to be used.

> +    {
>          .error_fmt = QERR_FD_NOT_FOUND,
>          .desc      = "File descriptor named '%(name)' not found",
>      },
> diff --git a/qerror.h b/qerror.h
> index 495d0a4..35e7253 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -98,6 +98,9 @@ void qerror_set_desc(QError *qerr, const char *fmt);
>  #define QERR_DUPLICATE_ID \
>      "{ 'class': 'DuplicateId', 'data': { 'id': %s, 'object': %s } }"
>  
> +#define QERR_ENUM_VALUE_INVALID \
> +    "{ 'class': 'EnumValueInvalid', 'data': { 'type': %s, 'value': %s } }"
> +
>  #define QERR_FD_NOT_FOUND \
>      "{ 'class': 'FdNotFound', 'data': { 'name': %s } }"
>  

[Qemu-devel] [PATCH 06/11] qerror: add JSON parsing error message

[Anthony Liguori]

Using a string like this is a cop-out.  I plan on changing this before 0.15.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/qerror.c b/qerror.c
index 5a1e637..c12dd3d 100644
--- a/qerror.c
+++ b/qerror.c
@@ -145,6 +145,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Invalid JSON syntax",
     },
     {
+        .error_fmt = QERR_JSON_PARSE_ERROR,
+        .desc      = "Error parsing JSON: %(message)",
+    },
+    {
         .error_fmt = QERR_KVM_MISSING_CAP,
         .desc      = "Using KVM without %(capability), %(feature) unavailable",
     },
diff --git a/qerror.h b/qerror.h
index 35e7253..a0fb98d 100644
--- a/qerror.h
+++ b/qerror.h
@@ -125,6 +125,9 @@ void qerror_set_desc(QError *qerr, const char *fmt);
 #define QERR_JSON_PARSING \
     "{ 'class': 'JSONParsing', 'data': {} }"
 
+#define QERR_JSON_PARSE_ERROR \
+    "{ 'class': 'JSONParseError', 'data': { 'message': %s } }"
+
 #define QERR_KVM_MISSING_CAP \
     "{ 'class': 'KVMMissingCap', 'data': { 'capability': %s, 'feature': %s } }"
 
-- 
1.7.0.4

[Qemu-devel] [PATCH 07/11] json: propagate error from parser

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/json-parser.c b/json-parser.c
index 6c06ef9..ac4063a 100644
--- a/json-parser.c
+++ b/json-parser.c
@@ -22,9 +22,11 @@
 #include "qbool.h"
 #include "json-parser.h"
 #include "json-lexer.h"
+#include "qerror.h"
 
 typedef struct JSONParserContext
 {
+    Error *err;
 } JSONParserContext;
 
 #define BUG_ON(cond) assert(!(cond))
@@ -95,11 +97,15 @@ static void GCC_FMT_ATTR(3, 4) parse_error(JSONParserContext *ctxt,
                                            QObject *token, const char *msg, ...)
 {
     va_list ap;
+    char message[1024];
     va_start(ap, msg);
-    fprintf(stderr, "parse error: ");
-    vfprintf(stderr, msg, ap);
-    fprintf(stderr, "\n");
+    vsnprintf(message, sizeof(message), msg, ap);
     va_end(ap);
+    if (ctxt->err) {
+        error_free(ctxt->err);
+        ctxt->err = NULL;
+    }
+    error_set(&ctxt->err, QERR_JSON_PARSE_ERROR, message);
 }
 
 /**
@@ -565,6 +571,11 @@ static QObject *parse_value(JSONParserContext *ctxt, QList **tokens, va_list *ap
 
 QObject *json_parser_parse(QList *tokens, va_list *ap)
 {
+    return json_parser_parse_err(tokens, ap, NULL);
+}
+
+QObject *json_parser_parse_err(QList *tokens, va_list *ap, Error **errp)
+{
     JSONParserContext ctxt = {};
     QList *working = qlist_copy(tokens);
     QObject *result;
@@ -573,5 +584,7 @@ QObject *json_parser_parse(QList *tokens, va_list *ap)
 
     QDECREF(working);
 
+    error_propagate(errp, ctxt.err);
+
     return result;
 }
diff --git a/json-parser.h b/json-parser.h
index 97f43f6..8f2b5ec 100644
--- a/json-parser.h
+++ b/json-parser.h
@@ -16,7 +16,9 @@
 
 #include "qemu-common.h"
 #include "qlist.h"
+#include "error.h"
 
 QObject *json_parser_parse(QList *tokens, va_list *ap);
+QObject *json_parser_parse_err(QList *tokens, va_list *ap, Error **errp);
 
 #endif
-- 
1.7.0.4

[Qemu-devel] [PATCH 08/11] json-lexer: reset the lexer state on an invalid token

[Anthony Liguori]

Not everything handles errors from json parsing gracefully.  By at least
resetting the lexer, we'll start generating valid tokens again and hopefully
recover the stream.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/json-lexer.c b/json-lexer.c
index c736f42..834d7af 100644
--- a/json-lexer.c
+++ b/json-lexer.c
@@ -303,6 +303,9 @@ static int json_lexer_feed_char(JSONLexer *lexer, char ch)
             new_state = IN_START;
             break;
         case ERROR:
+            QDECREF(lexer->token);
+            lexer->token = qstring_new();
+            new_state = IN_START;
             return -EINVAL;
         default:
             break;
-- 
1.7.0.4

[Qemu-devel] Re: [PATCH 08/11] json-lexer: reset the lexer state on an invalid token

[Luiz Capitulino]

On Fri, 11 Mar 2011 15:00:46 -0600
Anthony Liguori <aliguori@us.ibm.com> wrote:

> Not everything handles errors from json parsing gracefully.  By at least
> resetting the lexer, we'll start generating valid tokens again and hopefully
> recover the stream.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> diff --git a/json-lexer.c b/json-lexer.c
> index c736f42..834d7af 100644
> --- a/json-lexer.c
> +++ b/json-lexer.c
> @@ -303,6 +303,9 @@ static int json_lexer_feed_char(JSONLexer *lexer, char ch)
>              new_state = IN_START;
>              break;
>          case ERROR:
> +            QDECREF(lexer->token);
> +            lexer->token = qstring_new();
> +            new_state = IN_START;
>              return -EINVAL;

This makes the parser accept broken input like:

  { "execute": xxxxx }
  {"return": {}}

  { "execute": _ }
  {"return": {}}

Today, it handles this kind of input correctly:

  { "execute": xxxxx }
  {"error": {"class": "JSONParsing", "desc": "Invalid JSON syntax", "data": {}}}

Although it also accepts broken stuff today, like:

 { "execute": ___"query-block" }


>          default:
>              break;

[Qemu-devel] Re: [PATCH 08/11] json-lexer: reset the lexer state on an invalid token

[Anthony Liguori]

On 03/14/2011 02:22 PM, Luiz Capitulino wrote:
> On Fri, 11 Mar 2011 15:00:46 -0600
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>
>> Not everything handles errors from json parsing gracefully.  By at least
>> resetting the lexer, we'll start generating valid tokens again and hopefully
>> recover the stream.
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>
>> diff --git a/json-lexer.c b/json-lexer.c
>> index c736f42..834d7af 100644
>> --- a/json-lexer.c
>> +++ b/json-lexer.c
>> @@ -303,6 +303,9 @@ static int json_lexer_feed_char(JSONLexer *lexer, char ch)
>>               new_state = IN_START;
>>               break;
>>           case ERROR:
>> +            QDECREF(lexer->token);
>> +            lexer->token = qstring_new();
>> +            new_state = IN_START;
>>               return -EINVAL;
> This makes the parser accept broken input like:
>
>    { "execute": xxxxx }
>    {"return": {}}

This is a bug in the current QMP server.  Here's how my new QMP server 
responds:

{"QMP": {"version": {"qemu": {"micro": 50, "minor": 13, "major": 0}, 
"package": ""}, "capabilities": []}}
{"error": {"class": "JSONParseError", "data": {"message": "Missing value 
in dict"}}}

>    { "execute": _ }
>    {"return": {}}

Likewise, the new QMP server does not respond to this at all (which 
confuses me TBH).

> Today, it handles this kind of input correctly:
>
>    { "execute": xxxxx }
>    {"error": {"class": "JSONParsing", "desc": "Invalid JSON syntax", "data": {}}}

The parser rejects this verses trying to get what it can out of it and 
passing that to QMP.  The idea here is to be more graceful in dealing 
with bad input and trying to recover.

> Although it also accepts broken stuff today, like:
>
>   { "execute": ___"query-block" }

This is really the server, not the parser.  The new server doesn't 
accept this.

I guess QMP today just ignores the incoming QObject in capabilities mode 
and always returns {}.  You'll see the same thing with:

{ "execute": "not-a-valid-command" }
{"return": {}}

But once you're in command mode, it does the right thing.

Regards,

Anthony Liguori

>>           default:
>>               break;

[Qemu-devel] Re: [PATCH 08/11] json-lexer: reset the lexer state on an invalid token

[Luiz Capitulino]

On Mon, 14 Mar 2011 14:43:48 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> On 03/14/2011 02:22 PM, Luiz Capitulino wrote:
> > On Fri, 11 Mar 2011 15:00:46 -0600
> > Anthony Liguori<aliguori@us.ibm.com>  wrote:
> >
> >> Not everything handles errors from json parsing gracefully.  By at least
> >> resetting the lexer, we'll start generating valid tokens again and hopefully
> >> recover the stream.
> >>
> >> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
> >>
> >> diff --git a/json-lexer.c b/json-lexer.c
> >> index c736f42..834d7af 100644
> >> --- a/json-lexer.c
> >> +++ b/json-lexer.c
> >> @@ -303,6 +303,9 @@ static int json_lexer_feed_char(JSONLexer *lexer, char ch)
> >>               new_state = IN_START;
> >>               break;
> >>           case ERROR:
> >> +            QDECREF(lexer->token);
> >> +            lexer->token = qstring_new();
> >> +            new_state = IN_START;
> >>               return -EINVAL;
> > This makes the parser accept broken input like:
> >
> >    { "execute": xxxxx }
> >    {"return": {}}
> 
> This is a bug in the current QMP server.  Here's how my new QMP server 
> responds:
> 
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 13, "major": 0}, 
> "package": ""}, "capabilities": []}}
> {"error": {"class": "JSONParseError", "data": {"message": "Missing value 
> in dict"}}}

How do you handle it? Do you check the return of json_message_parser_feed()?

If that's the case, then the real problem in the current server is that we
use qemu's chardev interface and its read handler doesn't allow for
signaling errors. I did not consider not using it.

By looking at your branch I have the impression you wrote your own stuff,
am I right? If yes, doesn't it duplicate the chardev implementation?

> 
> >    { "execute": _ }
> >    {"return": {}}
> 
> Likewise, the new QMP server does not respond to this at all (which 
> confuses me TBH).
> 
> > Today, it handles this kind of input correctly:
> >
> >    { "execute": xxxxx }
> >    {"error": {"class": "JSONParsing", "desc": "Invalid JSON syntax", "data": {}}}
> 
> The parser rejects this verses trying to get what it can out of it and 
> passing that to QMP.  The idea here is to be more graceful in dealing 
> with bad input and trying to recover.

I'm all for trying to recover, but we can't have varied responses for
bad input. It seems easier to just fail.
> 
> > Although it also accepts broken stuff today, like:
> >
> >   { "execute": ___"query-block" }
> 
> This is really the server, not the parser.  The new server doesn't 
> accept this.

This is probably the same as the xxxxx above.

> I guess QMP today just ignores the incoming QObject in capabilities mode 
> and always returns {}.  You'll see the same thing with:
> 
> { "execute": "not-a-valid-command" }
> {"return": {}}
> 
> But once you're in command mode, it does the right thing.

I can't reproduce it w/o this series applied:

{"QMP": {"version": {"qemu": {"micro": 50, "minor": 14, "major": 0}, "package": ""}, "capabilities": []}}
{ "execute": "not-a-valid-command" }
{"error": {"class": "CommandNotFound", "desc": "The command not-a-valid-command has not been found", "data": {"name": "not-a-valid-command"}}}

[Qemu-devel] Re: [PATCH 08/11] json-lexer: reset the lexer state on an invalid token

[Anthony Liguori]

On 03/14/2011 03:12 PM, Luiz Capitulino wrote:
> On Mon, 14 Mar 2011 14:43:48 -0500
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>
>> On 03/14/2011 02:22 PM, Luiz Capitulino wrote:
>>> On Fri, 11 Mar 2011 15:00:46 -0600
>>> Anthony Liguori<aliguori@us.ibm.com>   wrote:
>>>
>>>> Not everything handles errors from json parsing gracefully.  By at least
>>>> resetting the lexer, we'll start generating valid tokens again and hopefully
>>>> recover the stream.
>>>>
>>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>>>
>>>> diff --git a/json-lexer.c b/json-lexer.c
>>>> index c736f42..834d7af 100644
>>>> --- a/json-lexer.c
>>>> +++ b/json-lexer.c
>>>> @@ -303,6 +303,9 @@ static int json_lexer_feed_char(JSONLexer *lexer, char ch)
>>>>                new_state = IN_START;
>>>>                break;
>>>>            case ERROR:
>>>> +            QDECREF(lexer->token);
>>>> +            lexer->token = qstring_new();
>>>> +            new_state = IN_START;
>>>>                return -EINVAL;
>>> This makes the parser accept broken input like:
>>>
>>>     { "execute": xxxxx }
>>>     {"return": {}}
>> This is a bug in the current QMP server.  Here's how my new QMP server
>> responds:
>>
>> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 13, "major": 0},
>> "package": ""}, "capabilities": []}}
>> {"error": {"class": "JSONParseError", "data": {"message": "Missing value
>> in dict"}}}
> How do you handle it? Do you check the return of json_message_parser_feed()?
>
> If that's the case, then the real problem in the current server is that we
> use qemu's chardev interface and its read handler doesn't allow for
> signaling errors. I did not consider not using it.
>
> By looking at your branch I have the impression you wrote your own stuff,
> am I right? If yes, doesn't it duplicate the chardev implementation?

No, that test was with the chardev interface.  There is both a chardev 
server and a unix domain socket server.

I'm not really sure why the current server isn't working correctly.  I'd 
have to investigate.

>>>     { "execute": _ }
>>>     {"return": {}}
>> Likewise, the new QMP server does not respond to this at all (which
>> confuses me TBH).
>>
>>> Today, it handles this kind of input correctly:
>>>
>>>     { "execute": xxxxx }
>>>     {"error": {"class": "JSONParsing", "desc": "Invalid JSON syntax", "data": {}}}
>> The parser rejects this verses trying to get what it can out of it and
>> passing that to QMP.  The idea here is to be more graceful in dealing
>> with bad input and trying to recover.
> I'm all for trying to recover, but we can't have varied responses for
> bad input. It seems easier to just fail.

I think we need to make sure that we don't ever succeed in the face of 
bad input, right?

So far, none of the test cases (against the new QMP server) succeed 
given bad input.

>> I guess QMP today just ignores the incoming QObject in capabilities mode
>> and always returns {}.  You'll see the same thing with:
>>
>> { "execute": "not-a-valid-command" }
>> {"return": {}}
>>
>> But once you're in command mode, it does the right thing.
> I can't reproduce it w/o this series applied:
>
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 14, "major": 0}, "package": ""}, "capabilities": []}}
> { "execute": "not-a-valid-command" }
> {"error": {"class": "CommandNotFound", "desc": "The command not-a-valid-command has not been found", "data": {"name": "not-a-valid-command"}}}

Curious, maybe I'm remembering this wrong then.  Let me dig in a big.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 08/11] json-lexer: reset the lexer state on an invalid token

[Luiz Capitulino]

On Mon, 14 Mar 2011 15:30:33 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> On 03/14/2011 03:12 PM, Luiz Capitulino wrote:
> > On Mon, 14 Mar 2011 14:43:48 -0500
> > Anthony Liguori<aliguori@us.ibm.com>  wrote:
> >
> >> On 03/14/2011 02:22 PM, Luiz Capitulino wrote:
> >>> On Fri, 11 Mar 2011 15:00:46 -0600
> >>> Anthony Liguori<aliguori@us.ibm.com>   wrote:
> >>>
> >>>> Not everything handles errors from json parsing gracefully.  By at least
> >>>> resetting the lexer, we'll start generating valid tokens again and hopefully
> >>>> recover the stream.
> >>>>
> >>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
> >>>>
> >>>> diff --git a/json-lexer.c b/json-lexer.c
> >>>> index c736f42..834d7af 100644
> >>>> --- a/json-lexer.c
> >>>> +++ b/json-lexer.c
> >>>> @@ -303,6 +303,9 @@ static int json_lexer_feed_char(JSONLexer *lexer, char ch)
> >>>>                new_state = IN_START;
> >>>>                break;
> >>>>            case ERROR:
> >>>> +            QDECREF(lexer->token);
> >>>> +            lexer->token = qstring_new();
> >>>> +            new_state = IN_START;
> >>>>                return -EINVAL;
> >>> This makes the parser accept broken input like:
> >>>
> >>>     { "execute": xxxxx }
> >>>     {"return": {}}
> >> This is a bug in the current QMP server.  Here's how my new QMP server
> >> responds:
> >>
> >> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 13, "major": 0},
> >> "package": ""}, "capabilities": []}}
> >> {"error": {"class": "JSONParseError", "data": {"message": "Missing value
> >> in dict"}}}
> > How do you handle it? Do you check the return of json_message_parser_feed()?
> >
> > If that's the case, then the real problem in the current server is that we
> > use qemu's chardev interface and its read handler doesn't allow for
> > signaling errors. I did not consider not using it.
> >
> > By looking at your branch I have the impression you wrote your own stuff,
> > am I right? If yes, doesn't it duplicate the chardev implementation?
> 
> No, that test was with the chardev interface.  There is both a chardev 
> server and a unix domain socket server.
> 
> I'm not really sure why the current server isn't working correctly.  I'd 
> have to investigate.

That's the only problem wrt bad input that should be handled by the current
server but isn't afaik.

> >>>     { "execute": _ }
> >>>     {"return": {}}
> >> Likewise, the new QMP server does not respond to this at all (which
> >> confuses me TBH).
> >>
> >>> Today, it handles this kind of input correctly:
> >>>
> >>>     { "execute": xxxxx }
> >>>     {"error": {"class": "JSONParsing", "desc": "Invalid JSON syntax", "data": {}}}
> >> The parser rejects this verses trying to get what it can out of it and
> >> passing that to QMP.  The idea here is to be more graceful in dealing
> >> with bad input and trying to recover.
> > I'm all for trying to recover, but we can't have varied responses for
> > bad input. It seems easier to just fail.
> 
> I think we need to make sure that we don't ever succeed in the face of 
> bad input, right?

Right.

> So far, none of the test cases (against the new QMP server) succeed 
> given bad input.

But the server has to return an error, not responding is also wrong.

Also note that I'm not aware of this kind of thing happening with the
current server either (only with this series applied).

> >> I guess QMP today just ignores the incoming QObject in capabilities mode
> >> and always returns {}.  You'll see the same thing with:
> >>
> >> { "execute": "not-a-valid-command" }
> >> {"return": {}}
> >>
> >> But once you're in command mode, it does the right thing.
> > I can't reproduce it w/o this series applied:
> >
> > {"QMP": {"version": {"qemu": {"micro": 50, "minor": 14, "major": 0}, "package": ""}, "capabilities": []}}
> > { "execute": "not-a-valid-command" }
> > {"error": {"class": "CommandNotFound", "desc": "The command not-a-valid-command has not been found", "data": {"name": "not-a-valid-command"}}}
> 
> Curious, maybe I'm remembering this wrong then.  Let me dig in a big.
> 
> Regards,
> 
> Anthony Liguori
> 
> 

[Qemu-devel] [PATCH 09/11] json-lexer: limit the maximum size of a given token

[Anthony Liguori]

This is a security consideration.  We don't want a client to cause an arbitrary
amount of memory to be allocated in QEMU.  For now, we use a limit of 64MB
which should be large enough for any reasonably sized token.

This is important for parsing JSON from untrusted sources.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/json-lexer.c b/json-lexer.c
index 834d7af..3462c89 100644
--- a/json-lexer.c
+++ b/json-lexer.c
@@ -18,6 +18,8 @@
 #include "qemu-common.h"
 #include "json-lexer.h"
 
+#define MAX_TOKEN_SIZE (64ULL << 20)
+
 /*
  * \"([^\\\"]|(\\\"\\'\\\\\\/\\b\\f\\n\\r\\t\\u[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]))*\"
  * '([^\\']|(\\\"\\'\\\\\\/\\b\\f\\n\\r\\t\\u[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]))*'
@@ -312,6 +314,17 @@ static int json_lexer_feed_char(JSONLexer *lexer, char ch)
         }
         lexer->state = new_state;
     } while (!char_consumed);
+
+    /* Do not let a single token grow to an arbitrarily large size,
+     * this is a security consideration.
+     */
+    if (lexer->token->length > MAX_TOKEN_SIZE) {
+        lexer->emit(lexer, lexer->token, lexer->state, lexer->x, lexer->y);
+        QDECREF(lexer->token);
+        lexer->token = qstring_new();
+        lexer->state = IN_START;
+    }
+
     return 0;
 }
 
-- 
1.7.0.4

[Qemu-devel] Re: [PATCH 09/11] json-lexer: limit the maximum size of a given token

[Luiz Capitulino]

On Fri, 11 Mar 2011 15:00:47 -0600
Anthony Liguori <aliguori@us.ibm.com> wrote:

> This is a security consideration.  We don't want a client to cause an arbitrary
> amount of memory to be allocated in QEMU.  For now, we use a limit of 64MB
> which should be large enough for any reasonably sized token.
> 
> This is important for parsing JSON from untrusted sources.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> diff --git a/json-lexer.c b/json-lexer.c
> index 834d7af..3462c89 100644
> --- a/json-lexer.c
> +++ b/json-lexer.c
> @@ -18,6 +18,8 @@
>  #include "qemu-common.h"
>  #include "json-lexer.h"
>  
> +#define MAX_TOKEN_SIZE (64ULL << 20)
> +
>  /*
>   * \"([^\\\"]|(\\\"\\'\\\\\\/\\b\\f\\n\\r\\t\\u[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]))*\"
>   * '([^\\']|(\\\"\\'\\\\\\/\\b\\f\\n\\r\\t\\u[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]))*'
> @@ -312,6 +314,17 @@ static int json_lexer_feed_char(JSONLexer *lexer, char ch)
>          }
>          lexer->state = new_state;
>      } while (!char_consumed);
> +
> +    /* Do not let a single token grow to an arbitrarily large size,
> +     * this is a security consideration.
> +     */
> +    if (lexer->token->length > MAX_TOKEN_SIZE) {
> +        lexer->emit(lexer, lexer->token, lexer->state, lexer->x, lexer->y);
> +        QDECREF(lexer->token);
> +        lexer->token = qstring_new();
> +        lexer->state = IN_START;
> +    }

Entering an invalid token is an error, we should fail here. Which brings
two features:

 1. A test code could trigger this condition and check for the specific
    error code

 2. Developers will know when they hit the limit. Although I don't expect
    expect this to happen, there was talking about adding base64 support
    to transfer something (I can't remember what, but we never know how the
    protocol will evolve).

Also, by testing this I found that the parser seems to get confused when
the limit is reached: it stops responding.

> +
>      return 0;
>  }
>  

Re: [Qemu-devel] Re: [PATCH 09/11] json-lexer: limit the maximum size of a given token

[Anthony Liguori]

On 03/14/2011 02:25 PM, Luiz Capitulino wrote:
> On Fri, 11 Mar 2011 15:00:47 -0600
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>
>> This is a security consideration.  We don't want a client to cause an arbitrary
>> amount of memory to be allocated in QEMU.  For now, we use a limit of 64MB
>> which should be large enough for any reasonably sized token.
>>
>> This is important for parsing JSON from untrusted sources.
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>
>> diff --git a/json-lexer.c b/json-lexer.c
>> index 834d7af..3462c89 100644
>> --- a/json-lexer.c
>> +++ b/json-lexer.c
>> @@ -18,6 +18,8 @@
>>   #include "qemu-common.h"
>>   #include "json-lexer.h"
>>
>> +#define MAX_TOKEN_SIZE (64ULL<<  20)
>> +
>>   /*
>>    * \"([^\\\"]|(\\\"\\'\\\\\\/\\b\\f\\n\\r\\t\\u[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]))*\"
>>    * '([^\\']|(\\\"\\'\\\\\\/\\b\\f\\n\\r\\t\\u[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]))*'
>> @@ -312,6 +314,17 @@ static int json_lexer_feed_char(JSONLexer *lexer, char ch)
>>           }
>>           lexer->state = new_state;
>>       } while (!char_consumed);
>> +
>> +    /* Do not let a single token grow to an arbitrarily large size,
>> +     * this is a security consideration.
>> +     */
>> +    if (lexer->token->length>  MAX_TOKEN_SIZE) {
>> +        lexer->emit(lexer, lexer->token, lexer->state, lexer->x, lexer->y);
>> +        QDECREF(lexer->token);
>> +        lexer->token = qstring_new();
>> +        lexer->state = IN_START;
>> +    }
> Entering an invalid token is an error, we should fail here.

It's not so clear to me.

I think of it like GCC.  GCC doesn't bail out on the first invalid 
character the lexer encounters.  Instead, it records that an error has 
occurred and tries its best to recover.

The result is that instead of getting an error message about the first 
error in your code, you'll get a long listing of all the mistakes you 
made (usually).

One thing that makes this more difficult in our case is that when you're 
testing, we don't have a clear EOI to flush things out.  So a bad 
sequence of inputs might make the message parser wait for an a character 
that you're not necessarily inputting which makes the session appear 
hung.  Usually, if you throw a couple extra brackets around, you'll get 
back to valid input.

>   Which brings
> two features:
>
>   1. A test code could trigger this condition and check for the specific
>      error code
>
>   2. Developers will know when they hit the limit. Although I don't expect
>      expect this to happen, there was talking about adding base64 support
>      to transfer something (I can't remember what, but we never know how the
>      protocol will evolve).
>
> Also, by testing this I found that the parser seems to get confused when
> the limit is reached: it stops responding.

Actually, it does respond.  The lexer just takes an incredibly long time 
to process a large token because qstring_append_ch is incredibly slow 
:-)  If you drop the token size down to 64k instead of 64mb, it'll seem 
a lot more reasonable.

Regards,

Anthony Liguori

>> +
>>       return 0;
>>   }
>>
>

Re: [Qemu-devel] Re: [PATCH 09/11] json-lexer: limit the maximum size of a given token

[Luiz Capitulino]

On Mon, 14 Mar 2011 15:18:57 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 03/14/2011 02:25 PM, Luiz Capitulino wrote:
> > On Fri, 11 Mar 2011 15:00:47 -0600
> > Anthony Liguori<aliguori@us.ibm.com>  wrote:
> >
> >> This is a security consideration.  We don't want a client to cause an arbitrary
> >> amount of memory to be allocated in QEMU.  For now, we use a limit of 64MB
> >> which should be large enough for any reasonably sized token.
> >>
> >> This is important for parsing JSON from untrusted sources.
> >>
> >> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
> >>
> >> diff --git a/json-lexer.c b/json-lexer.c
> >> index 834d7af..3462c89 100644
> >> --- a/json-lexer.c
> >> +++ b/json-lexer.c
> >> @@ -18,6 +18,8 @@
> >>   #include "qemu-common.h"
> >>   #include "json-lexer.h"
> >>
> >> +#define MAX_TOKEN_SIZE (64ULL<<  20)
> >> +
> >>   /*
> >>    * \"([^\\\"]|(\\\"\\'\\\\\\/\\b\\f\\n\\r\\t\\u[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]))*\"
> >>    * '([^\\']|(\\\"\\'\\\\\\/\\b\\f\\n\\r\\t\\u[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]))*'
> >> @@ -312,6 +314,17 @@ static int json_lexer_feed_char(JSONLexer *lexer, char ch)
> >>           }
> >>           lexer->state = new_state;
> >>       } while (!char_consumed);
> >> +
> >> +    /* Do not let a single token grow to an arbitrarily large size,
> >> +     * this is a security consideration.
> >> +     */
> >> +    if (lexer->token->length>  MAX_TOKEN_SIZE) {
> >> +        lexer->emit(lexer, lexer->token, lexer->state, lexer->x, lexer->y);
> >> +        QDECREF(lexer->token);
> >> +        lexer->token = qstring_new();
> >> +        lexer->state = IN_START;
> >> +    }
> > Entering an invalid token is an error, we should fail here.
> 
> It's not so clear to me.
> 
> I think of it like GCC.  GCC doesn't bail out on the first invalid 
> character the lexer encounters.  Instead, it records that an error has 
> occurred and tries its best to recover.
> 
> The result is that instead of getting an error message about the first 
> error in your code, you'll get a long listing of all the mistakes you 
> made (usually).

I'm not a compiler expert, but I think compilers do this because it would be
very problematic from a usability perspective to report one error at time:
by reporting most errors per run, the compiler gives the user to fix as much
as possible programming mistakes w/o having to re-run.

Our target are not humans, though.

> One thing that makes this more difficult in our case is that when you're 
> testing, we don't have a clear EOI to flush things out.  So a bad 
> sequence of inputs might make the message parser wait for an a character 
> that you're not necessarily inputting which makes the session appear 
> hung.  Usually, if you throw a couple extra brackets around, you'll get 
> back to valid input.

Not sure if this is easy to do, but shouldn't we fail if we don't get
the expected character?

> >   Which brings
> > two features:
> >
> >   1. A test code could trigger this condition and check for the specific
> >      error code
> >
> >   2. Developers will know when they hit the limit. Although I don't expect
> >      expect this to happen, there was talking about adding base64 support
> >      to transfer something (I can't remember what, but we never know how the
> >      protocol will evolve).
> >
> > Also, by testing this I found that the parser seems to get confused when
> > the limit is reached: it stops responding.
> 
> Actually, it does respond.  The lexer just takes an incredibly long time 
> to process a large token because qstring_append_ch is incredibly slow 
> :-)  If you drop the token size down to 64k instead of 64mb, it'll seem 
> a lot more reasonable.

Actually, I dropped it down to 20 :) This is supposed to work like your 64k
suggestion, right?

> 
> Regards,
> 
> Anthony Liguori
> 
> >> +
> >>       return 0;
> >>   }
> >>
> >
> 

[Qemu-devel] [PATCH 10/11] json-streamer: limit the maximum recursion depth and maximum token count

[Anthony Liguori]

The JSON parse we use is a recursive decent parser which means that recursion
is used to backtrack.  To avoid stack overflows from malformed input, we need
to limit recursion depth.

Fortunately, we know the maximum recursion depth in the message parsing phase
so we can very easily check for unreasonably deep recursion.  If we find that
emit the current token stream and let the parser handle it.  The idea here is
that hopefully the stream will recover or at least error out gracefully.

We also limit the maximum token count.  We don't limit the number of tokens, but
rather the total memory used for tokens at any given point in time.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/json-streamer.c b/json-streamer.c
index f7e7a68..c7f43b0 100644
--- a/json-streamer.c
+++ b/json-streamer.c
@@ -18,6 +18,9 @@
 #include "json-lexer.h"
 #include "json-streamer.h"
 
+#define MAX_TOKEN_SIZE (64ULL << 20)
+#define MAX_NESTING (1ULL << 10)
+
 static void json_message_process_token(JSONLexer *lexer, QString *token, JSONTokenType type, int x, int y)
 {
     JSONMessageParser *parser = container_of(lexer, JSONMessageParser, lexer);
@@ -49,6 +52,8 @@ static void json_message_process_token(JSONLexer *lexer, QString *token, JSONTok
     qdict_put(dict, "x", qint_from_int(x));
     qdict_put(dict, "y", qint_from_int(y));
 
+    parser->token_size += token->length;
+
     qlist_append(parser->tokens, dict);
 
     if (parser->brace_count == 0 &&
@@ -56,6 +61,19 @@ static void json_message_process_token(JSONLexer *lexer, QString *token, JSONTok
         parser->emit(parser, parser->tokens);
         QDECREF(parser->tokens);
         parser->tokens = qlist_new();
+        parser->token_size = 0;
+    } else if (parser->token_size > MAX_TOKEN_SIZE ||
+               parser->bracket_count > MAX_NESTING ||
+               parser->brace_count > MAX_NESTING) {
+        /* Security consideration, we limit total memory allocated per object
+         * and the maximum recursion depth that a message can force.
+         */
+        parser->brace_count = 0;
+        parser->bracket_count = 0;
+        parser->emit(parser, parser->tokens);
+        QDECREF(parser->tokens);
+        parser->tokens = qlist_new();
+        parser->token_size = 0;
     }
 }
 
@@ -66,6 +84,7 @@ void json_message_parser_init(JSONMessageParser *parser,
     parser->brace_count = 0;
     parser->bracket_count = 0;
     parser->tokens = qlist_new();
+    parser->token_size = 0;
 
     json_lexer_init(&parser->lexer, json_message_process_token);
 }
diff --git a/json-streamer.h b/json-streamer.h
index 09f3bd7..f09bc4d 100644
--- a/json-streamer.h
+++ b/json-streamer.h
@@ -24,6 +24,7 @@ typedef struct JSONMessageParser
     int brace_count;
     int bracket_count;
     QList *tokens;
+    uint64_t token_size;
 } JSONMessageParser;
 
 void json_message_parser_init(JSONMessageParser *parser,
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH 10/11] json-streamer: limit the maximum recursion depth and maximum token count

[Michael Roth]

On 03/11/2011 03:00 PM, Anthony Liguori wrote:
> The JSON parse we use is a recursive decent parser which means that recursion
> is used to backtrack.  To avoid stack overflows from malformed input, we need
> to limit recursion depth.
>
> Fortunately, we know the maximum recursion depth in the message parsing phase
> so we can very easily check for unreasonably deep recursion.  If we find that
> emit the current token stream and let the parser handle it.  The idea here is
> that hopefully the stream will recover or at least error out gracefully.
>
> We also limit the maximum token count.  We don't limit the number of tokens, but
> rather the total memory used for tokens at any given point in time.
>
> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>
> diff --git a/json-streamer.c b/json-streamer.c
> index f7e7a68..c7f43b0 100644
> --- a/json-streamer.c
> +++ b/json-streamer.c
> @@ -18,6 +18,9 @@
>   #include "json-lexer.h"
>   #include "json-streamer.h"
>
> +#define MAX_TOKEN_SIZE (64ULL<<  20)
> +#define MAX_NESTING (1ULL<<  10)
> +
>   static void json_message_process_token(JSONLexer *lexer, QString *token, JSONTokenType type, int x, int y)
>   {
>       JSONMessageParser *parser = container_of(lexer, JSONMessageParser, lexer);
> @@ -49,6 +52,8 @@ static void json_message_process_token(JSONLexer *lexer, QString *token, JSONTok
>       qdict_put(dict, "x", qint_from_int(x));
>       qdict_put(dict, "y", qint_from_int(y));
>
> +    parser->token_size += token->length;
> +
>       qlist_append(parser->tokens, dict);
>
>       if (parser->brace_count == 0&&
> @@ -56,6 +61,19 @@ static void json_message_process_token(JSONLexer *lexer, QString *token, JSONTok
>           parser->emit(parser, parser->tokens);
>           QDECREF(parser->tokens);
>           parser->tokens = qlist_new();
> +        parser->token_size = 0;
> +    } else if (parser->token_size>  MAX_TOKEN_SIZE ||
> +               parser->bracket_count>  MAX_NESTING ||
> +               parser->brace_count>  MAX_NESTING) {
> +        /* Security consideration, we limit total memory allocated per object
> +         * and the maximum recursion depth that a message can force.
> +         */
> +        parser->brace_count = 0;
> +        parser->bracket_count = 0;
> +        parser->emit(parser, parser->tokens);

Should we even bother passing this to the parser? That's a lot stuff to 
churn on that we know isn't going to result in anything useful. If there 
was a proper object earlier in the stream it would've been emitted when 
brace/bracket count reached 0.

I think it might be nicer to do parser->emit(parser, NULL), and fix up 
json_parser_parse_err() to check for this and simply return NULL back to 
qmp_dispatch_err() or whoever is calling.

I think brace_count < 0 || bracket_count < 0 should get similar treatment.

> +        QDECREF(parser->tokens);
> +        parser->tokens = qlist_new();
> +        parser->token_size = 0;
>       }
>   }
>
> @@ -66,6 +84,7 @@ void json_message_parser_init(JSONMessageParser *parser,
>       parser->brace_count = 0;
>       parser->bracket_count = 0;
>       parser->tokens = qlist_new();
> +    parser->token_size = 0;
>
>       json_lexer_init(&parser->lexer, json_message_process_token);
>   }
> diff --git a/json-streamer.h b/json-streamer.h
> index 09f3bd7..f09bc4d 100644
> --- a/json-streamer.h
> +++ b/json-streamer.h
> @@ -24,6 +24,7 @@ typedef struct JSONMessageParser
>       int brace_count;
>       int bracket_count;
>       QList *tokens;
> +    uint64_t token_size;
>   } JSONMessageParser;
>
>   void json_message_parser_init(JSONMessageParser *parser,

Re: [Qemu-devel] [PATCH 10/11] json-streamer: limit the maximum recursion depth and maximum token count

[Anthony Liguori]

On 03/11/2011 05:16 PM, Michael Roth wrote:
>>           parser->emit(parser, parser->tokens);
>>           QDECREF(parser->tokens);
>>           parser->tokens = qlist_new();
>> +        parser->token_size = 0;
>> +    } else if (parser->token_size>  MAX_TOKEN_SIZE ||
>> +               parser->bracket_count>  MAX_NESTING ||
>> +               parser->brace_count>  MAX_NESTING) {
>> +        /* Security consideration, we limit total memory allocated 
>> per object
>> +         * and the maximum recursion depth that a message can force.
>> +         */
>> +        parser->brace_count = 0;
>> +        parser->bracket_count = 0;
>> +        parser->emit(parser, parser->tokens);
> @@ -56,6 +61,19 @@ static void json_message_process_token(JSONLexer 
> *lexer, QString *token, JSONTok
>
> Should we even bother passing this to the parser? That's a lot stuff 
> to churn on that we know isn't going to result in anything useful. If 
> there was a proper object earlier in the stream it would've been 
> emitted when brace/bracket count reached 0.
>
> I think it might be nicer to do parser->emit(parser, NULL), and fix up 
> json_parser_parse_err() to check for this and simply return NULL back 
> to qmp_dispatch_err() or whoever is calling.
>
> I think brace_count < 0 || bracket_count < 0 should get similar 
> treatment.

I think the main advantage of doing it this way is that we can test the 
maximum stack usage by just doing a simple:

(while true; do echo "{'key': "; done) | socat stdio 
unix:/path/to/qmp.sock > /dev/null

However, if we don't pass the token list to the parser, we need to know 
exactly what the maximum is and only generate that depth in order to 
test stack usage.

The parser needs to be robust against bad input so from a test 
perspective, I like the idea of passing something to the parser even if 
we know it's bad.

Regards,

Anthony Liguori

[Qemu-devel] [PATCH 11/11] json-parser: detect premature EOI

[Anthony Liguori]

We don't handle premature EOI very well and may SEGV.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/json-parser.c b/json-parser.c
index ac4063a..58e973b 100644
--- a/json-parser.c
+++ b/json-parser.c
@@ -275,10 +275,15 @@ out:
  */
 static int parse_pair(JSONParserContext *ctxt, QDict *dict, QList **tokens, va_list *ap)
 {
-    QObject *key, *token = NULL, *value, *peek;
+    QObject *key = NULL, *token = NULL, *value, *peek;
     QList *working = qlist_copy(*tokens);
 
     peek = qlist_peek(working);
+    if (peek == NULL) {
+        parse_error(ctxt, NULL, "premature EOI");
+        goto out;
+    }
+
     key = parse_value(ctxt, &working, ap);
     if (!key || qobject_type(key) != QTYPE_QSTRING) {
         parse_error(ctxt, peek, "key is not a string in object");
@@ -286,6 +291,11 @@ static int parse_pair(JSONParserContext *ctxt, QDict *dict, QList **tokens, va_l
     }
 
     token = qlist_pop(working);
+    if (token == NULL) {
+        parse_error(ctxt, NULL, "premature EOI");
+        goto out;
+    }
+
     if (!token_is_operator(token, ':')) {
         parse_error(ctxt, token, "missing : in object pair");
         goto out;
@@ -321,6 +331,10 @@ static QObject *parse_object(JSONParserContext *ctxt, QList **tokens, va_list *a
     QList *working = qlist_copy(*tokens);
 
     token = qlist_pop(working);
+    if (token == NULL) {
+        goto out;
+    }
+
     if (!token_is_operator(token, '{')) {
         goto out;
     }
@@ -330,12 +344,22 @@ static QObject *parse_object(JSONParserContext *ctxt, QList **tokens, va_list *a
     dict = qdict_new();
 
     peek = qlist_peek(working);
+    if (peek == NULL) {
+        parse_error(ctxt, NULL, "premature EOI");
+        goto out;
+    }
+
     if (!token_is_operator(peek, '}')) {
         if (parse_pair(ctxt, dict, &working, ap) == -1) {
             goto out;
         }
 
         token = qlist_pop(working);
+        if (token == NULL) {
+            parse_error(ctxt, NULL, "premature EOI");
+            goto out;
+        }
+
         while (!token_is_operator(token, '}')) {
             if (!token_is_operator(token, ',')) {
                 parse_error(ctxt, token, "expected separator in dict");
@@ -349,6 +373,10 @@ static QObject *parse_object(JSONParserContext *ctxt, QList **tokens, va_list *a
             }
 
             token = qlist_pop(working);
+            if (token == NULL) {
+                parse_error(ctxt, NULL, "premature EOI");
+                goto out;
+            }
         }
         qobject_decref(token);
         token = NULL;
@@ -377,6 +405,10 @@ static QObject *parse_array(JSONParserContext *ctxt, QList **tokens, va_list *ap
     QList *working = qlist_copy(*tokens);
 
     token = qlist_pop(working);
+    if (token == NULL) {
+        goto out;
+    }
+
     if (!token_is_operator(token, '[')) {
         goto out;
     }
@@ -386,6 +418,11 @@ static QObject *parse_array(JSONParserContext *ctxt, QList **tokens, va_list *ap
     list = qlist_new();
 
     peek = qlist_peek(working);
+    if (peek == NULL) {
+        parse_error(ctxt, NULL, "premature EOI");
+        goto out;
+    }
+
     if (!token_is_operator(peek, ']')) {
         QObject *obj;
 
@@ -398,6 +435,11 @@ static QObject *parse_array(JSONParserContext *ctxt, QList **tokens, va_list *ap
         qlist_append_obj(list, obj);
 
         token = qlist_pop(working);
+        if (token == NULL) {
+            parse_error(ctxt, NULL, "premature EOI");
+            goto out;
+        }
+
         while (!token_is_operator(token, ']')) {
             if (!token_is_operator(token, ',')) {
                 parse_error(ctxt, token, "expected separator in list");
@@ -416,6 +458,10 @@ static QObject *parse_array(JSONParserContext *ctxt, QList **tokens, va_list *ap
             qlist_append_obj(list, obj);
 
             token = qlist_pop(working);
+            if (token == NULL) {
+                parse_error(ctxt, NULL, "premature EOI");
+                goto out;
+            }
         }
 
         qobject_decref(token);
@@ -444,6 +490,9 @@ static QObject *parse_keyword(JSONParserContext *ctxt, QList **tokens)
     QList *working = qlist_copy(*tokens);
 
     token = qlist_pop(working);
+    if (token == NULL) {
+        goto out;
+    }
 
     if (token_get_type(token) != JSON_KEYWORD) {
         goto out;
@@ -481,6 +530,9 @@ static QObject *parse_escape(JSONParserContext *ctxt, QList **tokens, va_list *a
     }
 
     token = qlist_pop(working);
+    if (token == NULL) {
+        goto out;
+    }
 
     if (token_is_escape(token, "%p")) {
         obj = va_arg(*ap, QObject *);
@@ -520,6 +572,10 @@ static QObject *parse_literal(JSONParserContext *ctxt, QList **tokens)
     QList *working = qlist_copy(*tokens);
 
     token = qlist_pop(working);
+    if (token == NULL) {
+        goto out;
+    }
+
     switch (token_get_type(token)) {
     case JSON_STRING:
         obj = QOBJECT(qstring_from_escaped_str(ctxt, token));
-- 
1.7.0.4