Patchwork [29/34] qmp: switch to the new error format on the wire

login
register
mail settings
Submitter Luiz Capitulino
Date Aug. 2, 2012, 1:02 a.m.
Message ID <1343869374-23417-30-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/174666/
State New
Headers show

Comments

Luiz Capitulino - Aug. 2, 2012, 1:02 a.m.
IMPORTANT: this BREAKS QMP's compatibility for the error response.

This commit changes QMP's wire protocol to make use of the simpler
error format introduced by previous commits.

There are two important (and mostly incompatible) changes:

 1. Almost all error classes have been replaced by GenericError. The
    only classes that are still supported for compatibility with
    libvirt are: CommandNotFound, DeviceNotActive, KVMMissingCap,
    DeviceNotFound and MigrationExpected

 2. The 'data' field of the error dictionary is gone

As an example, an error response like:

  { "error": { "class": "DeviceNotRemovable",
               "data": { "device": "virtio0" },
               "desc": "Device 'virtio0' is not removable" } }

Will now be emitted as:

  { "error": { "class": "GenericError",
               "desc": "Device 'virtio0' is not removable" } }

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 QMP/qmp-spec.txt | 10 +++-------
 monitor.c        | 18 +++++++++++++-----
 qmp-commands.hx  |  3 +--
 3 files changed, 17 insertions(+), 14 deletions(-)
Markus Armbruster - Aug. 2, 2012, 5:12 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> IMPORTANT: this BREAKS QMP's compatibility for the error response.
>
> This commit changes QMP's wire protocol to make use of the simpler
> error format introduced by previous commits.
>
> There are two important (and mostly incompatible) changes:
>
>  1. Almost all error classes have been replaced by GenericError. The
>     only classes that are still supported for compatibility with
>     libvirt are: CommandNotFound, DeviceNotActive, KVMMissingCap,
>     DeviceNotFound and MigrationExpected
>
>  2. The 'data' field of the error dictionary is gone

Making use of license from qmp-commands.hx:

    3. Errors, in special, are not documented. Applications should NOT check
       for specific errors classes or data (it's strongly recommended to only
       check for the "error" key)

> As an example, an error response like:
>
>   { "error": { "class": "DeviceNotRemovable",
>                "data": { "device": "virtio0" },
>                "desc": "Device 'virtio0' is not removable" } }
>
> Will now be emitted as:
>
>   { "error": { "class": "GenericError",
>                "desc": "Device 'virtio0' is not removable" } }
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  QMP/qmp-spec.txt | 10 +++-------
>  monitor.c        | 18 +++++++++++++-----
>  qmp-commands.hx  |  3 +--
>  3 files changed, 17 insertions(+), 14 deletions(-)

Doesn't qapi-schema.json need updating, too?  It mentions specific error
classes that go away in this patch, such as IOError.

