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

[Markus Armbruster]

Luiz Capitulino <address@hidden> 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 <address@hidden>
> ---
>  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.