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

[Kevin Wolf]

Am 26.03.2012 17:14, schrieb Anthony Liguori:
> On 03/26/2012 10:08 AM, Kevin Wolf wrote:
>> Am 26.03.2012 15:37, schrieb Anthony Liguori:
>>> On 03/26/2012 03:39 AM, Kevin Wolf 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"}}}
>>>
>>> This is not QMP's fault.  This is the block layers.  Specifically, you're 
>>> missing:
>>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index 1a500b8..04c3a39 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -777,7 +777,11 @@ void qmp_transaction(BlockdevActionList *dev_list, 
>>> Error **
>>>                                      states->old_bs->drv->format_name,
>>>                                      NULL, -1, flags);
>>>                if (ret) {
>>> -                error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
>>> +                if (ret == -EPERM) {
>>> +                    error_set(errp, QERR_PERMISSION_DENIED);
>>> +                } else {
>>> +                    error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
>>> +                }
>>>                    goto delete_and_fail;
>>>                }
>>>            }
>>>
>>> Which is handling:
>>>
>>>               ret = bdrv_img_create(new_image_file, format,
>>>                                     states->old_bs->filename,
>>>                                     states->old_bs->drv->format_name,
>>>                                     NULL, -1, flags);
>>
>> It really should be something like this:
>>
>> -    error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
>> +    error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file, -ret);
>>
>> And QERR_OPEN_FILE_FAILED would contain a conversion specifier for
>> errnos in qobject_from_jsonv().
> 
> No, it really shouldn't be.
> 
> Errors are verbs, not knows, you're treating the error as a noun "the 
> operation 
> open file" and looking to use errno as the verb.  This is wrong.  The noun is 
> implied in the operation.
> 
> You could use error_set_from_errno(errp, -ret) which doesn't exist, but 
> could. 
> But errno on it's own lacks a lot of useful information so I wouldn't suggest 
> always using such a function.

I couldn't care less about nouns and verbs and stuff.

I want to transfer the information that a "permission denied" error has
happened and on which file it has happened. The existing OpenFileFailed
error doesn't allow to specify that the missing permission was the
problem, and a hypothetical PermissionDenied error wouldn't allow me to
specify the file name because it would be too generic.

This is my problem, and nothing else.

>> Yes, but that's a completely independent problem.
> 
> It's not really.  If you want high quality errors, you have to push the error 
> handling up the stack.  That's the reason we have Error--to introduce a 
> common 
> error handling framework capable of generating high quality error information.

Yes, but if there is no appropriate error, then even if I added Error
support to the Linux syscalls they couldn't generate the right error
message. This is why I still think it's completely independent.

>>>> 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)
>>>>
>>>> How should management tools ever be able to provide a helpful error
>>>> message to their users if all they get is this useless "something went
>>>> wrong" error?
>>>
>>> You need to kill off error_report in the block layer and replace it with
>>> error_set.  The problem with error_report is that while you can understand 
>>> what
>>> "Unknown file format 'qcow2'" means, management tools can't.  Responding 
>>> that
>>> "the tool can just present that error to the user" implies that the 
>>> management
>>> tool only provides an English-language interface which is not terribly 
>>> friendly.
>>>
>>> QMP provides all the infrastructure you need.   You just have to use it.
>>
>> It doesn't provide the portable way of reporting errno yet.
> 
> I think what you'll find is that 90% of the time, the errno is being 
> generated 
> somewhere within QEMU code or that there's a system call that returns on one 
> errno that we care about.  If you push error handling down to the source of 
> the 
> error, I'm sure you'll find that you almost never have to switch on errno.

I'm looking for a solution that works now and not only in five years
when all of qemu has been rewritten. I'm also not quite sure if we
really want to drag Errors through coroutines and AIO code in the block
layer...

> Having an error_set_from_errno() would be a stop-gap to help bridge 
> unconverted 
> code, but if you want high quality errors, the right answer is to convert the 
> existing code to use the Error infrastructure properly.

Only if it can be used properly. That is, if I can somehow create an
error message that contains _both_ the file name and the error cause.

>> I could add
>> tests for specific errors (like you suggested above) in every single
>> place that sets an error, but I'd rather not. It would make the code
>> verbose and the error reporting probably inconsistent, if not buggy.
> 
> We have a lot of:
> 
> error_report("Some english string\n");
> return -ERANDOMERRORCODE;
> 
> This idiom does not make for good on the wire errors.  You can replace these 
> lines with a single error_set() call.  There's no need for switching.

But this is not the case I have asked for.

Kevin