Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Markus Armbruster]

Anthony Liguori <address@hidden> writes:

> Markus Armbruster wrote:
>> We have:
>>
>> (1) machine-readable error code
>>
>> (2) human-readable error message
>>
>> (3) machine-readable additional error data
>>
>> The old monitor prints just (3).
>>   
>
> s:(3):(2):

D'oh!

>> You propose to have QMP send (1) and (3).  This forces all clients to
>> come up with (2) themselves.  Why that's problematic is discussed
>> elsewhere in this thread.
>>   
>
> As I said else where, sending (2) along with (1) and (3) doesn't
> bother me.  It's whether (2) is generated automatically from (1) and
> (2) or whether within qemu, the caller who creates the error supplies
> that text free hand.  That's what I'm fundamentally opposed to.

Having the caller pass both (2) and (3) would be dumb indeed.

>> Imagine we already had a QMP that sent (1) and (2), encoded in JSON.
>> Then nothing could stop you to add a "data" part holding (3).
>
> If (2) is generated from (1) based on a table, then I agree with you.
> If (2) is open coded, then I disagree fundamentally because you're
> mixing in information from (3) in there.

I'm thinking and talking primarily about the protocol, and that probably
makes me too terse on implementation.

I didn't mean to suggest that for adding the data part we should add new
arguments providing the data.  That would be dumb indeed.

Instead, I'd like to start with an extremely simple error reporting
mechanism, which requires an equally simple conversion, namely from
monitor_printf("human-readable error") to something of the form
qmp_error(error_code, "human-readable error").

If we later decide we want structured data in the error object, we can
still change it to something closer to the current proposal.  From the
protocol's point of view, it's a straightforward extension.  In the
server code, it's a change, not an extension.  And that's fine.

>>   Ergo,
>> keeping things simple by sending just (1) and (2) now does not make it
>> impossible to send (3) later, hence does not make it impossible to
>> provide your important feature in a future iteration.
>>   
>
> Let me give a concrete example of where your logic falls apart.  Say
> you start out with:
>
> qemu_error_new(QERR_DEVICE_NOT_READY, "Ballooning is not enabled in
> the guest");
>
> And somewhere else you have:
>
> qemu_error_new(QERR_DEVICE_NOT_READY, "The virtio-net driver is not
> loaded in the guest");
>
> There is no way to unify these into a coherent message.  It's forever
> free form and the there will always be additional information in the
> description that requires parsing from a client.  This is obviously
> more complicated for the client.  It's also impossible to
> internationalize effectively because even if you introduce a data
> field, you aren't passing enough information to generate these
> messages.  In this case, it's a subtle case of grammatical mismatch
> although both messages are saying the exact same thing.

Again, you're making an argument for specific, machine-readable errors
(an end), not for structured error data (means).  Reasonably specific
error codes could achieve that end just as well.  QERR_DEVICE_NOT_READY
isn't sufficiently specific.

In fact, even with additional structured data, I'd expect sharing the
same error code for such different errors to be rather inconvenient for
clients.  What's easier, switching over a sufficiently specific error
code, or switching over an insufficiently specific error code, then
digging through additional data to figure out what went wrong?

I'm afraid we're running in cycles...  Could explain the somewhat
irritated tone I sense further down.

>>> An error it doesn't know about is a bug in the application.  Adding a
>>> new type of error to a monitor function is equivalent to changing it's
>>> behavior.  It should require a versioning change.
>>>     
>>
>> What do you mean by versioning change?  And what does it mean for
>> clients?
>>   
>
> We really haven't gotten into versioning/feature negotation, but in my
> mind, if we add a new error type for a function, that's the same as
> adding an additional argument.  It's a new feature that requires some
> sort of version/feature mechanism.

This topic is somewhat tangential to the business at hand, but I think
it sheds some light on our different points of view, so here goes.

If we were talking about an interface within a single program, I think
I'd agree with you.  The program's parts should always be kept fully
synchronized with respect to the interface.  Easy, because you link them
together into a single package, and the tools help enforce the interface
contract.

However, here we're talking about separate programs, client and server.
Updating client and server in lock-step may be practical in some
deployments, but certainly not in others, e.g. when they run on
different computers that are separately administered.

Now, what should a client do when it discovers the monitor command it
needs to use has grown a new error?  Refuse to use the command for fear
of having to present a sub-par error message to the user in case it
fails?

Yes, the client needs updating in such a scenario.  But since it's a
normal, expected situation, I wouldn't call it a client bug.  And
regardless how we call it, we better handle it gracefully.

>>> My basic concerns boil down to:
>>>
>>> 1) It must be possible to support localization from the very beginning
>>>     
>>
>> If you insist on supporting localization in the client right now,
>> despite costs & risks, there's nothing I can do, and nothing more for me
>> to say on my first concern (QMP overengineered for a first iteration of
>> the protocol).
>>   
>
> Your proposal makes it impossible forever so yes, it's a hard
> requirement.

If my proposal made it impossible forever, you'd be right.  But it
clearly doesn't.

>               I'm making a lot of attempts to meet you half way but if
> your argument is "I don't like this, it's too complicated, I don't
> care about localization" then I don't think we're going to get over
> that.

I wouldn't consider that a fair characterization of my argument.  I
propose to take small, simple, safe steps forward instead of big leaps,
and I'm happy to delay localization in the client some for that.  Maybe
we just have to agree to disagree here.

I appreciate your willingness to accomodate me on what I consider
shortcomings of the protocol, which is really a different topic.  I'm
sure we can work that out.