[Qemu-devel] [PATCH V4 3/9] snapshot: distinguish id and name in snapshot delete

[Wenchao Xia]

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(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 4f9c524..e8a8e15 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -288,31 +288,47 @@ static void find_new_snapshot_id(BlockDriverState *bs,
     snapshot_id_string_generate(id_max + 1, id_str, id_str_size);
 }
 
-static int find_snapshot_by_id(BlockDriverState *bs, const char *id_str)
+static int find_snapshot_by_id_and_name(BlockDriverState *bs,
+                                        const char *id,
+                                        const char *name)
 {
     BDRVQcowState *s = bs->opaque;
     int i;
 
-    for(i = 0; i < s->nb_snapshots; i++) {
-        if (!strcmp(s->snapshots[i].id_str, id_str))
-            return i;
+    if (id && name) {
+        for (i = 0; i < s->nb_snapshots; i++) {
+            if (!strcmp(s->snapshots[i].id_str, id) &&
+                !strcmp(s->snapshots[i].name, name)) {
+                return i;
+            }
+        }
+    } else if (id) {
+        for (i = 0; i < s->nb_snapshots; i++) {
+            if (!strcmp(s->snapshots[i].id_str, id)) {
+                return i;
+            }
+        }
+    } else if (name) {
+        for (i = 0; i < s->nb_snapshots; i++) {
+            if (!strcmp(s->snapshots[i].name, name)) {
+                return i;
+            }
+        }
     }
+
     return -1;
 }
 
-static int find_snapshot_by_id_or_name(BlockDriverState *bs, const char *name)
+static int find_snapshot_by_id_or_name(BlockDriverState *bs,
+                                       const char *id_or_name)
 {
-    BDRVQcowState *s = bs->opaque;
-    int i, ret;
+    int ret;
 
-    ret = find_snapshot_by_id(bs, name);
-    if (ret >= 0)
+    ret = find_snapshot_by_id_and_name(bs, id_or_name, NULL);
+    if (ret >= 0) {
         return ret;
-    for(i = 0; i < s->nb_snapshots; i++) {
-        if (!strcmp(s->snapshots[i].name, name))
-            return i;
     }
-    return -1;
+    return find_snapshot_by_id_and_name(bs, NULL, id_or_name);
 }
 
 /* if no id is provided, a new one is constructed */
@@ -334,7 +350,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, 
QEMUSnapshotInfo *sn_info)
     }
 
     /* Check that the ID is unique */
-    if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
+    if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) {
         return -EEXIST;
     }
 
@@ -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);
         return -ENOENT;
     }
     sn = s->snapshots[snapshot_index];
@@ -551,6 +575,10 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char 
*snapshot_id)
     s->nb_snapshots--;
     ret = qcow2_write_snapshots(bs);
     if (ret < 0) {
+        error_setg(errp,
+                   "Failed to remove snapshot with ID '%s' and name '%s' from "
+                   "the snapshot list on device '%s'",
+                   STR_PRINT_CHAR(snapshot_id), STR_PRINT_CHAR(name), device);
         return ret;
     }
 
@@ -568,6 +596,9 @@ 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) {
+        error_setg(errp,
+                   "Failed to free the cluster and L1 table on device '%s'",
+                   device);
         return ret;
     }
     qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * sizeof(uint64_t),
@@ -576,6 +607,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char 
*snapshot_id)
     /* 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) {
+        error_setg(errp,
+                   "Failed to update snapshot status in disk on device '%s'",
+                   device);
         return ret;
     }
 
diff --git a/block/qcow2.h b/block/qcow2.h
index 3b2d5cd..2e6c471 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -411,7 +411,10 @@ 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);
+int qcow2_snapshot_delete(BlockDriverState *bs,
+                          const char *snapshot_id,
+                          const char *name,
+                          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 cb71751..688b571 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -903,12 +903,33 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
 }
 
 static int qemu_rbd_snap_remove(BlockDriverState *bs,
-                                const char *snapshot_name)
+                                const char *snapshot_id,
+                                const char *snapshot_name,
+                                Error **errp)
 {
     BDRVRBDState *s = bs->opaque;
     int r;
+    const char *device = bdrv_get_device_name(bs);
+
+    if (snapshot_id && snapshot_id[0] != '\0') {
+        error_setg(errp,
+                   "rbd do not support snapshot id on device '%s'", device);
+        return -EINVAL;
+    }
+    /* then snapshot_id == NULL or it contain an empty line, ignore it */
+
+    if (!snapshot_name) {
+        error_setg(errp,
+                   "rbd need a valid snapshot name on device '%s'", device);
+        return -EINVAL;
+    }
 
     r = rbd_snap_remove(s->image, snapshot_name);
