diff mbox

[05/11] block: update error reporting for bdrv_snapshot_goto() and related functions

Message ID bc286e9c6c8615ca4925bfedaa7bbbb6c37e3983.1366127809.git.phrdina@redhat.com
State New
Headers show

Commit Message

Pavel Hrdina April 16, 2013, 4:05 p.m. UTC 
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 block.c                   | 55 ++++++++++++++++++++++++++---------------------
 block/qcow2-snapshot.c    | 32 +++++++++++++++++++--------
 block/qcow2.h             |  8 +++++--
 block/rbd.c               | 19 +++++++++++-----
 block/sheepdog.c          | 33 ++++++++++++++++------------
 include/block/block.h     |  8 ++++---
 include/block/block_int.h |  8 ++++---
 qemu-img.c                | 20 ++++++++++-------
 savevm.c                  | 21 ++++++++++--------
 9 files changed, 127 insertions(+), 77 deletions(-)

Comments

Eric Blake April 16, 2013, 8:48 p.m. UTC | #1 
On 04/16/2013 10:05 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  block.c                   | 55 ++++++++++++++++++++++++++---------------------
>  block/qcow2-snapshot.c    | 32 +++++++++++++++++++--------
>  block/qcow2.h             |  8 +++++--
>  block/rbd.c               | 19 +++++++++++-----
>  block/sheepdog.c          | 33 ++++++++++++++++------------
>  include/block/block.h     |  8 ++++---
>  include/block/block_int.h |  8 ++++---
>  qemu-img.c                | 20 ++++++++++-------
>  savevm.c                  | 21 ++++++++++--------
>  9 files changed, 127 insertions(+), 77 deletions(-)
> 

