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

[Pavel Hrdina <phrdina@redhat.com>]

On 08/30/2012 04:47 PM, Luiz Capitulino wrote:
> 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);

I agree that we should use error_setg() for all GenericError, but I 
think that would be better have macros for errors which are used 
many-times . For example:

#define QERR_ERRMSG_DEVICE_HAS_NO_MEDIUM "Device '%s' has no medium"

and use error_setg(errp, QERR_ERRMSG_DEVICE_HAS_NO_MEDIUM, 
bdrv_get_device_name(bs));

I think that the consistency of error messages is important and an error 
message should be the same for every error occurrence.
>
>> +        ret = -ENOMEDIUM;
> And, usually, we should get rid of errno propagation. There are two cases here:
>
>   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.
>
>         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.
>
>> +    } else if (drv->bdrv_snapshot_create) {
>> +        ret = drv->bdrv_snapshot_create(bs, sn_info, errp);
>> +    } else if (bs->file) {
>> +        ret = bdrv_snapshot_create(bs->file, sn_info, errp);
>> +    } else {
>> +        error_set(errp, QERR_NOT_SUPPORTED);
>> +        ret = -ENOTSUP;
>> +    }
>> +
>> +    return ret;
>>   }
>>   
>>   int bdrv_snapshot_goto(BlockDriverState *bs,
>> diff --git a/block.h b/block.h
>> index 2e2be11..92e782b 100644
>> --- a/block.h
>> +++ b/block.h
>> @@ -296,7 +296,8 @@ int bdrv_can_snapshot(BlockDriverState *bs);
>>   int bdrv_is_snapshot(BlockDriverState *bs);
>>   BlockDriverState *bdrv_snapshots(void);
>>   int bdrv_snapshot_create(BlockDriverState *bs,
>> -                         QEMUSnapshotInfo *sn_info);
>> +                         QEMUSnapshotInfo *sn_info,
>> +                         Error **errp);
>>   int bdrv_snapshot_goto(BlockDriverState *bs,
>>                          const char *snapshot_id);
>>   int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
>> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
>> index 4e7c93b..cf86dae 100644
>> --- a/block/qcow2-snapshot.c
>> +++ b/block/qcow2-snapshot.c
>> @@ -25,6 +25,7 @@
>>   #include "qemu-common.h"
>>   #include "block_int.h"
>>   #include "block/qcow2.h"
>> +#include "qerror.h"
>>   
>>   typedef struct QEMU_PACKED QCowSnapshotHeader {
>>       /* header is 8 byte aligned */
>> @@ -312,7 +313,9 @@ static int find_snapshot_by_id_or_name(BlockDriverState *bs, const char *name)
>>   }
>>   
>>   /* if no id is provided, a new one is constructed */
>> -int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>> +int qcow2_snapshot_create(BlockDriverState *bs,
>> +                          QEMUSnapshotInfo *sn_info,
>> +                          Error **errp)
>>   {
>>       BDRVQcowState *s = bs->opaque;
>>       QCowSnapshot *new_snapshot_list = NULL;
>> @@ -331,6 +334,8 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>>   
>>       /* Check that the ID is unique */
>>       if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
>> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> +                  "name", "non-existing id identifier");
>>           return -EEXIST;
>>       }
>>   
>> @@ -415,6 +420,8 @@ fail:
>>       g_free(sn->name);
>>       g_free(l1_table);
>>   
>> +    error_set(errp, QERR_GENERIC_ERROR, ret);
>> +
>>       return ret;
>>   }
>>   
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index b4eb654..854bd12 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -308,7 +308,9 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
>>   int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors);
>>   
>>   /* qcow2-snapshot.c functions */
>> -int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
>> +int qcow2_snapshot_create(BlockDriverState *bs,
>> +                          QEMUSnapshotInfo *sn_info,
>> +                          Error **errp);
>>   int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
>>   int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
>>   int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
>> diff --git a/block/rbd.c b/block/rbd.c
>> index 5a0f79f..7bc42f0 100644
>> --- a/block/rbd.c
>> +++ b/block/rbd.c
>> @@ -16,6 +16,7 @@
>>   #include "qemu-common.h"
>>   #include "qemu-error.h"
>>   #include "block_int.h"
>> +#include "qerror.h"
>>   
>>   #include <rbd/librbd.h>
>>   
>> @@ -817,12 +818,15 @@ static int qemu_rbd_truncate(BlockDriverState *bs, int64_t offset)
>>   }
>>   
>>   static int qemu_rbd_snap_create(BlockDriverState *bs,
>> -                                QEMUSnapshotInfo *sn_info)
>> +                                QEMUSnapshotInfo *sn_info,
>> +                                Error **errp)
>>   {
>>       BDRVRBDState *s = bs->opaque;
>> -    int r;
>> +    int ret;
>>   
>>       if (sn_info->name[0] == '\0') {
>> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> +                  "name", "some tag identifier");
>>           return -EINVAL; /* we need a name for rbd snapshots */
>>       }
>>   
>> @@ -832,17 +836,21 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
>>        */
>>       if (sn_info->id_str[0] != '\0' &&
>>           strcmp(sn_info->id_str, sn_info->name) != 0) {
>> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> +                  "name", "id and tag to equal");
>>           return -EINVAL;
>>       }
>>   
>>       if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
>> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> +                  "name", "shorter than 127 chars");
>>           return -ERANGE;
>>       }
>>   
>> -    r = rbd_snap_create(s->image, sn_info->name);
>> -    if (r < 0) {
>> -        error_report("failed to create snap: %s", strerror(-r));
>> -        return r;
>> +    ret = rbd_snap_create(s->image, sn_info->name);
>> +    if (ret < 0) {
>> +        error_set(errp, QERR_GENERIC_ERROR, ret);
>> +        return ret;
> You should always be careful when dropping error_report(), most of the time you
> also have to change callers. In this case, bdrv_snapshot_create() is called
> by qemu-img, which should be changed to print the error from the Error object.
>
>>       }
>>   
>>       return 0;
>> diff --git a/block/sheepdog.c b/block/sheepdog.c
>> index a04ad99..fc51e04 100644
>> --- a/block/sheepdog.c
>> +++ b/block/sheepdog.c
>> @@ -17,6 +17,7 @@
>>   #include "qemu_socket.h"
>>   #include "block_int.h"
>>   #include "bitops.h"
>> +#include "qerror.h"
>>   
>>   #define SD_PROTO_VER 0x01
>>   
>> @@ -1730,7 +1731,9 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)
>>       return 0;
>>   }
>>   
>> -static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>> +static int sd_snapshot_create(BlockDriverState *bs,
>> +                              QEMUSnapshotInfo *sn_info,
>> +                              Error **errp)
>>   {
>>       BDRVSheepdogState *s = bs->opaque;
>>       int ret, fd;
>> @@ -1743,9 +1746,8 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>>               s->name, sn_info->vm_state_size, s->is_snapshot);
>>   
>>       if (s->is_snapshot) {
>> -        error_report("You can't create a snapshot of a snapshot VDI, "
>> -                     "%s (%" PRIu32 ").", s->name, s->inode.vdi_id);
>> -
>> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> +                  "name", " not VDI snapshot");
>>           return -EINVAL;
>>       }
>>   
>> @@ -1767,15 +1769,12 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>>       ret = write_object(fd, (char *)&s->inode, vid_to_vdi_oid(s->inode.vdi_id),
>>                          s->inode.nr_copies, datalen, 0, 0, s->cache_enabled);
>>       if (ret < 0) {
>> -        error_report("failed to write snapshot's inode.");
>>           goto cleanup;
>>       }
>>   
>>       ret = do_sd_create(s->name, s->inode.vdi_size, s->inode.vdi_id, &new_vid, 1,
>>                          s->addr, s->port);
>>       if (ret < 0) {
>> -        error_report("failed to create inode for snapshot. %s",
>> -                     strerror(errno));
>>           goto cleanup;
>>       }
>>   
>> @@ -1785,7 +1784,6 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>>                         s->inode.nr_copies, datalen, 0, s->cache_enabled);
>>   
>>       if (ret < 0) {
>> -        error_report("failed to read new inode info. %s", strerror(errno));
>>           goto cleanup;
>>       }
>>   
>> @@ -1795,6 +1793,9 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>>   
>>   cleanup:
>>       closesocket(fd);
>> +    if (ret < 0) {
>> +        error_set(errp, QERR_GENERIC_ERROR, ret);
>> +    }
>>       return ret;
>>   }
>>   
>> diff --git a/block_int.h b/block_int.h
>> index 4452f6f..b76c943 100644
>> --- a/block_int.h
>> +++ b/block_int.h
>> @@ -206,7 +206,8 @@ struct BlockDriver {
>>                                    const uint8_t *buf, int nb_sectors);
>>   
>>       int (*bdrv_snapshot_create)(BlockDriverState *bs,
>> -                                QEMUSnapshotInfo *sn_info);
>> +                                QEMUSnapshotInfo *sn_info,
>> +                                Error **errp);
>>       int (*bdrv_snapshot_goto)(BlockDriverState *bs,
>>                                 const char *snapshot_id);
>>       int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id);
>> diff --git a/qemu-img.c b/qemu-img.c
>> index b41e670..c798d66 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1267,7 +1267,7 @@ static int img_snapshot(int argc, char **argv)
>>           sn.date_sec = tv.tv_sec;
>>           sn.date_nsec = tv.tv_usec * 1000;
>>   
>> -        ret = bdrv_snapshot_create(bs, &sn);
>> +        ret = bdrv_snapshot_create(bs, &sn, NULL);
>>           if (ret) {
>>               error_report("Could not create snapshot '%s': %d (%s)",
>>                   snapshot_name, ret, strerror(-ret));
>> diff --git a/savevm.c b/savevm.c
>> index 0ea10c9..1b67af6 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -2165,7 +2165,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>>           if (bdrv_can_snapshot(bs1)) {
>>               /* Write VM state size only to the image that contains the state */
>>               sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
>> -            ret = bdrv_snapshot_create(bs1, sn);
>> +            ret = bdrv_snapshot_create(bs1, sn, NULL);
>>               if (ret < 0) {
>>                   monitor_printf(mon, "Error while creating snapshot on '%s'\n",
>>                                  bdrv_get_device_name(bs1));