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

[Luiz Capitulino <lcapitulino@redhat.com>]

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:

 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));