> +    } else if (bs->file) {
>          drv->bdrv_close(bs);
> -        ret = bdrv_snapshot_goto(bs->file, snapshot_id);
> -        open_ret = drv->bdrv_open(bs, NULL, bs->open_flags);
> -        if (open_ret < 0) {
> +        bdrv_snapshot_goto(bs->file, snapshot_id, errp);
> +        ret = drv->bdrv_open(bs, NULL, bs->open_flags);
> +        if (ret < 0) {
>              bdrv_delete(bs->file);
>              bs->drv = NULL;
> -            return open_ret;
> +            error_setg(errp, "failed to open '%s'", bdrv_get_device_name(bs));

Do we still want to try bdrv_open() if bdrv_snapshot_goto() set errp?
For that matter, if bdrv_snapshot_goto() _did_ set errp, does
error_setg() allow you to overwrite errors, or are you violating
constraints?

This is another case of partial failure; I'm wondering if we need to
eventually tidy this code up to use transaction semantics, so that a
loadvm is all-or-none, rather than risking partial failure.

> @@ -3410,16 +3410,23 @@ void bdrv_snapshot_delete(BlockDriverState *bs,
>  }
>  
>  int bdrv_snapshot_list(BlockDriverState *bs,
> -                       QEMUSnapshotInfo **psn_info)
> +                       QEMUSnapshotInfo **psn_info,
> +                       Error **errp)
>  {
>      BlockDriver *drv = bs->drv;
> -    if (!drv)
> -        return -ENOMEDIUM;
> -    if (drv->bdrv_snapshot_list)
> -        return drv->bdrv_snapshot_list(bs, psn_info);
> -    if (bs->file)
> -        return bdrv_snapshot_list(bs->file, psn_info);
> -    return -ENOTSUP;
> +
> +    if (!drv) {
> +        error_setg(errp, "device '%s' has no medium", bdrv_get_device_name(bs));
> +        return 0;
> +    } else if (drv->bdrv_snapshot_list) {
> +        return drv->bdrv_snapshot_list(bs, psn_info, errp);
> +    } else if (bs->file) {
> +        return bdrv_snapshot_list(bs->file, psn_info, errp);
> +    } else {
> +        error_setg(errp, "snapshots are not supported on device '%s'",
> +                   bdrv_get_device_name(bs));
> +        return 0;

Why 0 for error?  Don't you want to reserve 0 for 'snapshots are
supported, but none exist', and use -1 for error instead?  Or are we
safe treating no medium and no support in the same was as supported but
the list is 0-length?

> @@ -447,6 +450,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
>       */
>      ret = qcow2_grow_l1_table(bs, sn->l1_size, true);
>      if (ret < 0) {
> +        error_setg_errno(errp, -ret, "fqcow2: ailed to grow L1 table");

s/ailed/failed/

> @@ -595,7 +606,9 @@ void qcow2_snapshot_delete(BlockDriverState *bs,
>  #endif
>  }
>  
> -int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
> +int qcow2_snapshot_list(BlockDriverState *bs,
> +                        QEMUSnapshotInfo **psn_tab,
> +                        Error **errp)
>  {
>      BDRVQcowState *s = bs->opaque;
>      QEMUSnapshotInfo *sn_tab, *sn_info;
> @@ -604,7 +617,8 @@ int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
>  
>      if (!s->nb_snapshots) {
>          *psn_tab = NULL;
> -        return s->nb_snapshots;
> +        error_setg(errp, "qcow2: there is no snapshot available");
> +        return 0;

Note that inside the 'if' block, s->nb_snapshots == 0, so there is no
change in the return value, but now you are declaring this an error
where it previously just left a NULL *psn_tab.  Does the caller care?

> @@ -913,7 +917,12 @@ static int qemu_rbd_snap_list(BlockDriverState *bs,
>          }
>      } while (snap_count == -ERANGE);
>  
> -    if (snap_count <= 0) {
> +    if (snap_count < 0) {
> +        error_setg_errno(errp, -snap_count, "rbd: failed to find snapshots");
> +        snap_count = 0;

This is changing the return value from negative to 0.  Does the caller care?

> +static int sd_snapshot_list(BlockDriverState *bs,
> +                            QEMUSnapshotInfo **psn_tab,
> +                            Error **errp)
>  {
>      BDRVSheepdogState *s = bs->opaque;
>      SheepdogReq req;
> -    int fd, nr = 1024, ret, max = BITS_TO_LONGS(SD_NR_VDIS) * sizeof(long);
> +    int fd, nr = 1024, ret = 0, max = BITS_TO_LONGS(SD_NR_VDIS) * sizeof(long);
>      QEMUSnapshotInfo *sn_tab = NULL;
>      unsigned wlen, rlen;
>      int found = 0;
> @@ -1934,7 +1937,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
>  
>      fd = connect_to_sdog(s);
>      if (fd < 0) {
> -        ret = fd;
> +        error_setg_errno(errp, -fd, "sd: failed to connect to sdog");
>          goto out;

Another case of changing the result from -1 to 0; does the caller care?

> @@ -2001,7 +2005,8 @@ out:
>      g_free(vdi_inuse);
>  
>      if (ret < 0) {
> -        return ret;
> +        error_setg_errno(errp, -ret, "sd: failed to read VDI object");
> +        return 0;

and again, in the same function.

> +++ b/include/block/block_int.h
> @@ -155,13 +155,15 @@ struct BlockDriver {
>  
>      int (*bdrv_snapshot_create)(BlockDriverState *bs,
>                                  QEMUSnapshotInfo *sn_info);
> -    int (*bdrv_snapshot_goto)(BlockDriverState *bs,
> -                              const char *snapshot_id);
> +    void (*bdrv_snapshot_goto)(BlockDriverState *bs,
> +                               const char *snapshot_id,
> +                               Error **errp);

It would be really nice if we documented the contracts of all these
callback functions.

> +++ b/qemu-img.c
> @@ -1563,10 +1563,13 @@ static void dump_snapshots(BlockDriverState *bs)
>      QEMUSnapshotInfo *sn_tab, *sn;
>      int nb_sns, i;
>      char buf[256];
> +    Error *local_err = NULL;
>  
> -    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> -    if (nb_sns <= 0)
> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab, &local_err);
> +    if (qemu_img_handle_error("qemu-img dump snapshots failed", local_err)
> +            || nb_sns == 0) {

Interesting - you are stating that if no error was reported in
local_err, then a return of 0 short-circuits.  I was asking about all
the places where you changed return semantics; if this is the only
caller, then when local_err is set you ignore nb_sns and therefore the
return value is irrelevant (and changing from -1 to 0 makes no
difference to this code doing a short-circuit).  But again, calling that
out as a contract, where we can verify that each implementation of the
callback function obeys the contract, would make me feel a bit better.

> +++ b/savevm.c
> @@ -2206,7 +2206,7 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>          return found;
>      }
>  
> -    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab, NULL);
>      if (nb_sns < 0) {
>          return found;

Oops.  qemu-img wasn't the only caller.  Here, changing -1 to 0 on error
return means we fall through instead of returning 0 early.  Did you mean
for that to happen?  Calling with NULL says you don't care about error
reporting - is that right?
Kevin Wolf April 23, 2013, 2:08 p.m. UTC | #2 
Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben:
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>

Can you split this in two separate patches for goto and for list?

> ---
>  block.c                   | 55 ++++++++++++++++++++++++++---------------------
>  block/qcow2-snapshot.c    | 32 +++++++++++++++++++--------
>  block/qcow2.h             |  8 +++++--
>  block/rbd.c               | 19 +++++++++++-----
>  block/sheepdog.c          | 33 ++++++++++++++++------------
>  include/block/block.h     |  8 ++++---
>  include/block/block_int.h |  8 ++++---
>  qemu-img.c                | 20 ++++++++++-------
>  savevm.c                  | 21 ++++++++++--------
>  9 files changed, 127 insertions(+), 77 deletions(-)
> 
> diff --git a/block.c b/block.c
> index fb065e6..a760a2f 100644
> --- a/block.c
> +++ b/block.c
> @@ -3365,30 +3365,30 @@ int bdrv_snapshot_create(BlockDriverState *bs,
>      return -ENOTSUP;
>  }
>  
> -int bdrv_snapshot_goto(BlockDriverState *bs,
> -                       const char *snapshot_id)
> +void bdrv_snapshot_goto(BlockDriverState *bs,
> +                        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);
> +    int ret;
>  
> -    if (bs->file) {
> +    if (!drv) {
> +        error_setg(errp, "device '%s' has no medium", bdrv_get_device_name(bs));

It would probably make sense to define a macro for this message because
it's the same thing for pretty much every block layer function.

And the usual comment about upper case and the device name not being
necessary here (won't repeat them everywhere).

> +    } else if (drv->bdrv_snapshot_goto) {
> +        drv->bdrv_snapshot_goto(bs, snapshot_id, errp);
> +    } else if (bs->file) {
>          drv->bdrv_close(bs);
> -        ret = bdrv_snapshot_goto(bs->file, snapshot_id);
> -        open_ret = drv->bdrv_open(bs, NULL, bs->open_flags);
> -        if (open_ret < 0) {
> +        bdrv_snapshot_goto(bs->file, snapshot_id, errp);
> +        ret = drv->bdrv_open(bs, NULL, bs->open_flags);
> +        if (ret < 0) {
>              bdrv_delete(bs->file);
>              bs->drv = NULL;
> -            return open_ret;
> +            error_setg(errp, "failed to open '%s'", bdrv_get_device_name(bs));

Use error_setg_errno() so that we don't lose information.

If both bdrv_snapshot_goto() and bdrv_open() fail, errp is already set
and you'll trigger an assertion here. You need to decide which of the
errors should take precedence.

>          }
> -        return ret;
> +    } else {
> +        error_setg(errp, "snapshots are not supported on device '%s'",
> +                           bdrv_get_device_name(bs));
>      }
> -
> -    return -ENOTSUP;
>  }
>  
>  void bdrv_snapshot_delete(BlockDriverState *bs,
> @@ -3410,16 +3410,23 @@ void bdrv_snapshot_delete(BlockDriverState *bs,
>  }
>  
>  int bdrv_snapshot_list(BlockDriverState *bs,
> -                       QEMUSnapshotInfo **psn_info)
> +                       QEMUSnapshotInfo **psn_info,
> +                       Error **errp)
>  {
>      BlockDriver *drv = bs->drv;
> -    if (!drv)
> -        return -ENOMEDIUM;
> -    if (drv->bdrv_snapshot_list)
> -        return drv->bdrv_snapshot_list(bs, psn_info);
> -    if (bs->file)
> -        return bdrv_snapshot_list(bs->file, psn_info);
> -    return -ENOTSUP;
> +
> +    if (!drv) {
> +        error_setg(errp, "device '%s' has no medium", bdrv_get_device_name(bs));
> +        return 0;
> +    } else if (drv->bdrv_snapshot_list) {
> +        return drv->bdrv_snapshot_list(bs, psn_info, errp);
> +    } else if (bs->file) {
> +        return bdrv_snapshot_list(bs->file, psn_info, errp);
> +    } else {
> +        error_setg(errp, "snapshots are not supported on device '%s'",
> +                   bdrv_get_device_name(bs));
> +        return 0;
> +    }
>  }
>  
>  int bdrv_snapshot_load_tmp(BlockDriverState *bs,
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 2ebeadc..4a69b88 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -417,7 +417,9 @@ fail:
>  }
>  
>  /* copy the snapshot 'snapshot_name' into the current disk image */
> -int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
> +void qcow2_snapshot_goto(BlockDriverState *bs,
> +                         const char *snapshot_id,
> +                         Error **errp)
>  {
>      BDRVQcowState *s = bs->opaque;
>      QCowSnapshot *sn;
> @@ -429,14 +431,15 @@ 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) {
> -        return -ENOENT;
> +        error_setg(errp, "qcow2: failed to find snapshot '%s' by id or name",
> +                   snapshot_id);
> +        return;
>      }
>      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");
> -        ret = -ENOTSUP;
> +        error_setg(errp, "qcow2: loading snapshots with different disk size "
> +                   "is not implemented");
>          goto fail;
>      }
>  
> @@ -447,6 +450,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
>       */
>      ret = qcow2_grow_l1_table(bs, sn->l1_size, true);
>      if (ret < 0) {
> +        error_setg_errno(errp, -ret, "fqcow2: ailed to grow L1 table");

That 'f' isn't quite in the right place :-)

>          goto fail;
>      }
>  
> @@ -465,18 +469,23 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
>  
>      ret = bdrv_pread(bs->file, sn->l1_table_offset, sn_l1_table, sn_l1_bytes);
>      if (ret < 0) {
> +        error_setg_errno(errp, -ret,
> +                         "qcow2: failed to load data into L1 table");
>          goto fail;
>      }
>  
>      ret = qcow2_update_snapshot_refcount(bs, sn->l1_table_offset,
>                                           sn->l1_size, 1);
>      if (ret < 0) {
> +        error_setg_errno(errp, -ret,
> +                         "qcow2: failed to update snapshot refcount");

"Failed to increase the cluster refcounts of the snapshot to load"

>          goto fail;
>      }
>  
>      ret = bdrv_pwrite_sync(bs->file, s->l1_table_offset, sn_l1_table,
>                             cur_l1_bytes);
>      if (ret < 0) {
> +        error_setg_errno(errp, -ret, "qcow2: failed to save L1 table");
>          goto fail;
>      }
>  
> @@ -502,6 +511,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
>      }
>  
>      if (ret < 0) {
> +        error_setg_errno(errp, -ret, "qcow2: failed to sync L1 table");

"Failed to decrease the cluster refcounts of the old snapshot"

>          goto fail;
>      }
>  
> @@ -514,6 +524,8 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
>       */
>      ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 0);
>      if (ret < 0) {
> +        error_setg_errno(errp, -ret,
> +                         "qcow2: failed to update snapshot refcount");

"Failed to update the cluster flags"

>          goto fail;
>      }
>  
> @@ -523,11 +535,10 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
>          qcow2_check_refcounts(bs, &result, 0);
>      }
>  #endif
> -    return 0;
> +    return;
>  
>  fail:
>      g_free(sn_l1_table);
> -    return ret;
>  }
>  
>  void qcow2_snapshot_delete(BlockDriverState *bs,
> @@ -595,7 +606,9 @@ void qcow2_snapshot_delete(BlockDriverState *bs,
>  #endif
>  }
>  
> -int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
> +int qcow2_snapshot_list(BlockDriverState *bs,
> +                        QEMUSnapshotInfo **psn_tab,
> +                        Error **errp)
>  {
>      BDRVQcowState *s = bs->opaque;
>      QEMUSnapshotInfo *sn_tab, *sn_info;
> @@ -604,7 +617,8 @@ int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
>  
>      if (!s->nb_snapshots) {
>          *psn_tab = NULL;
> -        return s->nb_snapshots;
> +        error_setg(errp, "qcow2: there is no snapshot available");
> +        return 0;

Why is this an error condition?

>      }
>  
>      sn_tab = g_malloc0(s->nb_snapshots * sizeof(QEMUSnapshotInfo));
> diff --git a/block/qcow2.h b/block/qcow2.h
> index dbd332d..ae62953 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -383,11 +383,15 @@ 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_goto(BlockDriverState *bs, const char *snapshot_id);
> +void qcow2_snapshot_goto(BlockDriverState *bs,
> +                         const char *snapshot_id,
> +                         Error **errp);
>  void qcow2_snapshot_delete(BlockDriverState *bs,
>                             const char *snapshot_id,
>                             Error **errp);
> -int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
> +int qcow2_snapshot_list(BlockDriverState *bs,
> +                        QEMUSnapshotInfo **psn_tab,
> +                        Error **errp);
>  int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name);
>  
>  void qcow2_free_snapshots(BlockDriverState *bs);
> diff --git a/block/rbd.c b/block/rbd.c
> index c10edbf..9259621 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -886,18 +886,22 @@ static void qemu_rbd_snap_remove(BlockDriverState *bs,
>      }
>  }
>  
> -static int qemu_rbd_snap_rollback(BlockDriverState *bs,
> -                                  const char *snapshot_name)
> +static void qemu_rbd_snap_rollback(BlockDriverState *bs,
> +                                   const char *snapshot_name,
> +                                   Error **errp)
>  {
>      BDRVRBDState *s = bs->opaque;
>      int r;
>  
>      r = rbd_snap_rollback(s->image, snapshot_name);
> -    return r;
> +    if (r < 0) {
> +        error_setg_errno(errp, -r, "rbd: failed to rollback snapshot");
> +    }
>  }
>  
>  static int qemu_rbd_snap_list(BlockDriverState *bs,
> -                              QEMUSnapshotInfo **psn_tab)
> +                              QEMUSnapshotInfo **psn_tab,
> +                              Error **errp)
>  {
>      BDRVRBDState *s = bs->opaque;
>      QEMUSnapshotInfo *sn_info, *sn_tab = NULL;
> @@ -913,7 +917,12 @@ static int qemu_rbd_snap_list(BlockDriverState *bs,
>          }
>      } while (snap_count == -ERANGE);
>  
> -    if (snap_count <= 0) {
> +    if (snap_count < 0) {
> +        error_setg_errno(errp, -snap_count, "rbd: failed to find snapshots");

Here you don't have snap_count == 0 as an error condition, which makes
more sense to me. I think you should change qcow2.

> +        snap_count = 0;
> +    }
> +
> +    if (snap_count == 0) {
>          goto done;
>      }
>  
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 270fa64..3d44575 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1838,7 +1838,9 @@ cleanup:
>      return ret;
>  }
>  
> -static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
> +static void sd_snapshot_goto(BlockDriverState *bs,
> +                             const char *snapshot_id,
> +                             Error **errp)
>  {
>      BDRVSheepdogState *s = bs->opaque;
>      BDRVSheepdogState *old_s;
> @@ -1863,12 +1865,14 @@ 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_setg_errno(errp, -ret, "sd: failed to find VDI snapshot '%s'",
> +                         vdi);
>          goto out;
>      }
>  
>      fd = connect_to_sdog(s);
>      if (fd < 0) {
> +        error_setg_errno(errp, -fd, "sd: failed to connect to sdog");
>          ret = fd;

It ret ever used after this assignment?

>          goto out;
>      }
> @@ -1880,14 +1884,15 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
>      closesocket(fd);
>  
>      if (ret) {
> +        error_setg_errno(errp, -ret, "sd: failed to open VDI snapshot '%s'",
> +                         vdi);
>          goto out;
>      }
>  
>      memcpy(&s->inode, buf, sizeof(s->inode));
>  
>      if (!s->inode.vm_state_size) {
> -        error_report("Invalid snapshot");
> -        ret = -ENOENT;
> +        error_setg(errp, "sd: invalid snapshot '%s'", snapshot_id);
>          goto out;
>      }
>  
> @@ -1896,16 +1901,12 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
>      g_free(buf);
>      g_free(old_s);
>  
> -    return 0;
> +    return;
>  out:
>      /* recover bdrv_sd_state */
>      memcpy(s, old_s, sizeof(BDRVSheepdogState));
>      g_free(buf);
>      g_free(old_s);
> -
> -    error_report("failed to open. recover old bdrv_sd_state.");
> -
> -    return ret;
>  }
>  
>  static void sd_snapshot_delete(BlockDriverState *bs,
> @@ -1916,11 +1917,13 @@ static void sd_snapshot_delete(BlockDriverState *bs,
>      return;
>  }
>  
> -static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
> +static int sd_snapshot_list(BlockDriverState *bs,
> +                            QEMUSnapshotInfo **psn_tab,
> +                            Error **errp)
>  {
>      BDRVSheepdogState *s = bs->opaque;
>      SheepdogReq req;
> -    int fd, nr = 1024, ret, max = BITS_TO_LONGS(SD_NR_VDIS) * sizeof(long);
> +    int fd, nr = 1024, ret = 0, max = BITS_TO_LONGS(SD_NR_VDIS) * sizeof(long);

This change has no effect. ret is overwritten with the return value of
do_req() before it's read.

>      QEMUSnapshotInfo *sn_tab = NULL;
>      unsigned wlen, rlen;
>      int found = 0;
> @@ -1934,7 +1937,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
>  
>      fd = connect_to_sdog(s);
>      if (fd < 0) {
> -        ret = fd;
> +        error_setg_errno(errp, -fd, "sd: failed to connect to sdog");

It's probably nicer to leave an explicit ret = 0 here, even if it
happens to always be 0 in this place anyway.

>          goto out;
>      }
>  
> @@ -1950,6 +1953,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
>  
>      closesocket(fd);
>      if (ret) {
> +        error_setg_errno(errp, -ret, "sd: failed to read VDIs");
>          goto out;
>      }
>  
> @@ -1961,7 +1965,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
>  
>      fd = connect_to_sdog(s);
>      if (fd < 0) {
> -        ret = fd;
> +        error_setg_errno(errp, -fd, "sd: failed to connect to sdog");
>          goto out;
>      }
>  
> @@ -2001,7 +2005,8 @@ out:
>      g_free(vdi_inuse);
>  
>      if (ret < 0) {
> -        return ret;
> +        error_setg_errno(errp, -ret, "sd: failed to read VDI object");
> +        return 0;
>      }
>  
>      return found;
> diff --git a/include/block/block.h b/include/block/block.h
> index db8c6aa..33d498b 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -335,13 +335,15 @@ int bdrv_is_snapshot(BlockDriverState *bs);
>  BlockDriverState *bdrv_snapshots(void);
>  int bdrv_snapshot_create(BlockDriverState *bs,
>                           QEMUSnapshotInfo *sn_info);
> -int bdrv_snapshot_goto(BlockDriverState *bs,
> -                       const char *snapshot_id);
> +void bdrv_snapshot_goto(BlockDriverState *bs,
> +                        const char *snapshot_id,
> +                        Error **errp);
>  void bdrv_snapshot_delete(BlockDriverState *bs,
>                            const char *snapshot_id,
>                            Error **errp);
>  int bdrv_snapshot_list(BlockDriverState *bs,
> -                       QEMUSnapshotInfo **psn_info);
> +                       QEMUSnapshotInfo **psn_info,
> +                       Error **errp);
>  int bdrv_snapshot_load_tmp(BlockDriverState *bs,
>                             const char *snapshot_name);
>  char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 53c52bb..c56d923 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -155,13 +155,15 @@ struct BlockDriver {
>  
>      int (*bdrv_snapshot_create)(BlockDriverState *bs,
>                                  QEMUSnapshotInfo *sn_info);
> -    int (*bdrv_snapshot_goto)(BlockDriverState *bs,
> -                              const char *snapshot_id);
> +    void (*bdrv_snapshot_goto)(BlockDriverState *bs,
> +                               const char *snapshot_id,
> +                               Error **errp);
>      void (*bdrv_snapshot_delete)(BlockDriverState *bs,
>                                   const char *snapshot_id,
>                                   Error **errp);
>      int (*bdrv_snapshot_list)(BlockDriverState *bs,
> -                              QEMUSnapshotInfo **psn_info);
> +                              QEMUSnapshotInfo **psn_info,
> +                              Error **errp);
>      int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
>                                    const char *snapshot_name);
>      int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
> diff --git a/qemu-img.c b/qemu-img.c
> index 4828fe4..06cd8af 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1563,10 +1563,13 @@ static void dump_snapshots(BlockDriverState *bs)
>      QEMUSnapshotInfo *sn_tab, *sn;
>      int nb_sns, i;
>      char buf[256];
> +    Error *local_err = NULL;
>  
> -    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> -    if (nb_sns <= 0)
> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab, &local_err);
> +    if (qemu_img_handle_error("qemu-img dump snapshots failed", local_err)
> +            || nb_sns == 0) {
>          return;
> +    }
>      printf("Snapshot list:\n");
>      printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
>      for(i = 0; i < nb_sns; i++) {
> @@ -1598,7 +1601,10 @@ static void collect_snapshots(BlockDriverState *bs , ImageInfo *info)
>      int i, sn_count;
>      QEMUSnapshotInfo *sn_tab = NULL;
>      SnapshotInfoList *info_list, *cur_item = NULL;
> -    sn_count = bdrv_snapshot_list(bs, &sn_tab);
> +    Error *local_err = NULL;
> +    sn_count = bdrv_snapshot_list(bs, &sn_tab, &local_err);
> +
> +    qemu_img_handle_error("qemu-img collect snapshots failed", local_err);
>  
>      for (i = 0; i < sn_count; i++) {
>          info->has_snapshots = true;
> @@ -2026,11 +2032,9 @@ static int img_snapshot(int argc, char **argv)
>          break;
>  
>      case SNAPSHOT_APPLY:
> -        ret = bdrv_snapshot_goto(bs, snapshot_name);
> -        if (ret) {
> -            error_report("Could not apply snapshot '%s': %d (%s)",
> -                snapshot_name, ret, strerror(-ret));
> -        }
> +        bdrv_snapshot_goto(bs, snapshot_name, &local_err);
> +        ret = qemu_img_handle_error("qemu-img snapshot apply failed",
> +                                    local_err);
>          break;
>  
>      case SNAPSHOT_DELETE:
> diff --git a/savevm.c b/savevm.c
> index 4af7d2d..ca4a42e 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2206,7 +2206,7 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>          return found;
>      }
>  
> -    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab, NULL);
>      if (nb_sns < 0) {
>          return found;
>      }
> @@ -2401,6 +2401,7 @@ int load_vmstate(const char *name)
>      QEMUSnapshotInfo sn;
>      QEMUFile *f;
>      int ret;
> +    Error *local_err;
>  
>      bs_vm_state = bdrv_snapshots();
>      if (!bs_vm_state) {
> @@ -2445,11 +2446,11 @@ int load_vmstate(const char *name)
>      bs = NULL;
>      while ((bs = bdrv_next(bs))) {
>          if (bdrv_can_snapshot(bs)) {
> -            ret = bdrv_snapshot_goto(bs, name);
> -            if (ret < 0) {
> -                error_report("Error %d while activating snapshot '%s' on '%s'",
> -                             ret, name, bdrv_get_device_name(bs));
> -                return ret;
> +            bdrv_snapshot_goto(bs, name, &local_err);
> +            if (error_is_set(&local_err)) {
> +                error_report("%s", error_get_pretty(local_err));

qerror_report_err()

> +                error_free(local_err);
> +                return -1;

Use some -errno code, and if it's only -EIO. If you don't choose one, it
becomes -EPERM in practice, which is confusing.

>              }
>          }
>      }
> @@ -2528,6 +2529,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
>      int total;
>      int *available_snapshots;
>      char buf[256];
> +    Error *local_err = NULL;
>  
>      bs = bdrv_snapshots();
>      if (!bs) {
> @@ -2535,9 +2537,10 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
>          return;
>      }
>  
> -    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> -    if (nb_sns < 0) {
> -        monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab, &local_err);
> +    if (error_is_set(&local_err)) {
> +        monitor_printf(mon, "%s\n", error_get_pretty(local_err));

And another qerror_report_err().

> +        error_free(local_err);
>          return;
>      }

Kevin
diff mbox

Patch

diff --git a/block.c b/block.c
index fb065e6..a760a2f 100644
--- a/block.c
+++ b/block.c
@@ -3365,30 +3365,30 @@  int bdrv_snapshot_create(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
-int bdrv_snapshot_goto(BlockDriverState *bs,
-                       const char *snapshot_id)
+void bdrv_snapshot_goto(BlockDriverState *bs,
+                        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);
+    int ret;
 
-    if (bs->file) {
+    if (!drv) {
+        error_setg(errp, "device '%s' has no medium", bdrv_get_device_name(bs));
+    } else if (drv->bdrv_snapshot_goto) {
+        drv->bdrv_snapshot_goto(bs, snapshot_id, errp);
+    } else if (bs->file) {
         drv->bdrv_close(bs);
-        ret = bdrv_snapshot_goto(bs->file, snapshot_id);
-        open_ret = drv->bdrv_open(bs, NULL, bs->open_flags);
-        if (open_ret < 0) {
+        bdrv_snapshot_goto(bs->file, snapshot_id, errp);
+        ret = drv->bdrv_open(bs, NULL, bs->open_flags);
+        if (ret < 0) {
             bdrv_delete(bs->file);
             bs->drv = NULL;
-            return open_ret;
+            error_setg(errp, "failed to open '%s'", bdrv_get_device_name(bs));
         }
-        return ret;
+    } else {
+        error_setg(errp, "snapshots are not supported on device '%s'",
+                           bdrv_get_device_name(bs));
     }
-
-    return -ENOTSUP;
 }
 
 void bdrv_snapshot_delete(BlockDriverState *bs,
@@ -3410,16 +3410,23 @@  void bdrv_snapshot_delete(BlockDriverState *bs,
 }
 
 int bdrv_snapshot_list(BlockDriverState *bs,
-                       QEMUSnapshotInfo **psn_info)
+                       QEMUSnapshotInfo **psn_info,
+                       Error **errp)
 {
     BlockDriver *drv = bs->drv;
-    if (!drv)
-        return -ENOMEDIUM;
-    if (drv->bdrv_snapshot_list)
-        return drv->bdrv_snapshot_list(bs, psn_info);
-    if (bs->file)
-        return bdrv_snapshot_list(bs->file, psn_info);
-    return -ENOTSUP;
+
+    if (!drv) {
+        error_setg(errp, "device '%s' has no medium", bdrv_get_device_name(bs));
+        return 0;
+    } else if (drv->bdrv_snapshot_list) {
+        return drv->bdrv_snapshot_list(bs, psn_info, errp);
+    } else if (bs->file) {
+        return bdrv_snapshot_list(bs->file, psn_info, errp);
+    } else {
+        error_setg(errp, "snapshots are not supported on device '%s'",
+                   bdrv_get_device_name(bs));
+        return 0;
+    }
 }
 
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 2ebeadc..4a69b88 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -417,7 +417,9 @@  fail:
 }
 
 /* copy the snapshot 'snapshot_name' into the current disk image */
-int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
+void qcow2_snapshot_goto(BlockDriverState *bs,
+                         const char *snapshot_id,
+                         Error **errp)
 {
     BDRVQcowState *s = bs->opaque;
     QCowSnapshot *sn;
@@ -429,14 +431,15 @@  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) {
-        return -ENOENT;
+        error_setg(errp, "qcow2: failed to find snapshot '%s' by id or name",
+                   snapshot_id);
+        return;
     }
     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");
-        ret = -ENOTSUP;
+        error_setg(errp, "qcow2: loading snapshots with different disk size "
+                   "is not implemented");
         goto fail;
     }
 
@@ -447,6 +450,7 @@  int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
      */
     ret = qcow2_grow_l1_table(bs, sn->l1_size, true);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "fqcow2: ailed to grow L1 table");
         goto fail;
     }
 
@@ -465,18 +469,23 @@  int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
 
     ret = bdrv_pread(bs->file, sn->l1_table_offset, sn_l1_table, sn_l1_bytes);
     if (ret < 0) {
+        error_setg_errno(errp, -ret,
+                         "qcow2: failed to load data into L1 table");
         goto fail;
     }
 
     ret = qcow2_update_snapshot_refcount(bs, sn->l1_table_offset,
                                          sn->l1_size, 1);
     if (ret < 0) {
+        error_setg_errno(errp, -ret,
+                         "qcow2: failed to update snapshot refcount");
         goto fail;
     }
 
     ret = bdrv_pwrite_sync(bs->file, s->l1_table_offset, sn_l1_table,
                            cur_l1_bytes);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "qcow2: failed to save L1 table");
         goto fail;
     }
 
@@ -502,6 +511,7 @@  int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     }
 
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "qcow2: failed to sync L1 table");
         goto fail;
     }
 