>
> diff --git a/QMP/qmp-spec.txt b/QMP/qmp-spec.txt
> index 1ba916c..a277896 100644
> --- a/QMP/qmp-spec.txt
> +++ b/QMP/qmp-spec.txt
> @@ -106,14 +106,11 @@ completed because of an error condition.
>  
>  The format is:
>  
> -{ "error": { "class": json-string, "data": json-object, "desc": json-string },
> -  "id": json-value }
> +{ "error": { "class": json-string, "desc": json-string }, "id": json-value }
>  
>   Where,
>  
> -- The "class" member contains the error class name (eg. "ServiceUnavailable")
> -- The "data" member contains specific error data and is defined in a
> -  per-command basis, it will be an empty json-object if the error has no data
> +- The "class" member contains the error class name (eg. "GenericError")
>  - The "desc" member is a human-readable error message. Clients should
>    not attempt to parse this message.
>  - The "id" member contains the transaction identification associated with
> @@ -173,8 +170,7 @@ S: {"return": {"enabled": true, "present": true}, "id": "example"}
>  ------------------
>  
>  C: { "execute": }
> -S: {"error": {"class": "JSONParsing", "desc": "Invalid JSON syntax", "data":
> -{}}}
> +S: {"error": {"class": "GenericError", "desc": "Invalid JSON syntax" } }
>  
>  3.5 Powerdown event
>  -------------------
> diff --git a/monitor.c b/monitor.c
> index aa57167..3694590 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -353,16 +353,26 @@ static void monitor_json_emitter(Monitor *mon, const QObject *data)
>      QDECREF(json);
>  }
>  
> +static QDict *build_qmp_error_dict(const QError *err)
> +{
> +    QObject *obj;
> +
> +    obj = qobject_from_jsonf("{ 'error': { 'class': %s, 'desc': %p } }",
> +                             ErrorClass_lookup[err->err_class],
> +                             qerror_human(err));
> +
> +    return qobject_to_qdict(obj);
> +}
> +
>  static void monitor_protocol_emitter(Monitor *mon, QObject *data)
>  {
>      QDict *qmp;
>  
>      trace_monitor_protocol_emitter(mon);
>  
> -    qmp = qdict_new();
> -
>      if (!monitor_has_error(mon)) {
>          /* success response */
> +        qmp = qdict_new();
>          if (data) {
>              qobject_incref(data);
>              qdict_put_obj(qmp, "return", data);
> @@ -372,9 +382,7 @@ static void monitor_protocol_emitter(Monitor *mon, QObject *data)
>          }
>      } else {
>          /* error response */
> -        qdict_put(mon->error->error, "desc", qerror_human(mon->error));
> -        qdict_put(qmp, "error", mon->error->error);
> -        QINCREF(mon->error->error);
> +        qmp = build_qmp_error_dict(mon->error);
>          QDECREF(mon->error);
>          mon->error = NULL;
>      }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index e3cf3c5..e520b51 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -435,8 +435,7 @@ Example:
>  -> { "execute": "inject-nmi" }
>  <- { "return": {} }
>  
> -Note: inject-nmi is only supported for x86 guest currently, it will
> -      returns "Unsupported" error for non-x86 guest.
> +Note: inject-nmi is only supported for x86 guests.
>  
>  EQMP

Suggest:

    Note: inject-nmi fails when the guest doesn't support injecting.
    Currently, only x86 guests do.
Luiz Capitulino - Aug. 2, 2012, 5:19 p.m.
On Thu, 02 Aug 2012 19:12:03 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > IMPORTANT: this BREAKS QMP's compatibility for the error response.
> >
> > This commit changes QMP's wire protocol to make use of the simpler
> > error format introduced by previous commits.
> >
> > There are two important (and mostly incompatible) changes:
> >
> >  1. Almost all error classes have been replaced by GenericError. The
> >     only classes that are still supported for compatibility with
> >     libvirt are: CommandNotFound, DeviceNotActive, KVMMissingCap,
> >     DeviceNotFound and MigrationExpected
> >
> >  2. The 'data' field of the error dictionary is gone
> 
> Making use of license from qmp-commands.hx:
> 
>     3. Errors, in special, are not documented. Applications should NOT check
>        for specific errors classes or data (it's strongly recommended to only
>        check for the "error" key)
> 
> > As an example, an error response like:
> >
> >   { "error": { "class": "DeviceNotRemovable",
> >                "data": { "device": "virtio0" },
> >                "desc": "Device 'virtio0' is not removable" } }
> >
> > Will now be emitted as:
> >
> >   { "error": { "class": "GenericError",
> >                "desc": "Device 'virtio0' is not removable" } }
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  QMP/qmp-spec.txt | 10 +++-------
> >  monitor.c        | 18 +++++++++++++-----
> >  qmp-commands.hx  |  3 +--
> >  3 files changed, 17 insertions(+), 14 deletions(-)
> 
> Doesn't qapi-schema.json need updating, too?  It mentions specific error
> classes that go away in this patch, such as IOError.

Oh, true. I remembered about that when I started working on v1 but then
forgot about doing it. It would be also nice to add some examples to
docs/writing-qmp-commands.txt.

> 
> >
> > diff --git a/QMP/qmp-spec.txt b/QMP/qmp-spec.txt
> > index 1ba916c..a277896 100644
> > --- a/QMP/qmp-spec.txt
> > +++ b/QMP/qmp-spec.txt
> > @@ -106,14 +106,11 @@ completed because of an error condition.
> >  
> >  The format is:
> >  
> > -{ "error": { "class": json-string, "data": json-object, "desc": json-string },
> > -  "id": json-value }
> > +{ "error": { "class": json-string, "desc": json-string }, "id": json-value }
> >  
> >   Where,
> >  
> > -- The "class" member contains the error class name (eg. "ServiceUnavailable")
> > -- The "data" member contains specific error data and is defined in a
> > -  per-command basis, it will be an empty json-object if the error has no data
> > +- The "class" member contains the error class name (eg. "GenericError")
> >  - The "desc" member is a human-readable error message. Clients should
> >    not attempt to parse this message.
> >  - The "id" member contains the transaction identification associated with
> > @@ -173,8 +170,7 @@ S: {"return": {"enabled": true, "present": true}, "id": "example"}
> >  ------------------
> >  
> >  C: { "execute": }
> > -S: {"error": {"class": "JSONParsing", "desc": "Invalid JSON syntax", "data":
> > -{}}}
> > +S: {"error": {"class": "GenericError", "desc": "Invalid JSON syntax" } }
> >  
> >  3.5 Powerdown event
> >  -------------------
> > diff --git a/monitor.c b/monitor.c
> > index aa57167..3694590 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -353,16 +353,26 @@ static void monitor_json_emitter(Monitor *mon, const QObject *data)
> >      QDECREF(json);
> >  }
> >  
> > +static QDict *build_qmp_error_dict(const QError *err)
> > +{
> > +    QObject *obj;
> > +
> > +    obj = qobject_from_jsonf("{ 'error': { 'class': %s, 'desc': %p } }",
> > +                             ErrorClass_lookup[err->err_class],
> > +                             qerror_human(err));
> > +
> > +    return qobject_to_qdict(obj);
> > +}
> > +
> >  static void monitor_protocol_emitter(Monitor *mon, QObject *data)
> >  {
> >      QDict *qmp;
> >  
> >      trace_monitor_protocol_emitter(mon);
> >  
> > -    qmp = qdict_new();
> > -
> >      if (!monitor_has_error(mon)) {
> >          /* success response */
> > +        qmp = qdict_new();
> >          if (data) {
> >              qobject_incref(data);
> >              qdict_put_obj(qmp, "return", data);
> > @@ -372,9 +382,7 @@ static void monitor_protocol_emitter(Monitor *mon, QObject *data)
> >          }
> >      } else {
> >          /* error response */
> > -        qdict_put(mon->error->error, "desc", qerror_human(mon->error));
> > -        qdict_put(qmp, "error", mon->error->error);
> > -        QINCREF(mon->error->error);
> > +        qmp = build_qmp_error_dict(mon->error);
> >          QDECREF(mon->error);
> >          mon->error = NULL;
> >      }
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index e3cf3c5..e520b51 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -435,8 +435,7 @@ Example:
> >  -> { "execute": "inject-nmi" }
> >  <- { "return": {} }
> >  
> > -Note: inject-nmi is only supported for x86 guest currently, it will
> > -      returns "Unsupported" error for non-x86 guest.
> > +Note: inject-nmi is only supported for x86 guests.
> >  
> >  EQMP
> 
> Suggest:
> 
>     Note: inject-nmi fails when the guest doesn't support injecting.
>     Currently, only x86 guests do.
>

Patch

diff --git a/QMP/qmp-spec.txt b/QMP/qmp-spec.txt
index 1ba916c..a277896 100644
--- a/QMP/qmp-spec.txt
+++ b/QMP/qmp-spec.txt
@@ -106,14 +106,11 @@  completed because of an error condition.
 
 The format is:
 
-{ "error": { "class": json-string, "data": json-object, "desc": json-string },
-  "id": json-value }
+{ "error": { "class": json-string, "desc": json-string }, "id": json-value }
 
  Where,
 
-- The "class" member contains the error class name (eg. "ServiceUnavailable")
-- The "data" member contains specific error data and is defined in a
-  per-command basis, it will be an empty json-object if the error has no data
+- The "class" member contains the error class name (eg. "GenericError")
 - The "desc" member is a human-readable error message. Clients should
   not attempt to parse this message.
 - The "id" member contains the transaction identification associated with
@@ -173,8 +170,7 @@  S: {"return": {"enabled": true, "present": true}, "id": "example"}
 ------------------
 
 C: { "execute": }
-S: {"error": {"class": "JSONParsing", "desc": "Invalid JSON syntax", "data":
-{}}}
+S: {"error": {"class": "GenericError", "desc": "Invalid JSON syntax" } }
 
 3.5 Powerdown event
 -------------------
diff --git a/monitor.c b/monitor.c
index aa57167..3694590 100644
--- a/monitor.c
+++ b/monitor.c
@@ -353,16 +353,26 @@  static void monitor_json_emitter(Monitor *mon, const QObject *data)
     QDECREF(json);
 }
 