+    if (r < 0) {
+        error_setg_errno(errp, -r,
+                         "Failed to remove snapshot '%s' on device '%s'",
+                         snapshot_name, device);
+    }
     return r;
 }
 
diff --git a/block/sheepdog.c b/block/sheepdog.c
index b397b5b..7fdae71 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2089,7 +2089,10 @@ out:
     return ret;
 }
 
-static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+static int sd_snapshot_delete(BlockDriverState *bs,
+                              const char *snapshot_id,
+                              const char *name,
+                              Error **errp)
 {
     /* FIXME: Delete specified snapshot id.  */
     return 0;
diff --git a/block/snapshot.c b/block/snapshot.c
index 40e0cc7..707b42d 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -182,18 +182,48 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
-int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+/**
+ * Delete an internal snapshot by @snapshot_id and @name.
+ * @bs: block device used in the operation
+ * @snapshot_id: unique snapshot ID, or NULL
+ * @name: snapshot name, or NULL
+ * @errp: location to store error
+ *
+ * If both @snapshot_id and @name are specified, delete the first one with
+ * id @snapshot_id and name @name.
+ * If only @snapshot_id is specified, delete the first one with id
+ * @snapshot_id.
+ * If only @name is specified, delete the first one with name @name.
+ * if none is specified, return -ENINVAL.
+ *
+ * Returns: 0 on success, -errno on fail. If @bs is not inserted, return
+ * -ENOMEDIUM. If can't find one matching @id and @name, return -ENOENT.
+ * If @bs did not support internal snapshot, return -ENOTSUP. If @bs do not
+ * support parameter @snapshot_id or @name, return -EINVAL.
+ */
+int bdrv_snapshot_delete(BlockDriverState *bs,
+                         const char *snapshot_id,
+                         const char *name,
+                         Error **errp)
 {
     BlockDriver *drv = bs->drv;
     if (!drv) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
         return -ENOMEDIUM;
     }
+    if (!snapshot_id && !name) {
+        error_setg(errp, "snapshot_id and name are both NULL");
+        return -EINVAL;
+    }
     if (drv->bdrv_snapshot_delete) {
-        return drv->bdrv_snapshot_delete(bs, snapshot_id);
+        return drv->bdrv_snapshot_delete(bs, snapshot_id, name, errp);
     }
     if (bs->file) {
-        return bdrv_snapshot_delete(bs->file, snapshot_id);
+        return bdrv_snapshot_delete(bs->file, snapshot_id, name, errp);
     }
+    error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+              drv->format_name, bdrv_get_device_name(bs),
+              "internal snapshot");
     return -ENOTSUP;
 }
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c6ac871..4499b8b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -166,7 +166,10 @@ 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);
+    int (*bdrv_snapshot_delete)(BlockDriverState *bs,
+                                const char *snapshot_id,
+                                const char *name,
+                                Error **errp);
     int (*bdrv_snapshot_list)(BlockDriverState *bs,
                               QEMUSnapshotInfo **psn_info);
     int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 3d93719..fbc03c0 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -51,7 +51,10 @@ 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);
+int bdrv_snapshot_delete(BlockDriverState *bs,
+                         const char *snapshot_id,
+                         const char *name,
+                         Error **errp);
 int bdrv_snapshot_list(BlockDriverState *bs,
                        QEMUSnapshotInfo **psn_info);
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
diff --git a/include/qemu-common.h b/include/qemu-common.h
index f439738..06c777f 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -191,6 +191,9 @@ int64_t strtosz_suffix(const char *nptr, char **end, const 
char default_suffix);
 int64_t strtosz_suffix_unit(const char *nptr, char **end,
                             const char default_suffix, int64_t unit);
 
+/* used to print char* safely */
+#define STR_PRINT_CHAR(str) ((str) ? (str) : "null")
+
 /* path.c */
 void init_paths(const char *prefix);
 const char *path(const char *pathname);
diff --git a/qemu-img.c b/qemu-img.c
index f8c97d3..05564d3 100644
--- 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));
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,
-- 
1.7.1