@@ -514,6 +524,8 @@  int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
      */
     ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 0);
     if (ret < 0) {
+        error_setg_errno(errp, -ret,
+                         "qcow2: failed to update snapshot refcount");
         goto fail;
     }
 
@@ -523,11 +535,10 @@  int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
         qcow2_check_refcounts(bs, &result, 0);
     }
 #endif
-    return 0;
+    return;
 
 fail:
     g_free(sn_l1_table);
-    return ret;
 }
 
 void qcow2_snapshot_delete(BlockDriverState *bs,
@@ -595,7 +606,9 @@  void qcow2_snapshot_delete(BlockDriverState *bs,
 #endif
 }
 
-int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
+int qcow2_snapshot_list(BlockDriverState *bs,
+                        QEMUSnapshotInfo **psn_tab,
+                        Error **errp)
 {
     BDRVQcowState *s = bs->opaque;
     QEMUSnapshotInfo *sn_tab, *sn_info;
@@ -604,7 +617,8 @@  int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
 
     if (!s->nb_snapshots) {
         *psn_tab = NULL;
-        return s->nb_snapshots;
+        error_setg(errp, "qcow2: there is no snapshot available");
+        return 0;
     }
 
     sn_tab = g_malloc0(s->nb_snapshots * sizeof(QEMUSnapshotInfo));
diff --git a/block/qcow2.h b/block/qcow2.h
index dbd332d..ae62953 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -383,11 +383,15 @@  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_goto(BlockDriverState *bs, const char *snapshot_id);
+void qcow2_snapshot_goto(BlockDriverState *bs,
+                         const char *snapshot_id,
+                         Error **errp);
 void qcow2_snapshot_delete(BlockDriverState *bs,
                            const char *snapshot_id,
                            Error **errp);
-int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
+int qcow2_snapshot_list(BlockDriverState *bs,
+                        QEMUSnapshotInfo **psn_tab,
+                        Error **errp);
 int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name);
 
 void qcow2_free_snapshots(BlockDriverState *bs);
diff --git a/block/rbd.c b/block/rbd.c
index c10edbf..9259621 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -886,18 +886,22 @@  static void qemu_rbd_snap_remove(BlockDriverState *bs,
     }
 }
 
