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

[Kevin Wolf]

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

> But it would be even better to push Error ** into bdrv_img_create().  There's 
> only two callers so it would be trivial to do that.  Then you could do:
> 
> diff --git a/block.c b/block.c
> index b88ee90..a7bf8a9 100644
> --- a/block.c
> +++ b/block.c
> @@ -3881,7 +3881,8 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie 
> *cook
> 
>   int bdrv_img_create(const char *filename, const char *fmt,
>                       const char *base_filename, const char *base_fmt,
> -                    char *options, uint64_t img_size, int flags)
> +                    char *options, uint64_t img_size, int flags,
> +                    Error **errp)
>   {
>       QEMUOptionParameter *param = NULL, *create_options = NULL;
>       QEMUOptionParameter *backing_fmt, *backing_file, *size;
> @@ -3893,14 +3894,14 @@ int bdrv_img_create(const char *filename, const char 
> *fm
>       /* Find driver and parse its options */
>       drv = bdrv_find_format(fmt);
>       if (!drv) {
> -        error_report("Unknown file format '%s'", fmt);
> +        error_set(errp, QERR_INVALID_BLOCK_FORMAT, fmt);
>           ret = -EINVAL;
>           goto out;
>       }
> 
> Etc.

Yes, but that's a completely independent problem.

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

Kevin