Re: [Qemu-devel] [PATCH 02/18] block: add error parameter to bdrv_snapshot_create() and related functions

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Wed, 15 Aug 2012 09:41:43 +0200
> Pavel Hrdina <address@hidden> wrote:
>
>> Signed-off-by: Pavel Hrdina <address@hidden>
>> ---
>>  block.c                | 25 +++++++++++++++++--------
>>  block.h                |  3 ++-
>>  block/qcow2-snapshot.c |  9 ++++++++-
>>  block/qcow2.h          |  4 +++-
>>  block/rbd.c            | 20 ++++++++++++++------
>>  block/sheepdog.c       | 17 +++++++++--------
>>  block_int.h            |  3 ++-
>>  qemu-img.c             |  2 +-
>>  savevm.c               |  2 +-
>>  9 files changed, 57 insertions(+), 28 deletions(-)
>> 
>> diff --git a/block.c b/block.c
>> index 016858b..8bc49b7 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2661,16 +2661,25 @@ BlockDriverState *bdrv_snapshots(void)
>>  }
>>  
>>  int bdrv_snapshot_create(BlockDriverState *bs,
>> -                         QEMUSnapshotInfo *sn_info)
>> +                         QEMUSnapshotInfo *sn_info,
>> +                         Error **errp)
>>  {
>>      BlockDriver *drv = bs->drv;
>> -    if (!drv)
>> -        return -ENOMEDIUM;
>> -    if (drv->bdrv_snapshot_create)
>> -        return drv->bdrv_snapshot_create(bs, sn_info);
>> -    if (bs->file)
>> -        return bdrv_snapshot_create(bs->file, sn_info);
>> -    return -ENOTSUP;
>> +    int ret;
>> +
>> +    if (!drv) {
>> +        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, 
>> bdrv_get_device_name(bs));
>
> We should only use QERR_ macros for the errors listed in the ErrorClass enum
> (except GenericError), all other errors should generally use error_setg(), 
> like
> this:
>
>  error_setg(errp, "device '%s' has no medium);
>
>> +        ret = -ENOMEDIUM;
>
> And, usually, we should get rid of errno propagation. There are two cases 
> here:

The block layer consistently[*] uses -errno return values.  Its
consistency is valuable, and I'm a bit reluctant to break it.  Maybe a
new rule "returns -errno, except when it has an Error ** argument" could
work.  I'd like to hear Kevin's advice on this.

>
>  1. errno is propagated up so that upper layers can print a decent error
>     message to the user.
>
>     In this case, it's safe to eliminate errno. error_setg() will store a
>     decent message already and the Error object can be propagated up.
>
>  2. errno is propagated up so that upper layers can distinguish among
>     error causes and take different actions accordingly.
>
>     Doesn't seem to be the case of bdrv_snapshot_create() (ie. errno is only
>     used to communicate the error to the user). However, I'm pretty sure that
>     such usage exists in qemu and the error API will break it, as most of our
>     errors are generic.
>
>     I see two solutions to this problem:
>
>        A. Add specific errors to ErrorClass. I don't like this very much,
>           as it's possible that such errors are going to be useful only 
> internally.

Let's not reinvent errno, poorly.

>        B. Add two new functions:
>
>             void error_sete(Error **err, ErrorClass err_class, int errno, 
> const char *fmt, ...);
>             int error_get_errno(const Error **err);
>
>          So that we can maintain errno when it's used to communicate
>          error cause among functions.

Better.

What's ErrorClass doing there?

[...]


[*] Almost; it's still QEMU after all.