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

[Kevin Wolf <kwolf@redhat.com>]

Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben:
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  block.c                   | 22 ++++++++++++++--------
>  block/qcow2-snapshot.c    | 21 +++++++++++++++------
>  block/qcow2.h             |  4 +++-
>  block/rbd.c               | 11 ++++++++---
>  block/sheepdog.c          |  6 ++++--
>  include/block/block.h     |  4 +++-
>  include/block/block_int.h |  4 +++-
>  qemu-img.c                |  9 ++++-----
>  savevm.c                  | 26 ++++++++++----------------
>  9 files changed, 64 insertions(+), 43 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 4ad663d..fb065e6 100644
> --- a/block.c
> +++ b/block.c
> @@ -3391,16 +3391,22 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>      return -ENOTSUP;
>  }
>  
> -int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> +void bdrv_snapshot_delete(BlockDriverState *bs,
> +                          const char *snapshot_id,
> +                          Error **errp)
>  {
>      BlockDriver *drv = bs->drv;
> -    if (!drv)
> -        return -ENOMEDIUM;
> -    if (drv->bdrv_snapshot_delete)
> -        return drv->bdrv_snapshot_delete(bs, snapshot_id);
> -    if (bs->file)
> -        return bdrv_snapshot_delete(bs->file, snapshot_id);
> -    return -ENOTSUP;
> +
> +    if (!drv) {
> +        error_setg(errp, "device '%s' has no medium", bdrv_get_device_name(bs));

I don't think the device name is useful here. It's always the device
that the user has specified in the monitor command.

Also, please start error messages with a capital letter. (This applies
to the whole patch, and probably also other patches in this series)

> +    } else if (drv->bdrv_snapshot_delete) {
> +        drv->bdrv_snapshot_delete(bs, snapshot_id, errp);
> +    } else if (bs->file) {
> +        bdrv_snapshot_delete(bs->file, snapshot_id, errp);
> +    } else {
> +        error_setg(errp, "snapshots are not supported on device '%s'",
> +                   bdrv_get_device_name(bs));

Same thing with the device name here.

> +    }
>  }
>  
>  int bdrv_snapshot_list(BlockDriverState *bs,
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 992a5c8..2ebeadc 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -530,7 +530,9 @@ fail:
>      return ret;
>  }
>  
> -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> +void qcow2_snapshot_delete(BlockDriverState *bs,
> +                           const char *snapshot_id,
> +                           Error **errp)
>  {
>      BDRVQcowState *s = bs->opaque;
>      QCowSnapshot sn;
> @@ -539,7 +541,9 @@ int qcow2_snapshot_delete(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];
>  
> @@ -550,7 +554,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
>      s->nb_snapshots--;
>      ret = qcow2_write_snapshots(bs);
>      if (ret < 0) {
> -        return ret;
> +        error_setg_errno(errp, -ret, "qcow2: failed to remove snapshot '%s' "
> +                         "from snapshot list", sn.name);
> +        return;
>      }
>  
>      /*
> @@ -567,14 +573,18 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
>      ret = qcow2_update_snapshot_refcount(bs, sn.l1_table_offset,
>                                           sn.l1_size, -1);
>      if (ret < 0) {
> -        return ret;
> +        error_setg_errno(errp, -ret,
> +                         "qcow2: failed to update snapshot refcount");

I'd make it "qcow2: Failed to update refcounts". It's the refcounts of
all clusters referred to by the snapshot's L1 table, not of the snapshot
itself.

> +        return;
>      }
>      qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * sizeof(uint64_t));
>  
>      /* must update the copied flag on the current cluster offsets */
>      ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 0);
>      if (ret < 0) {
> -        return ret;
> +        error_setg_errno(errp, -ret,
> +                         "qcow2: failed to update snapshot refcount");

"qcow2: Failed to update cluster flags"

> +        return;
>      }
>  
>  #ifdef DEBUG_ALLOC
> @@ -583,7 +593,6 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
>          qcow2_check_refcounts(bs, &result, 0);
>      }
>  #endif
> -    return 0;
>  }
>  
>  int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 9421843..dbd332d 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -384,7 +384,9 @@ 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);
> -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
> +void qcow2_snapshot_delete(BlockDriverState *bs,
> +                           const char *snapshot_id,
> +                           Error **errp);
>  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 141b488..c10edbf 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -871,14 +871,19 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
>      return 0;
>  }
>  
> -static int qemu_rbd_snap_remove(BlockDriverState *bs,
> -                                const char *snapshot_name)
> +static void qemu_rbd_snap_remove(BlockDriverState *bs,
> +                                 const char *snapshot_name,
> +                                 Error **errp)
>  {
>      BDRVRBDState *s = bs->opaque;
>      int r;
>  
>      r = rbd_snap_remove(s->image, snapshot_name);
> -    return r;
> +    if (r < 0) {
> +        error_setg_errno(errp, -r, "rbd: failed to remove snapshot '%s' on "
> +                         "device '%s'", snapshot_name,
> +                         bdrv_get_device_name(bs));

Remove the device name. You didn't have it in the qcow2 errors either.

> +    }
>  }
>  
>  static int qemu_rbd_snap_rollback(BlockDriverState *bs,
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 1c5b532..270fa64 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1908,10 +1908,12 @@ out:
>      return ret;
>  }
>  
> -static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> +static void sd_snapshot_delete(BlockDriverState *bs,
> +                               const char *snapshot_id,
> +                               Error **errp)
>  {
>      /* FIXME: Delete specified snapshot id.  */
> -    return 0;
> +    return;
>  }

