| Message ID | 1260218236-22143-19-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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
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
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).
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
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.
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.
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 ;)
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);
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(-)