-static int qemu_rbd_snap_rollback(BlockDriverState *bs,
-                                  const char *snapshot_name)
+static void qemu_rbd_snap_rollback(BlockDriverState *bs,
+                                   const char *snapshot_name,
+                                   Error **errp)
 {
     BDRVRBDState *s = bs->opaque;
     int r;
 
     r = rbd_snap_rollback(s->image, snapshot_name);
-    return r;
+    if (r < 0) {
+        error_setg_errno(errp, -r, "rbd: failed to rollback snapshot");
+    }
 }
 
 static int qemu_rbd_snap_list(BlockDriverState *bs,
-                              QEMUSnapshotInfo **psn_tab)
+                              QEMUSnapshotInfo **psn_tab,
+                              Error **errp)
 {
     BDRVRBDState *s = bs->opaque;
     QEMUSnapshotInfo *sn_info, *sn_tab = NULL;
@@ -913,7 +917,12 @@  static int qemu_rbd_snap_list(BlockDriverState *bs,
         }
     } while (snap_count == -ERANGE);
 
-    if (snap_count <= 0) {
+    if (snap_count < 0) {
+        error_setg_errno(errp, -snap_count, "rbd: failed to find snapshots");
+        snap_count = 0;
+    }
+
+    if (snap_count == 0) {
         goto done;
     }
 
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 270fa64..3d44575 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1838,7 +1838,9 @@  cleanup:
     return ret;
 }
 
