Re: [Qemu-devel] [PATCH V5 2/8] snapshot: distinguish id and name in snapshot delete

[Kevin Wolf]

Am 11.07.2013 um 07:46 hat Wenchao Xia geschrieben:
> Snapshot creation actually already distinguish id and name since it take
> a structured parameter *sn, but delete can't. Later an accurate delete
> is needed in qmp_transaction abort and blockdev-snapshot-delete-sync,
> so change its prototype. Also *errp is added to tip error, but return
> value is kepted to let caller check what kind of error happens. Existing
> caller for it are savevm, delvm and qemu-img, they are not impacted by
> check the return value and do the operations again.
> 
> Before this patch:
>   For qcow2, it search id first then name to find the one to delete.
>   For rbd, it search name.
>   For sheepdog, it does nothing.
> 
> After this patch:
>   For qcow2, logic is the same by call it twice in caller.
>   For rbd, it always fails in delete with id, but still search for name
> in second try, no change for user.
> 
> Some code for *errp is based on Pavel's patch.
> 
> Signed-off-by: Wenchao Xia <address@hidden>
> Signed-off-by: Pavel Hrdina <address@hidden>
> ---
>  block/qcow2-snapshot.c    |   66 
> ++++++++++++++++++++++++++++++++++-----------
>  block/qcow2.h             |    5 +++-
>  block/rbd.c               |   23 +++++++++++++++-
>  block/sheepdog.c          |    5 +++-
>  block/snapshot.c          |   36 ++++++++++++++++++++++--
>  include/block/block_int.h |    5 +++-
>  include/block/snapshot.h  |    5 +++-
>  include/qemu-common.h     |    3 ++
>  qemu-img.c                |    5 +++-
>  savevm.c                  |   10 +++++-
>  10 files changed, 136 insertions(+), 27 deletions(-)

> @@ -531,15 +547,23 @@ fail:
>      return ret;
>  }
>  
> -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> +int qcow2_snapshot_delete(BlockDriverState *bs,
> +                          const char *snapshot_id,
> +                          const char *name,
> +                          Error **errp)
>  {
>      BDRVQcowState *s = bs->opaque;
>      QCowSnapshot sn;
>      int snapshot_index, ret;
> +    const char *device = bdrv_get_device_name(bs);
>  
>      /* Search the snapshot */
> -    snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
> +    snapshot_index = find_snapshot_by_id_and_name(bs, snapshot_id, name);
>      if (snapshot_index < 0) {
> +        error_setg(errp,
> +                   "Can't find a snapshot with ID '%s' and name '%s' "
> +                   "on device '%s'",
> +                   STR_PRINT_CHAR(snapshot_id), STR_PRINT_CHAR(name), 
> device);

At least the "on device '%s'" part should be removed from the error
messages. It is something the caller can add if there is any ambiguity.

> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1883,7 +1883,10 @@ static int img_snapshot(int argc, char **argv)
>          break;
>  
>      case SNAPSHOT_DELETE:
> -        ret = bdrv_snapshot_delete(bs, snapshot_name);
> +        ret = bdrv_snapshot_delete(bs, snapshot_name, NULL, NULL);
> +        if (ret == -ENOENT || ret == -EINVAL) {
> +            ret = bdrv_snapshot_delete(bs, NULL, snapshot_name, NULL);
> +        }
>          if (ret) {
>              error_report("Could not delete snapshot '%s': %d (%s)",
>                  snapshot_name, ret, strerror(-ret));

Why don't you use the new error messages?

> diff --git a/savevm.c b/savevm.c
> index e0491e7..56afebb 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2332,7 +2332,10 @@ static int del_existing_snapshots(Monitor *mon, const 
> char *name)
>          if (bdrv_can_snapshot(bs) &&
>              bdrv_snapshot_find(bs, snapshot, name) >= 0)
>          {
> -            ret = bdrv_snapshot_delete(bs, name);
> +            ret = bdrv_snapshot_delete(bs, name, NULL, NULL);
> +            if (ret == -ENOENT || ret == -EINVAL) {
> +                ret = bdrv_snapshot_delete(bs, NULL, name, NULL);
> +            }
>              if (ret < 0) {
>                  monitor_printf(mon,
>                                 "Error while deleting snapshot on '%s'\n",
> @@ -2562,7 +2565,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);
> +            ret = bdrv_snapshot_delete(bs1, name, NULL, NULL);
> +            if (ret == -ENOENT || ret == -EINVAL) {
> +                ret = bdrv_snapshot_delete(bs, NULL, name, NULL);
> +            }
>              if (ret < 0) {
>                  if (ret == -ENOTSUP)
>                      monitor_printf(mon,

Same thing here.

Kevin