Re: [Qemu-devel] [PATCH 05/11] block: update error reporting for bdrv_snapshot_goto() and related functions

[Kevin Wolf]

Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben:
> 
> Signed-off-by: Pavel Hrdina <address@hidden>

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