-static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
+static void sd_snapshot_goto(BlockDriverState *bs,
+                             const char *snapshot_id,
+                             Error **errp)
 {
     BDRVSheepdogState *s = bs->opaque;
     BDRVSheepdogState *old_s;
@@ -1863,12 +1865,14 @@  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_setg_errno(errp, -ret, "sd: failed to find VDI snapshot '%s'",
+                         vdi);
         goto out;
     }
 
     fd = connect_to_sdog(s);
     if (fd < 0) {
+        error_setg_errno(errp, -fd, "sd: failed to connect to sdog");
         ret = fd;
         goto out;
     }
@@ -1880,14 +1884,15 @@  static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     closesocket(fd);
 
     if (ret) {
+        error_setg_errno(errp, -ret, "sd: failed to open VDI snapshot '%s'",
+                         vdi);
         goto out;
     }
 
     memcpy(&s->inode, buf, sizeof(s->inode));
 
     if (!s->inode.vm_state_size) {
-        error_report("Invalid snapshot");
-        ret = -ENOENT;
+        error_setg(errp, "sd: invalid snapshot '%s'", snapshot_id);
         goto out;
     }
 
@@ -1896,16 +1901,12 @@  static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     g_free(buf);
     g_free(old_s);
 
-    return 0;
+    return;
 out:
     /* recover bdrv_sd_state */
     memcpy(s, old_s, sizeof(BDRVSheepdogState));
     g_free(buf);
     g_free(old_s);
