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

[Luiz Capitulino]

On Wed, 15 Aug 2012 09:41:44 +0200
Pavel Hrdina <address@hidden> wrote:

> Signed-off-by: Pavel Hrdina <address@hidden>
> ---
>  block.c                | 26 +++++++++++++++-----------
>  block.h                |  3 ++-
>  block/qcow2-snapshot.c | 11 ++++++++---
>  block/qcow2.h          |  4 +++-
>  block/rbd.c            |  6 +++++-
>  block/sheepdog.c       | 16 +++++++++-------
>  block_int.h            |  3 ++-
>  qemu-img.c             |  2 +-
>  savevm.c               |  2 +-
>  9 files changed, 46 insertions(+), 27 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 8bc49b7..ad25184 100644
> --- a/block.c
> +++ b/block.c
> @@ -2683,29 +2683,33 @@ int bdrv_snapshot_create(BlockDriverState *bs,
>  }
>  
>  int bdrv_snapshot_goto(BlockDriverState *bs,
> -                       const char *snapshot_id)
> +                       const char *snapshot_id,
> +                       Error **errp)
>  {
>      BlockDriver *drv = bs->drv;
>      int ret, open_ret;
>  
> -    if (!drv)
> -        return -ENOMEDIUM;
> -    if (drv->bdrv_snapshot_goto)
> -        return drv->bdrv_snapshot_goto(bs, snapshot_id);
> -
> -    if (bs->file) {
> +    if (!drv) {
> +        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
> +        ret = -ENOMEDIUM;

Most of the comments I made in 02/18 apply for this patch (and probably
the next ones too), so to summarize:

 1. The QERR_ macros are deprecated, we should use error_setg() instead

 2. As a general rule, only the Error object should be propagated up (ie.
    we shouldn't propagate the Error object _and_ errno). It's debatable if
    we can get rid of errno, if we can't then we'll have to extend the error API
    to embed errno.

    We have to discuss this with block layer guys, preferably before doing
    large series.

One more comment below.

> +    } else if (drv->bdrv_snapshot_goto) {
> +        ret = drv->bdrv_snapshot_goto(bs, snapshot_id, errp);
> +    } else if (bs->file) {
>          drv->bdrv_close(bs);
> -        ret = bdrv_snapshot_goto(bs->file, snapshot_id);
> +        ret = bdrv_snapshot_goto(bs->file, snapshot_id, errp);
>          open_ret = drv->bdrv_open(bs, bs->open_flags);
>          if (open_ret < 0) {
>              bdrv_delete(bs->file);
>              bs->drv = NULL;
> -            return open_ret;
> +            error_set(errp, QERR_OPEN_FILE_FAILED, bdrv_get_device_name(bs));
> +            ret = open_ret;
>          }
> -        return ret;
> +    } else {
> +        error_set(errp, QERR_NOT_SUPPORTED);
> +        ret = -ENOTSUP;
>      }
>  
> -    return -ENOTSUP;
> +    return ret;
>  }
>  
>  int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> diff --git a/block.h b/block.h
> index 92e782b..11edcd3 100644
> --- a/block.h
> +++ b/block.h
> @@ -299,7 +299,8 @@ int bdrv_snapshot_create(BlockDriverState *bs,
>                           QEMUSnapshotInfo *sn_info,
>                           Error **errp);
>  int bdrv_snapshot_goto(BlockDriverState *bs,
> -                       const char *snapshot_id);
> +                       const char *snapshot_id,
> +                       Error **errp);
>  int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
>  int bdrv_snapshot_list(BlockDriverState *bs,
>                         QEMUSnapshotInfo **psn_info);
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index cf86dae..8a87b0c 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -426,7 +426,9 @@ fail:
>  }
>  
>  /* copy the snapshot 'snapshot_name' into the current disk image */
> -int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
> +int qcow2_snapshot_goto(BlockDriverState *bs,
> +                        const char *snapshot_id,
> +                        Error **errp)
>  {
>      BDRVQcowState *s = bs->opaque;
>      QCowSnapshot *sn;
> @@ -438,13 +440,13 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const 
> char *snapshot_id)
>      /* Search the snapshot */
>      snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
>      if (snapshot_index < 0) {
> +        error_set(errp, QERR_OPEN_FILE_FAILED, snapshot_id);
>          return -ENOENT;
>      }
>      sn = &s->snapshots[snapshot_index];
>  
>      if (sn->disk_size != bs->total_sectors * BDRV_SECTOR_SIZE) {
> -        error_report("qcow2: Loading snapshots with different disk "
> -            "size is not implemented");
> +        error_set(errp, QERR_NOT_SUPPORTED);
>          ret = -ENOTSUP;
>          goto fail;
>      }
> @@ -536,6 +538,9 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char 
> *snapshot_id)
>  
>  fail:
>      g_free(sn_l1_table);
> +    if (!error_is_set(errp)) {
> +        error_set(errp, QERR_GENERIC_ERROR, ret);
> +    }
>      return ret;
>  }
>  
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 854bd12..6babb56 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -311,7 +311,9 @@ int qcow2_zero_clusters(BlockDriverState *bs, uint64_t 
> offset, int nb_sectors);
>  int qcow2_snapshot_create(BlockDriverState *bs,
>                            QEMUSnapshotInfo *sn_info,
>                            Error **errp);
> -int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
> +int qcow2_snapshot_goto(BlockDriverState *bs,
> +                        const char *snapshot_id,
> +                        Error **errp);
>  int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
>  int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
>  int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name);
> diff --git a/block/rbd.c b/block/rbd.c
> index 7bc42f0..5a73d8a 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -867,12 +867,16 @@ static int qemu_rbd_snap_remove(BlockDriverState *bs,
>  }
>  
>  static int qemu_rbd_snap_rollback(BlockDriverState *bs,
> -                                  const char *snapshot_name)
> +                                  const char *snapshot_name,
> +                                  Error **errp)
>  {
>      BDRVRBDState *s = bs->opaque;
>      int r;
>  
>      r = rbd_snap_rollback(s->image, snapshot_name);
> +    if (r < 0) {
> +        error_set(errp, QERR_GENERIC_ERROR, r);
> +    }

The Right Thing to do here is to add an Error argument to rbd_snap_rollback(),
as it knows the error cause. An alternative could be to do something like:

 error_setg(errp, "can't roll back: %s\n", strerror(-r));

This could even be done by the higher layers. As I said above, I think
it's better to discuss with the block layer guys first how the block layer
should be "qapimized".

>      return r;
>  }
>  
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index fc51e04..557ff92 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1799,7 +1799,9 @@ cleanup:
>      return ret;
>  }
>  
> -static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
> +static int sd_snapshot_goto(BlockDriverState *bs,
> +                            const char *snapshot_id,
> +                            Error **errp)
>  {
>      BDRVSheepdogState *s = bs->opaque;
>      BDRVSheepdogState *old_s;
> @@ -1824,13 +1826,12 @@ static int sd_snapshot_goto(BlockDriverState *bs, 
> const char *snapshot_id)
>  
>      ret = find_vdi_name(s, vdi, snapid, tag, &vid, 1);
>      if (ret) {
> -        error_report("Failed to find_vdi_name");
> +        error_set(errp, QERR_OPEN_FILE_FAILED, vdi);
>          goto out;
>      }
>  
>      fd = connect_to_sdog(s->addr, s->port);
>      if (fd < 0) {
> -        error_report("failed to connect");
>          ret = fd;
>          goto out;
>      }
> @@ -1848,7 +1849,8 @@ static int sd_snapshot_goto(BlockDriverState *bs, const 
> char *snapshot_id)
>      memcpy(&s->inode, buf, sizeof(s->inode));
>  
>      if (!s->inode.vm_state_size) {
> -        error_report("Invalid snapshot");
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> +                  "name", "existing snapshot");
>          ret = -ENOENT;
>          goto out;
>      }
> @@ -1864,9 +1866,9 @@ out:
>      memcpy(s, old_s, sizeof(BDRVSheepdogState));
>      g_free(buf);
>      g_free(old_s);
> -
> -    error_report("failed to open. recover old bdrv_sd_state.");
> -
> +    if (!error_is_set(errp)) {
> +        error_set(errp, QERR_GENERIC_ERROR, ret);
> +    }
>      return ret;
>  }
>  
> diff --git a/block_int.h b/block_int.h
> index b76c943..fa6d72f 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -209,7 +209,8 @@ struct BlockDriver {
>                                  QEMUSnapshotInfo *sn_info,
>                                  Error **errp);
>      int (*bdrv_snapshot_goto)(BlockDriverState *bs,
> -                              const char *snapshot_id);
> +                              const char *snapshot_id,
> +                              Error **errp);
>      int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char 
> *snapshot_id);
>      int (*bdrv_snapshot_list)(BlockDriverState *bs,
>                                QEMUSnapshotInfo **psn_info);
> diff --git a/qemu-img.c b/qemu-img.c
> index c798d66..1f10ff4 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1275,7 +1275,7 @@ static int img_snapshot(int argc, char **argv)
>          break;
>  
>      case SNAPSHOT_APPLY:
> -        ret = bdrv_snapshot_goto(bs, snapshot_name);
> +        ret = bdrv_snapshot_goto(bs, snapshot_name, NULL);
>          if (ret) {
>              error_report("Could not apply snapshot '%s': %d (%s)",
>                  snapshot_name, ret, strerror(-ret));
> diff --git a/savevm.c b/savevm.c
> index 1b67af6..0931e54 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2256,7 +2256,7 @@ int load_vmstate(const char *name)
>      bs = NULL;
>      while ((bs = bdrv_next(bs))) {
>          if (bdrv_can_snapshot(bs)) {
> -            ret = bdrv_snapshot_goto(bs, name);
> +            ret = bdrv_snapshot_goto(bs, name, NULL);
>              if (ret < 0) {
>                  error_report("Error %d while activating snapshot '%s' on 
> '%s'",
>                               ret, name, bdrv_get_device_name(bs));