Patchwork [FOR,0.12,18/18] QMP: add human-readable description to error response

login
register
mail settings
Submitter Markus Armbruster
Date Dec. 7, 2009, 8:37 p.m.
Message ID <1260218236-22143-19-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/40532/
State New
Headers show

Comments

Markus Armbruster - Dec. 7, 2009, 8:37 p.m.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 QMP/qmp-spec.txt |    5 ++++-
 monitor.c        |    1 +
 qerror.c         |   21 ++++++++++++++++-----
 qerror.h         |    2 ++
 4 files changed, 23 insertions(+), 6 deletions(-)
Luiz Capitulino - Dec. 8, 2009, 12:11 p.m.
On Mon,  7 Dec 2009 21:37:16 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> -{ "error": { "class": json-string, "data": json-value }, "id": json-value }
> +{ "error": { "class": json-string, "data": json-value, "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 "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
>    the command execution (if issued by the Client)

 As we've talked on irc, I don't agree with this change.

 Basically, adding 'desc' to the standard error message introduces all
the problems we've discussed about free-form English strings.

 I feel that QError is becoming the worst of all proposals.

 I agree with you that it's not as easy as it should be to report errors,
but as we're targeting on Clients I was convinced that we could not have the
best API internally but offer a good interface for Clients.

 Now, having 'desc' as part of the standard protocol is like not having
the best API internally and offering a bad interface for Clients.

 Not to mention that those strings can't be modified when the protocol
becomes stable and we're probably talking about dozens if not a hundred
of strings. Ok, there isn't a reason to change them often, but it's
still one more thing to maintain.

 Having said that, I would agree to have 'desc' as part of debug
information. I have patches in my tree which adds CONFIG_DEBUG_QMP,
if one enables it information about the error location will also
be part of the error message. I would agree having 'desc' there
too.
Daniel P. Berrange - Dec. 8, 2009, 12:25 p.m.
On Tue, Dec 08, 2009 at 10:11:48AM -0200, Luiz Capitulino wrote:
> On Mon,  7 Dec 2009 21:37:16 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
> 
> > -{ "error": { "class": json-string, "data": json-value }, "id": json-value }
> > +{ "error": { "class": json-string, "data": json-value, "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 "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
> >    the command execution (if issued by the Client)
> 
>  As we've talked on irc, I don't agree with this change.
> 
>  Basically, adding 'desc' to the standard error message introduces all
> the problems we've discussed about free-form English strings.
> 
>  I feel that QError is becoming the worst of all proposals.
> 
>  I agree with you that it's not as easy as it should be to report errors,
> but as we're targeting on Clients I was convinced that we could not have the
> best API internally but offer a good interface for Clients.
> 
>  Now, having 'desc' as part of the standard protocol is like not having
> the best API internally and offering a bad interface for Clients.
> 
>  Not to mention that those strings can't be modified when the protocol
> becomes stable and we're probably talking about dozens if not a hundred
> of strings. Ok, there isn't a reason to change them often, but it's
> still one more thing to maintain.

I think it is fine to declare that 'desc' strings are subject to 
arbitrary change. Even if QEMU includes this 'desc' I think in libvirt
will end up doing its own error code -> human string conversion,
simply because we need to translate the strings. So we'd only use
'desc' in any logging calls if it were present.

>  Having said that, I would agree to have 'desc' as part of debug
> information. I have patches in my tree which adds CONFIG_DEBUG_QMP,
> if one enables it information about the error location will also
> be part of the error message. I would agree having 'desc' there
> too.


Daniel
Paolo Bonzini - Dec. 8, 2009, 12:38 p.m.
On 12/08/2009 01:11 PM, Luiz Capitulino wrote:
>   Not to mention that those strings can't be modified when the protocol
> becomes stable and we're probably talking about dozens if not a hundred
> of strings.

I would say that there is _explicitly no promise_ of keeping these 
stable.  You can serve

HTTP/1.1 302 You have bad taste in music
Location: http://www.britneyspears.com/

and clients will not complain.  It's the same for QMP and desc.

(Disclaimer: I haven't read the entire series from Markus).

Paolo
Markus Armbruster - Dec. 8, 2009, 12:48 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Mon,  7 Dec 2009 21:37:16 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> -{ "error": { "class": json-string, "data": json-value }, "id": json-value }
>> +{ "error": { "class": json-string, "data": json-value, "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 "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
>>    the command execution (if issued by the Client)
>
>  As we've talked on irc, I don't agree with this change.
>
>  Basically, adding 'desc' to the standard error message introduces all
> the problems we've discussed about free-form English strings.

I disagree.  See below.

>  I feel that QError is becoming the worst of all proposals.

I'm not exactly thrilled about it either, but it could clearly be worse
in many ways, so it can't be the worst of all :)

>  I agree with you that it's not as easy as it should be to report errors,
> but as we're targeting on Clients I was convinced that we could not have the
> best API internally but offer a good interface for Clients.
>
>  Now, having 'desc' as part of the standard protocol is like not having
> the best API internally and offering a bad interface for Clients.
>
>  Not to mention that those strings can't be modified when the protocol
> becomes stable

This is incorrect.  We explicitly threaten client writers that these
strings are not to be interpreted, in qmp-spec.txt: "Clients should not
attempt to parse this message."  For me, that implies that they can
change at any time.  Maybe we could use even stronger language there.

>                and we're probably talking about dozens if not a hundred
> of strings. Ok, there isn't a reason to change them often, but it's
> still one more thing to maintain.
>
>  Having said that, I would agree to have 'desc' as part of debug
> information. I have patches in my tree which adds CONFIG_DEBUG_QMP,
> if one enables it information about the error location will also
> be part of the error message. I would agree having 'desc' there
> too.

Debugging is one use for human-readable text in the error reply.  But it
is *not* the only use.  We discussed this at length in October[*], and
again in November[**].  Any new arguments?


[*] Sub-thread starting at
http://lists.gnu.org/archive/html/qemu-devel/2009-10/msg01245.html

[**] Sub-thread starting at
http://lists.gnu.org/archive/html/qemu-devel/2009-11/msg01071.html
Markus Armbruster - Dec. 8, 2009, 12:57 p.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 12/08/2009 01:11 PM, Luiz Capitulino wrote:
>>   Not to mention that those strings can't be modified when the protocol
>> becomes stable and we're probably talking about dozens if not a hundred
>> of strings.
>
> I would say that there is _explicitly no promise_ of keeping these
> stable.  You can serve
>
> HTTP/1.1 302 You have bad taste in music
> Location: http://www.britneyspears.com/
>
> and clients will not complain.

And even serve later in the same session

HTTP/1.1 302 Bad taste in music you have
Location: http://www.britneyspears.com/

>                                 It's the same for QMP and desc.

Precisely.

> (Disclaimer: I haven't read the entire series from Markus).
Anthony Liguori - Dec. 8, 2009, 1:18 p.m.
Luiz Capitulino wrote:
> On Mon,  7 Dec 2009 21:37:16 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>   
>> -{ "error": { "class": json-string, "data": json-value }, "id": json-value }
>> +{ "error": { "class": json-string, "data": json-value, "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 "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
>>    the command execution (if issued by the Client)
>>     
>
>  As we've talked on irc, I don't agree with this change.
>
>  Basically, adding 'desc' to the standard error message introduces all
> the problems we've discussed about free-form English strings.
>   

It's not free form English.  The 'desc' string is always autogenerated 
based on the error object.

It's completely redundant information because you can already generate 
that string, but it simplifies client creation because a lazy client 
does not have to include the conversion table if they only care about 
English error output.

Regards,

Anthony Liguori
Luiz Capitulino - Dec. 8, 2009, 2:11 p.m.
On Tue, 8 Dec 2009 12:25:13 +0000
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Tue, Dec 08, 2009 at 10:11:48AM -0200, Luiz Capitulino wrote:
> > On Mon,  7 Dec 2009 21:37:16 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> > 
> > > -{ "error": { "class": json-string, "data": json-value }, "id": json-value }
> > > +{ "error": { "class": json-string, "data": json-value, "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 "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
> > >    the command execution (if issued by the Client)
> > 
> >  As we've talked on irc, I don't agree with this change.
> > 
> >  Basically, adding 'desc' to the standard error message introduces all
> > the problems we've discussed about free-form English strings.
> > 
> >  I feel that QError is becoming the worst of all proposals.
> > 
> >  I agree with you that it's not as easy as it should be to report errors,
> > but as we're targeting on Clients I was convinced that we could not have the
> > best API internally but offer a good interface for Clients.
> > 
> >  Now, having 'desc' as part of the standard protocol is like not having
> > the best API internally and offering a bad interface for Clients.
> > 
> >  Not to mention that those strings can't be modified when the protocol
> > becomes stable and we're probably talking about dozens if not a hundred
> > of strings. Ok, there isn't a reason to change them often, but it's
> > still one more thing to maintain.
> 
> I think it is fine to declare that 'desc' strings are subject to 
> arbitrary change. Even if QEMU includes this 'desc' I think in libvirt
> will end up doing its own error code -> human string conversion,
> simply because we need to translate the strings. So we'd only use
> 'desc' in any logging calls if it were present.

 That's what we would expect from clients, but I'm not convinced that
they'll took the easy way and start using it the way they shouldn't.
Luiz Capitulino - Dec. 8, 2009, 2:41 p.m.
On Tue, 08 Dec 2009 07:18:11 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> Luiz Capitulino wrote:
> > On Mon,  7 Dec 2009 21:37:16 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >   
> >> -{ "error": { "class": json-string, "data": json-value }, "id": json-value }
> >> +{ "error": { "class": json-string, "data": json-value, "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 "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
> >>    the command execution (if issued by the Client)
> >>     
> >
> >  As we've talked on irc, I don't agree with this change.
> >
> >  Basically, adding 'desc' to the standard error message introduces all
> > the problems we've discussed about free-form English strings.
> >   
> 
> It's not free form English.  The 'desc' string is always autogenerated 
> based on the error object.
> 
> It's completely redundant information because you can already generate 
> that string, but it simplifies client creation because a lazy client 
> does not have to include the conversion table if they only care about 
> English error output.

 I wonder if we could have a simpler design for the internal API if
we knew in advance that 'desc' would be part of the standard
error message, eg. error code based.

 But, as Markus said it's not the 'worst' as I've put it, let's
move forward then.
Markus Armbruster - Dec. 8, 2009, 2:50 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Tue, 8 Dec 2009 12:25:13 +0000
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
>
>> On Tue, Dec 08, 2009 at 10:11:48AM -0200, Luiz Capitulino wrote:
>> > On Mon,  7 Dec 2009 21:37:16 +0100
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> > 
>> > > -{ "error": { "class": json-string, "data": json-value }, "id": json-value }
>> > > +{ "error": { "class": json-string, "data": json-value, "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 "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
>> > >    the command execution (if issued by the Client)
>> > 
>> >  As we've talked on irc, I don't agree with this change.
>> > 
>> >  Basically, adding 'desc' to the standard error message introduces all
>> > the problems we've discussed about free-form English strings.
>> > 
>> >  I feel that QError is becoming the worst of all proposals.
>> > 
>> >  I agree with you that it's not as easy as it should be to report errors,
>> > but as we're targeting on Clients I was convinced that we could not have the
>> > best API internally but offer a good interface for Clients.
>> > 
>> >  Now, having 'desc' as part of the standard protocol is like not having
>> > the best API internally and offering a bad interface for Clients.
>> > 
>> >  Not to mention that those strings can't be modified when the protocol
>> > becomes stable and we're probably talking about dozens if not a hundred
>> > of strings. Ok, there isn't a reason to change them often, but it's
>> > still one more thing to maintain.
>> 
>> I think it is fine to declare that 'desc' strings are subject to 
>> arbitrary change. Even if QEMU includes this 'desc' I think in libvirt
>> will end up doing its own error code -> human string conversion,
>> simply because we need to translate the strings. So we'd only use
>> 'desc' in any logging calls if it were present.
>
>  That's what we would expect from clients, but I'm not convinced that
> they'll took the easy way and start using it the way they shouldn't.

Change a couple of common messages in every release, and they'll stop
doing that real fast ;)

Patch

diff --git a/QMP/qmp-spec.txt b/QMP/qmp-spec.txt
index 8429789..1cbd21c 100644
--- a/QMP/qmp-spec.txt
+++ b/QMP/qmp-spec.txt
@@ -102,13 +102,16 @@  completed because of an error condition.
 
 The format is:
 
-{ "error": { "class": json-string, "data": json-value }, "id": json-value }
+{ "error": { "class": json-string, "data": json-value, "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 "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
   the command execution (if issued by the Client)
 
diff --git a/monitor.c b/monitor.c
index 0bcffbe..4ad1b5e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -305,6 +305,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);
         QDECREF(mon->error);
diff --git a/qerror.c b/qerror.c
index 8ffe4f6..5f8fc5d 100644
--- a/qerror.c
+++ b/qerror.c
@@ -283,13 +283,11 @@  static const char *append_field(QString *outstr, const QError *qerror,
 }
 
 /**
- * qerror_print(): Print QError data
+ * qerror_human(): Format QError data into human-readable string.
  *
- * This function will print the member 'desc' of the specified QError object,
- * it uses qemu_error() for this, so that the output is routed to the right
- * place (ie. stderr or Monitor's device).
+ * Formats according to member 'desc' of the specified QError object.
  */
-void qerror_print(const QError *qerror)
+QString *qerror_human(const QError *qerror)
 {
     const char *p;
     QString *qstring;
@@ -309,6 +307,19 @@  void qerror_print(const QError *qerror)
         }
     }
 
+    return qstring;
+}
+
+/**
+ * qerror_print(): Print QError data
+ *
+ * This function will print the member 'desc' of the specified QError object,
+ * it uses qemu_error() for this, so that the output is routed to the right
+ * place (ie. stderr or Monitor's device).
+ */
+void qerror_print(const QError *qerror)
+{
+    QString *qstring = qerror_human(qerror);
     qemu_error("%s\n", qstring_get_str(qstring));
     QDECREF(qstring);
 }
diff --git a/qerror.h b/qerror.h
index 9462d5c..09e32b9 100644
--- a/qerror.h
+++ b/qerror.h
@@ -13,6 +13,7 @@ 
 #define QERROR_H
 
 #include "qdict.h"
+#include "qstring.h"
 #include <stdarg.h>
 
 typedef struct QErrorStringTable {
@@ -32,6 +33,7 @@  typedef struct QError {
 QError *qerror_new(void);
 QError *qerror_from_info(const char *file, int linenr, const char *func,
                          const char *fmt, va_list *va);
+QString *qerror_human(const QError *qerror);
 void qerror_print(const QError *qerror);
 QError *qobject_to_qerror(const QObject *obj);