Patchwork [30/34] qemu-ga: 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-31-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/174671/
State New
Headers show

Comments

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

Instead of returning something like:

{ "error": { "class": "InvalidParameterValue",
             "data": {"name": "mode", "expected": "halt|powerdown|reboot" } } }

qemu-ga now returns:

 { "error": { "class": "GenericError",
              "desc": "Parameter 'mode' expects halt|powerdown|reboot" } }

Notice that this is also a bug fix, as qemu-ga wasn't returning the
human message.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 Makefile.objs       |  2 +-
 qapi/qmp-core.h     |  1 +
 qapi/qmp-dispatch.c | 10 +++++++++-
 qemu-ga.c           |  4 ++--
 4 files changed, 13 insertions(+), 4 deletions(-)
Michael Roth - Aug. 3, 2012, 5:44 p.m.
On Wed, Aug 01, 2012 at 10:02:50PM -0300, Luiz Capitulino wrote:
> IMPORTANT: this BREAKS qemu-ga compatibility for the error response.
> 
> Instead of returning something like:
> 
> { "error": { "class": "InvalidParameterValue",
>              "data": {"name": "mode", "expected": "halt|powerdown|reboot" } } }
> 
> qemu-ga now returns:
> 
>  { "error": { "class": "GenericError",
>               "desc": "Parameter 'mode' expects halt|powerdown|reboot" } }

Specific error responses weren't part of the documented API, so I think
anything reliant on those is making invalid assumptions; there
should always be a catch-all for unknown/unexpected error
messages/error payloads.

> 
> Notice that this is also a bug fix, as qemu-ga wasn't returning the
> human message.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

So, if the libvirt folks are okay with it:

Acked-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  Makefile.objs       |  2 +-
>  qapi/qmp-core.h     |  1 +
>  qapi/qmp-dispatch.c | 10 +++++++++-
>  qemu-ga.c           |  4 ++--
>  4 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 5ebbcfa..cbfbba5 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -220,7 +220,7 @@ universal-obj-y += $(qapi-obj-y)
>  ######################################################################
>  # guest agent
> 
> -qga-obj-y = qga/ qemu-ga.o module.o
> +qga-obj-y = qga/ qemu-ga.o module.o qapi-types.o qapi-visit.o
>  qga-obj-$(CONFIG_WIN32) += oslib-win32.o
>  qga-obj-$(CONFIG_POSIX) += oslib-posix.o qemu-sockets.o qemu-option.o
> 
> diff --git a/qapi/qmp-core.h b/qapi/qmp-core.h
> index b0f64ba..00446cf 100644
> --- a/qapi/qmp-core.h
> +++ b/qapi/qmp-core.h
> @@ -49,6 +49,7 @@ void qmp_disable_command(const char *name);
>  void qmp_enable_command(const char *name);
>  bool qmp_command_is_enabled(const char *name);
>  char **qmp_get_command_list(void);
> +QObject *qmp_build_error_object(Error *errp);
> 
>  #endif
> 
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index 122c1a2..ec613f8 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -14,6 +14,7 @@
>  #include "qemu-objects.h"
>  #include "qapi/qmp-core.h"
>  #include "json-parser.h"
> +#include "qapi-types.h"
>  #include "error.h"
>  #include "error_int.h"
>  #include "qerror.h"
> @@ -109,6 +110,13 @@ static QObject *do_qmp_dispatch(QObject *request, Error **errp)
>      return ret;
>  }
> 
> +QObject *qmp_build_error_object(Error *errp)
> +{
> +    return qobject_from_jsonf("{ 'class': %s, 'desc': %s }",
> +                              ErrorClass_lookup[error_get_class(errp)],
> +                              error_get_pretty(errp));
> +}
> +
>  QObject *qmp_dispatch(QObject *request)
>  {
>      Error *err = NULL;
> @@ -119,7 +127,7 @@ QObject *qmp_dispatch(QObject *request)
> 
>      rsp = qdict_new();
>      if (err) {
> -        qdict_put_obj(rsp, "error", error_get_qobject(err));
> +        qdict_put_obj(rsp, "error", qmp_build_error_object(err));
>          error_free(err);
>      } else if (ret) {
>          qdict_put_obj(rsp, "return", ret);
> diff --git a/qemu-ga.c b/qemu-ga.c
> index 8199da7..f45bc61 100644
> --- a/qemu-ga.c
> +++ b/qemu-ga.c
> @@ -515,7 +515,7 @@ static void process_event(JSONMessageParser *parser, QList *tokens)
>          } else {
>              g_warning("failed to parse event: %s", error_get_pretty(err));
>          }
> -        qdict_put_obj(qdict, "error", error_get_qobject(err));
> +        qdict_put_obj(qdict, "error", qmp_build_error_object(err));
>          error_free(err);
>      } else {
>          qdict = qobject_to_qdict(obj);
> @@ -532,7 +532,7 @@ static void process_event(JSONMessageParser *parser, QList *tokens)
>              qdict = qdict_new();
>              g_warning("unrecognized payload format");
>              error_set(&err, QERR_UNSUPPORTED);
> -            qdict_put_obj(qdict, "error", error_get_qobject(err));
> +            qdict_put_obj(qdict, "error", qmp_build_error_object(err));
>              error_free(err);
>          }
>          ret = send_response(s, QOBJECT(qdict));
> -- 
> 1.7.11.2.249.g31c7954.dirty
>
Eric Blake - Aug. 3, 2012, 5:56 p.m.
On 08/03/2012 11:44 AM, Michael Roth wrote:
> On Wed, Aug 01, 2012 at 10:02:50PM -0300, Luiz Capitulino wrote:
>> IMPORTANT: this BREAKS qemu-ga compatibility for the error response.
>>
>> Instead of returning something like:
>>
>> { "error": { "class": "InvalidParameterValue",
>>              "data": {"name": "mode", "expected": "halt|powerdown|reboot" } } }
>>
>> qemu-ga now returns:
>>
>>  { "error": { "class": "GenericError",
>>               "desc": "Parameter 'mode' expects halt|powerdown|reboot" } }
> 

