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

[Anthony Liguori <anthony@codemonkey.ws>]

On 03/26/2012 10:34 AM, Kevin Wolf wrote:
> 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.

Then extend PermissionDenied to include a filename.  Problem solved.

>>> 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.

Than add/improve the existing errors.

>>>> 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...

You can bridge this all today.  I showed you in patches how to do it.  It's not 
difficult.

>> 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.

You can add parameters to Errors in a fully compatible fashion, so just add an 
filename parameter to PermissionDenied.  Problem solved.

Regards,

Anthony Liguori