Re: [Qemu-devel] [PATCH 29/34] qmp: switch to the new error format on the wire

[Luiz Capitulino <lcapitulino@redhat.com>]

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