>>
>> Notice that this is also a bug fix, as qemu-ga wasn't returning the
>> human message.
>>
>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> 
> So, if the libvirt folks are okay with it:

The only use libvirt made of existing qemu-ga errors was to stringify
them in order to pass on an error message to the user when a command
failed.  Existing libvirt attempts to look up the 'desc' field, and when
it is lacking, then attempts to stringify the 'class' field based on a
finite list of known classes.  Qemu is now shrinking the list of known
classes but providing a 'desc' field, so the error message quality in
libvirt will actually improve.  After reading libvirt's
src/qemu/qemu_agent.c, I don't see any problem with this patch from
libvirt's point of view.

Reviewed-by: Eric Blake <eblake@redhat.com>
Luiz Capitulino - Aug. 3, 2012, 6:02 p.m.
On Fri, 03 Aug 2012 11:56:29 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 08/03/2012 11:44 AM, Michael Roth wrote:
> > On Wed, Aug 01, 2012 at 10:02:50PM -0300, Luiz Capitulino wrote:
> >> IMPORTANT: this BREAKS qemu-ga compatibility for the error response.
> >>
> >> Instead of returning something like:
> >>
> >> { "error": { "class": "InvalidParameterValue",
> >>              "data": {"name": "mode", "expected": "halt|powerdown|reboot" } } }
> >>
> >> qemu-ga now returns:
> >>
> >>  { "error": { "class": "GenericError",
> >>               "desc": "Parameter 'mode' expects halt|powerdown|reboot" } }
> > 
> 
> >>
> >> Notice that this is also a bug fix, as qemu-ga wasn't returning the
> >> human message.
> >>
> >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > 
> > So, if the libvirt folks are okay with it:
> 
> The only use libvirt made of existing qemu-ga errors was to stringify
> them in order to pass on an error message to the user when a command
> failed.  Existing libvirt attempts to look up the 'desc' field, and when
> it is lacking, then attempts to stringify the 'class' field based on a
> finite list of known classes.  Qemu is now shrinking the list of known
> classes but providing a 'desc' field, so the error message quality in
> libvirt will actually improve.  After reading libvirt's
> src/qemu/qemu_agent.c, I don't see any problem with this patch from
> libvirt's point of view.

