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

[Kevin Wolf]

Am 26.03.2012 17:38, schrieb Anthony Liguori:
> 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.

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

So your error types will end up accumulating optional parameters for all
contexts in which they can occur?

In the long run, PermissionDenied would have an optional file name (for
raw), optional host name, port, sheepdog volume ID (for sheepdog),
optional source and destination block devices (for blkmirror), remote
host and port, local address and port (for UDP chardevs)...

I could go on forever. Does this really make sense?

Kevin