Looks like there should be some error_setg("Deleting snapshots not
supported yet by sheepdog"), possibly in a separate patch.

>  static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
> diff --git a/include/block/block.h b/include/block/block.h
> index ebd9512..db8c6aa 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -337,7 +337,9 @@ int bdrv_snapshot_create(BlockDriverState *bs,
>                           QEMUSnapshotInfo *sn_info);
>  int bdrv_snapshot_goto(BlockDriverState *bs,
>                         const char *snapshot_id);
> -int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
> +void bdrv_snapshot_delete(BlockDriverState *bs,
> +                          const char *snapshot_id,
> +                          Error **errp);
>  int bdrv_snapshot_list(BlockDriverState *bs,
>                         QEMUSnapshotInfo **psn_info);
>  int bdrv_snapshot_load_tmp(BlockDriverState *bs,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 458cde3..53c52bb 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -157,7 +157,9 @@ struct BlockDriver {
>                                  QEMUSnapshotInfo *sn_info);
>      int (*bdrv_snapshot_goto)(BlockDriverState *bs,
>                                const char *snapshot_id);
> -    int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id);
> +    void (*bdrv_snapshot_delete)(BlockDriverState *bs,
> +                                 const char *snapshot_id,
> +                                 Error **errp);
>      int (*bdrv_snapshot_list)(BlockDriverState *bs,
>                                QEMUSnapshotInfo **psn_info);
>      int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
> diff --git a/qemu-img.c b/qemu-img.c
> index dbacdb7..4828fe4 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1936,6 +1936,7 @@ static int img_snapshot(int argc, char **argv)
>  {
>      BlockDriverState *bs;
>      QEMUSnapshotInfo sn;
> +    Error *local_err = NULL;
>      char *filename, *snapshot_name = NULL;
>      int c, ret = 0, bdrv_oflags;
>      int action = 0;
> @@ -2033,11 +2034,9 @@ static int img_snapshot(int argc, char **argv)
>          break;
>  
>      case SNAPSHOT_DELETE:
> -        ret = bdrv_snapshot_delete(bs, snapshot_name);
> -        if (ret) {
> -            error_report("Could not delete snapshot '%s': %d (%s)",
> -                snapshot_name, ret, strerror(-ret));
> -        }
> +        bdrv_snapshot_delete(bs, snapshot_name, &local_err);
> +        ret = qemu_img_handle_error("qemu-img snapshot delete failed",
> +                                    local_err);

Here again "qemu-img snapshot delete failed" is redundant. If this is
the command that I typed and I get an error, then it's obvious that it's
this command that failed.

>          break;
>      }
>  
> diff --git a/savevm.c b/savevm.c
> index 53515cb..6af84fd 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2225,18 +2225,17 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
>  {
>      BlockDriverState *bs;
>      QEMUSnapshotInfo sn1, *snapshot = &sn1;
> -    int ret;
> +    Error *local_err = NULL;
>  
>      bs = NULL;
>      while ((bs = bdrv_next(bs))) {
>          if (bdrv_can_snapshot(bs) &&
>              bdrv_snapshot_find(bs, snapshot, name) >= 0)
>          {
> -            ret = bdrv_snapshot_delete(bs, name);
> -            if (ret < 0) {
> -                monitor_printf(mon,
> -                               "Error while deleting snapshot on '%s'\n",
> -                               bdrv_get_device_name(bs));
> +            bdrv_snapshot_delete(bs, name, &local_err);
> +            if (error_is_set(&local_err)) {
> +                monitor_printf(mon, "%s\n", error_get_pretty(local_err));

Here the additional monitor_printf() actually had meaningful additional
information. Deleting an old snapshot is an implicitly taken action and
not explicitly requested, so an error message should indicate that it
happened during the deletion. Maybe something like:

qerror_report(ERROR_CLASS_GENERIC_ERROR,
              "Error while deleting old snapshot on device '%s': %s",
              bdrv_get_device_name(bs), error_get_pretty(local_err));

> +                error_free(local_err);
>                  return -1;
>              }
>          }
> @@ -2450,7 +2449,7 @@ int load_vmstate(const char *name)
>  void do_delvm(Monitor *mon, const QDict *qdict)
>  {
>      BlockDriverState *bs, *bs1;
> -    int ret;
> +    Error *local_err = NULL;
>      const char *name = qdict_get_str(qdict, "name");
>  
>      bs = bdrv_snapshots();
> @@ -2462,15 +2461,10 @@ void do_delvm(Monitor *mon, const QDict *qdict)
>      bs1 = NULL;
>      while ((bs1 = bdrv_next(bs1))) {
>          if (bdrv_can_snapshot(bs1)) {
> -            ret = bdrv_snapshot_delete(bs1, name);
> -            if (ret < 0) {
> -                if (ret == -ENOTSUP)
> -                    monitor_printf(mon,
> -                                   "Snapshots not supported on device '%s'\n",
> -                                   bdrv_get_device_name(bs1));
> -                else
> -                    monitor_printf(mon, "Error %d while deleting snapshot on "
> -                                   "'%s'\n", ret, bdrv_get_device_name(bs1));
> +            bdrv_snapshot_delete(bs1, name, &local_err);
> +            if (error_is_set(&local_err)) {
> +                monitor_printf(mon, "%s\n", error_get_pretty(local_err));

Either something like above, indicating the device name on which
bdrv_snapshot_delete() failed, or qerror_report_err().

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

Kevin