[Qemu-devel] [PATCH V2 06/12] snapshot: distinguish id and name in snapshot delete

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

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 <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 block/qcow2-snapshot.c    |   65 ++++++++++++++++++++++++++++++++++-----------
 block/qcow2.h             |    5 +++-
 block/rbd.c               |   19 ++++++++++++-
 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, 131 insertions(+), 27 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 7108d46..4aeb625 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -287,31 +287,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 */
@@ -333,7 +349,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;
     }
 
@@ -530,15 +546,22 @@ 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];
@@ -550,6 +573,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;
     }
 
@@ -567,6 +594,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));
@@ -574,6 +604,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 6959c6a..5e39f78 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -385,7 +385,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 0f2608b..c0ac1b0 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -903,12 +903,29 @@ 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;
 
+    if (snapshot_id && snapshot_id[0] != '\0') {
+        error_setg(errp, "rbd do not support snapshot id");
+        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");
+        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, bdrv_get_device_name(bs));
+    }
     return r;
 }
 
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 21a4edf..972d59b 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2092,7 +2092,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 ba52247..532acef 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -157,7 +157,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 ed8b6e2..a20fdc4 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -204,6 +204,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 809b4f1..b660b39 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 2ce439f..91c36df 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2289,7 +2289,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",
@@ -2519,7 +2522,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