+static QDict *build_qmp_error_dict(const QError *err)
+{
+    QObject *obj;
+
+    obj = qobject_from_jsonf("{ 'error': { 'class': %s, 'desc': %p } }",
+                             ErrorClass_lookup[err->err_class],
+                             qerror_human(err));
+
+    return qobject_to_qdict(obj);
+}
+
 static void monitor_protocol_emitter(Monitor *mon, QObject *data)
 {
     QDict *qmp;
 
     trace_monitor_protocol_emitter(mon);
 
-    qmp = qdict_new();
-
     if (!monitor_has_error(mon)) {
         /* success response */
+        qmp = qdict_new();
         if (data) {
             qobject_incref(data);
             qdict_put_obj(qmp, "return", data);
@@ -372,9 +382,7 @@  static void monitor_protocol_emitter(Monitor *mon, QObject *data)
         }
     } else {
         /* error response */
-        qdict_put(mon->error->error, "desc", qerror_human(mon->error));
-        qdict_put(qmp, "error", mon->error->error);
-        QINCREF(mon->error->error);
+        qmp = build_qmp_error_dict(mon->error);
         QDECREF(mon->error);
         mon->error = NULL;
     }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index e3cf3c5..e520b51 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -435,8 +435,7 @@  Example:
 -> { "execute": "inject-nmi" }
 <- { "return": {} }
 
-Note: inject-nmi is only supported for x86 guest currently, it will
-      returns "Unsupported" error for non-x86 guest.
+Note: inject-nmi is only supported for x86 guests.
 
 EQMP