-
-    error_report("failed to open. recover old bdrv_sd_state.");
-
-    return ret;
 }
 
 static void sd_snapshot_delete(BlockDriverState *bs,
@@ -1916,11 +1917,13 @@  static void sd_snapshot_delete(BlockDriverState *bs,
     return;
 }
 
-static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
+static int sd_snapshot_list(BlockDriverState *bs,
+                            QEMUSnapshotInfo **psn_tab,
+                            Error **errp)
 {
     BDRVSheepdogState *s = bs->opaque;
     SheepdogReq req;
-    int fd, nr = 1024, ret, max = BITS_TO_LONGS(SD_NR_VDIS) * sizeof(long);
+    int fd, nr = 1024, ret = 0, max = BITS_TO_LONGS(SD_NR_VDIS) * sizeof(long);
     QEMUSnapshotInfo *sn_tab = NULL;
     unsigned wlen, rlen;
     int found = 0;
@@ -1934,7 +1937,7 @@  static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
 
     fd = connect_to_sdog(s);
     if (fd < 0) {
-        ret = fd;
+        error_setg_errno(errp, -fd, "sd: failed to connect to sdog");
         goto out;
     }
 
@@ -1950,6 +1953,7 @@  static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
 
     closesocket(fd);
     if (ret) {
+        error_setg_errno(errp, -ret, "sd: failed to read VDIs");
         goto out;
     }
 
@@ -1961,7 +1965,7 @@  static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
 
     fd = connect_to_sdog(s);
     if (fd < 0) {
-        ret = fd;
+        error_setg_errno(errp, -fd, "sd: failed to connect to sdog");
         goto out;
     }
 
@@ -2001,7 +2005,8 @@  out:
     g_free(vdi_inuse);
 
     if (ret < 0) {
-        return ret;
+        error_setg_errno(errp, -ret, "sd: failed to read VDI object");
+        return 0;
     }
 
     return found;
diff --git a/include/block/block.h b/include/block/block.h
index db8c6aa..33d498b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -335,13 +335,15 @@  int bdrv_is_snapshot(BlockDriverState *bs);
 BlockDriverState *bdrv_snapshots(void);
 int bdrv_snapshot_create(BlockDriverState *bs,
                          QEMUSnapshotInfo *sn_info);
-int bdrv_snapshot_goto(BlockDriverState *bs,
-                       const char *snapshot_id);
+void bdrv_snapshot_goto(BlockDriverState *bs,
+                        const char *snapshot_id,
+                        Error **errp);
 void bdrv_snapshot_delete(BlockDriverState *bs,
                           const char *snapshot_id,
                           Error **errp);
 int bdrv_snapshot_list(BlockDriverState *bs,
-                       QEMUSnapshotInfo **psn_info);
+                       QEMUSnapshotInfo **psn_info,
+                       Error **errp);
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
                            const char *snapshot_name);
 char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 53c52bb..c56d923 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -155,13 +155,15 @@  struct BlockDriver {
 
     int (*bdrv_snapshot_create)(BlockDriverState *bs,
                                 QEMUSnapshotInfo *sn_info);
-    int (*bdrv_snapshot_goto)(BlockDriverState *bs,
-                              const char *snapshot_id);
+    void (*bdrv_snapshot_goto)(BlockDriverState *bs,
+                               const char *snapshot_id,
+                               Error **errp);
     void (*bdrv_snapshot_delete)(BlockDriverState *bs,
                                  const char *snapshot_id,
                                  Error **errp);
     int (*bdrv_snapshot_list)(BlockDriverState *bs,
-                              QEMUSnapshotInfo **psn_info);
+                              QEMUSnapshotInfo **psn_info,
+                              Error **errp);
     int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
                                   const char *snapshot_name);
     int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
