Re: [Qemu-devel] Ignoring errno makes QMP errors suck

[Kevin Wolf <kwolf@redhat.com>]

Am 26.03.2012 15:28, schrieb Luiz Capitulino:
> On Mon, 26 Mar 2012 15:13:36 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
>> Am 26.03.2012 14:46, schrieb Luiz Capitulino:
>>> On Mon, 26 Mar 2012 10:39:50 +0200
>>> Kevin Wolf <kwolf@redhat.com> wrote:
>>>
>>>> Hi,
>>>>
>>>> I keep getting reports of problems, with nice error descriptions that
>>>> usually look very similar to what I produced here:
>>>>
>>>> {"execute":"blockdev-snapshot-sync","arguments":{"device":"ide0-hd0","snapshot-file":"/tmp/backing.qcow2"}}
>>>> {"error": {"class": "OpenFileFailed", "desc": "Could not open
>>>> '/tmp/backing.qcow2'", "data": {"filename": "/tmp/backing.qcow2"}}}
>>>>
>>>> Who can tell me what has happened here? Oh, yes, the command failed, I
>>>> would have guessed that from the "error" key. But the actual error
>>>> description is as useless as it gets. It doesn't tell me anything about
>>>> _why_ the snapshot couldn't be created. ("Permission denied" would have
>>>> been the helpful additional information in this case)
>>>
>>> There's a function called qemu_fopen_err() in the screendump conversion series
>>> that return more specific errors. It will be trivial to add qemu_open_err()
>>> as soon as qemu_fopen_err() is merged.
>>>
>>> We're adding a bunch of more precise errors (some map directly to errno). That's
>>> the easy part. The hard part is to convert everything to use them.
>>>
>>> Note that while it's true that this shouldn't have leaked to QMP, good error
>>> reporting is a general problem in QEMU.
>>
>> I guess my point is that we're actually moving backwards here. In HMP
>> things like this do print the right error message (using error_report).
>> And the return code is passed all the way down to where the QMP error is
>> generated, it's just ignored there.
>>
>> The last time I checked there was no easy way to handle it there because
>> errno and strerror(-errno) aren't things allowed in QMP messages. This
>> is the problem for which I wanted to get some attention.
> 
> What we're doing now is to add QErrors that map to errno. So, for example,
> we have PermissionDenied for EPERM.

EACCES in this case, but yes.

> I think this is exactly the same thing, except:
> 
>  1. We don't use the errno number, because this may differ among OSs
>  2. We don't use the sterrror() string to allow for internationalization,
>     but we have our own string that should have the same end result

Yes, I know the reasons why we can't use these options and they are good
reasons. If we get something to replace it (so that we can have a
errno_to_qmp()), that part should be solved.

>> Does the patch that you mentioned add a generic way for adding an
>> (converted) errno to QMP errors? Or does it split up existing errors
>> into more and finer grained errors?
> 
> The latter. The QMP errors have to be added manually. But it's just a matter
> of time to get the most used errnos added.

Your PermissionDenied example doesn't really do this. It is a generic
error message that may occur in multiple contexts, right? So in one
context you may need a file name as additional information, in another
context permission for something completely different may be missing
(especially if you include EPERM in the same error).

I think 'OpenFileFailed' is not too bad, it's just missing a field that
gives the detail 'PermissionDenied'. I'm not sure if having
'PermissionDenied' as the top-level error object is the best idea.

Kevin