Re: [Qemu-devel] Re: [FOR 0.12 PATCH 18/18] QMP: add human-readable description to error response
On Tue, 8 Dec 2009 12:25:13 +
"Daniel P. Berrange" 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 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.
Re: [Qemu-devel] Re: [FOR 0.12 PATCH 18/18] QMP: add human-readable description to error response
Luiz Capitulino writes:
> On Tue, 8 Dec 2009 12:25:13 +
> "Daniel P. Berrange" 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 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 ;)
Re: [Qemu-devel] Re: [FOR 0.12 PATCH 18/18] QMP: add human-readable description to error response
On Tue, 08 Dec 2009 07:18:11 -0600
Anthony Liguori wrote:
> Luiz Capitulino wrote:
> > On Mon, 7 Dec 2009 21:37:16 +0100
> > Markus Armbruster 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.
Re: [Qemu-devel] Re: [FOR 0.12 PATCH 18/18] QMP: add human-readable description to error response
Luiz Capitulino wrote:
On Mon, 7 Dec 2009 21:37:16 +0100
Markus Armbruster 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
[Qemu-devel] Re: [FOR 0.12 PATCH 18/18] QMP: add human-readable description to error response
Paolo Bonzini 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).
[Qemu-devel] Re: [FOR 0.12 PATCH 18/18] QMP: add human-readable description to error response
Luiz Capitulino writes:
> On Mon, 7 Dec 2009 21:37:16 +0100
> Markus Armbruster 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
[Qemu-devel] Re: [FOR 0.12 PATCH 18/18] QMP: add human-readable description to error response
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
Re: [Qemu-devel] Re: [FOR 0.12 PATCH 18/18] QMP: add human-readable description to error response
On Tue, Dec 08, 2009 at 10:11:48AM -0200, Luiz Capitulino wrote:
> On Mon, 7 Dec 2009 21:37:16 +0100
> Markus Armbruster 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
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
[Qemu-devel] Re: [FOR 0.12 PATCH 18/18] QMP: add human-readable description to error response
On Mon, 7 Dec 2009 21:37:16 +0100
Markus Armbruster 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.