diff --git a/qemu-img.c b/qemu-img.c
index 4828fe4..06cd8af 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1563,10 +1563,13 @@  static void dump_snapshots(BlockDriverState *bs)
     QEMUSnapshotInfo *sn_tab, *sn;
     int nb_sns, i;
     char buf[256];
+    Error *local_err = NULL;
 
-    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
-    if (nb_sns <= 0)
+    nb_sns = bdrv_snapshot_list(bs, &sn_tab, &local_err);
+    if (qemu_img_handle_error("qemu-img dump snapshots failed", local_err)
+            || nb_sns == 0) {
         return;
+    }
     printf("Snapshot list:\n");
     printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
     for(i = 0; i < nb_sns; i++) {
@@ -1598,7 +1601,10 @@  static void collect_snapshots(BlockDriverState *bs , ImageInfo *info)
     int i, sn_count;
     QEMUSnapshotInfo *sn_tab = NULL;
     SnapshotInfoList *info_list, *cur_item = NULL;
-    sn_count = bdrv_snapshot_list(bs, &sn_tab);
+    Error *local_err = NULL;
+    sn_count = bdrv_snapshot_list(bs, &sn_tab, &local_err);
+
+    qemu_img_handle_error("qemu-img collect snapshots failed", local_err);
 
     for (i = 0; i < sn_count; i++) {
         info->has_snapshots = true;
@@ -2026,11 +2032,9 @@  static int img_snapshot(int argc, char **argv)
         break;
 
     case SNAPSHOT_APPLY:
-        ret = bdrv_snapshot_goto(bs, snapshot_name);
-        if (ret) {
-            error_report("Could not apply snapshot '%s': %d (%s)",
-                snapshot_name, ret, strerror(-ret));
-        }
+        bdrv_snapshot_goto(bs, snapshot_name, &local_err);
+        ret = qemu_img_handle_error("qemu-img snapshot apply failed",
+                                    local_err);
         break;
 
     case SNAPSHOT_DELETE:
diff --git a/savevm.c b/savevm.c
index 4af7d2d..ca4a42e 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2206,7 +2206,7 @@  static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
         return found;
     }
 
-    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+    nb_sns = bdrv_snapshot_list(bs, &sn_tab, NULL);
     if (nb_sns < 0) {
         return found;
     }
@@ -2401,6 +2401,7 @@  int load_vmstate(const char *name)
     QEMUSnapshotInfo sn;
     QEMUFile *f;
     int ret;
+    Error *local_err;
 
     bs_vm_state = bdrv_snapshots();
     if (!bs_vm_state) {
@@ -2445,11 +2446,11 @@  int load_vmstate(const char *name)
     bs = NULL;
     while ((bs = bdrv_next(bs))) {
         if (bdrv_can_snapshot(bs)) {
-            ret = bdrv_snapshot_goto(bs, name);
-            if (ret < 0) {
-                error_report("Error %d while activating snapshot '%s' on '%s'",
-                             ret, name, bdrv_get_device_name(bs));
-                return ret;
+            bdrv_snapshot_goto(bs, name, &local_err);
+            if (error_is_set(&local_err)) {
+                error_report("%s", error_get_pretty(local_err));
+                error_free(local_err);
+                return -1;
             }
         }
     }
@@ -2528,6 +2529,7 @@  void do_info_snapshots(Monitor *mon, const QDict *qdict)
     int total;
     int *available_snapshots;
     char buf[256];
+    Error *local_err = NULL;
 
     bs = bdrv_snapshots();
     if (!bs) {
@@ -2535,9 +2537,10 @@  void do_info_snapshots(Monitor *mon, const QDict *qdict)
         return;
     }
 
-    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
-    if (nb_sns < 0) {
-        monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
+    nb_sns = bdrv_snapshot_list(bs, &sn_tab, &local_err);
+    if (error_is_set(&local_err)) {
+        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
+        error_free(local_err);
         return;
     }