Yeah, I actually have a request from Michal to do just that (add 'desc'
to qemu-ga's errors).

> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
>

Patch

diff --git a/Makefile.objs b/Makefile.objs
index 5ebbcfa..cbfbba5 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -220,7 +220,7 @@  universal-obj-y += $(qapi-obj-y)
 ######################################################################
 # guest agent
 
-qga-obj-y = qga/ qemu-ga.o module.o
+qga-obj-y = qga/ qemu-ga.o module.o qapi-types.o qapi-visit.o
 qga-obj-$(CONFIG_WIN32) += oslib-win32.o
 qga-obj-$(CONFIG_POSIX) += oslib-posix.o qemu-sockets.o qemu-option.o
 
diff --git a/qapi/qmp-core.h b/qapi/qmp-core.h
index b0f64ba..00446cf 100644
--- a/qapi/qmp-core.h
+++ b/qapi/qmp-core.h
@@ -49,6 +49,7 @@  void qmp_disable_command(const char *name);
 void qmp_enable_command(const char *name);
 bool qmp_command_is_enabled(const char *name);
 char **qmp_get_command_list(void);
+QObject *qmp_build_error_object(Error *errp);
 
 #endif
 
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 122c1a2..ec613f8 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -14,6 +14,7 @@ 
 #include "qemu-objects.h"
 #include "qapi/qmp-core.h"
 #include "json-parser.h"
+#include "qapi-types.h"
 #include "error.h"
 #include "error_int.h"
 #include "qerror.h"
@@ -109,6 +110,13 @@  static QObject *do_qmp_dispatch(QObject *request, Error **errp)
     return ret;
 }
 
+QObject *qmp_build_error_object(Error *errp)
+{
+    return qobject_from_jsonf("{ 'class': %s, 'desc': %s }",
+                              ErrorClass_lookup[error_get_class(errp)],
+                              error_get_pretty(errp));
+}
+
 QObject *qmp_dispatch(QObject *request)
 {
     Error *err = NULL;
@@ -119,7 +127,7 @@  QObject *qmp_dispatch(QObject *request)
 
     rsp = qdict_new();
     if (err) {
-        qdict_put_obj(rsp, "error", error_get_qobject(err));
+        qdict_put_obj(rsp, "error", qmp_build_error_object(err));
         error_free(err);
     } else if (ret) {
         qdict_put_obj(rsp, "return", ret);
diff --git a/qemu-ga.c b/qemu-ga.c
index 8199da7..f45bc61 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -515,7 +515,7 @@  static void process_event(JSONMessageParser *parser, QList *tokens)
         } else {
             g_warning("failed to parse event: %s", error_get_pretty(err));
         }
-        qdict_put_obj(qdict, "error", error_get_qobject(err));
+        qdict_put_obj(qdict, "error", qmp_build_error_object(err));
         error_free(err);
     } else {
         qdict = qobject_to_qdict(obj);
@@ -532,7 +532,7 @@  static void process_event(JSONMessageParser *parser, QList *tokens)
             qdict = qdict_new();
             g_warning("unrecognized payload format");
             error_set(&err, QERR_UNSUPPORTED);
-            qdict_put_obj(qdict, "error", error_get_qobject(err));
+            qdict_put_obj(qdict, "error", qmp_build_error_object(err));
             error_free(err);
         }
         ret = send_response(s, QOBJECT(qdict));