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

[Kevin Wolf <kwolf@redhat.com>]

Am 31.08.2012 08:26, schrieb Markus Armbruster:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
>> On Wed, 15 Aug 2012 09:41:43 +0200
>> Pavel Hrdina <phrdina@redhat.com> wrote:
>>
>>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>>> ---
>>>  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.

I'd rather not remove existing -errno returns if there's not good reason
why we can't provide them (or can't do so easily at least).

Kevin