[Qemu-devel] [PATCH V8 5/8] qcow2: full rollback on fail in qcow2_write_snapshots()

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

Header restore step is added, and old code section "fail" is
renamed to "dealloc_sn_table" to tip better, now "fail" section
does not rollback anything on disk. When one step in rollback
fails, following steps will be skipped, to make sure no dangling
pointer is left.

A new parameter "*errp_rollback" is added to tell caller the result
of rollback procedure.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qcow2-snapshot.c |   63 +++++++++++++++++++++++++++++++++++------------
 1 files changed, 47 insertions(+), 16 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 5619fc3..5cac714 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -152,7 +152,9 @@ fail:
 }
 
 /* add at the end of the file a new list of snapshots */
-static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
+static int qcow2_write_snapshots(BlockDriverState *bs,
+                                 Error **errp,
+                                 Error **errp_rollback)
 {
     BDRVQcowState *s = bs->opaque;
     QCowSnapshot *sn;
@@ -162,9 +164,9 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
     struct {
         uint32_t nb_snapshots;
         uint64_t snapshots_offset;
-    } QEMU_PACKED header_data;
+    } QEMU_PACKED header_data, header_data_old;
     int64_t offset, snapshots_offset;
-    int ret;
+    int ret, ret0;
 
     /* compute the size of the snapshots */
     offset = 0;
@@ -192,7 +194,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
         error_setg_errno(errp, -ret,
                          "Failed in flush after snapshot list cluster "
                          "allocation");
-        goto fail;
+        goto dealloc_sn_table;
     }
 
     /* The snapshot list position has not yet been updated, so these clusters
@@ -203,7 +205,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
                          "Failed in overlap check for snapshot list cluster "
                          "at %" PRIi64 " with size %d",
                          offset, snapshots_size);
-        goto fail;
+        goto dealloc_sn_table;
     }
 
 
@@ -240,7 +242,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
                              "Failed in write of snapshot header at %"
                              PRIi64 " with size %zu",
                              offset, sizeof(h));
-            goto fail;
+            goto dealloc_sn_table;
         }
         offset += sizeof(h);
 
@@ -250,7 +252,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
                              "Failed in write of extra snapshot data at %"
                              PRIi64 " with size %zu",
                              offset, sizeof(extra));
-            goto fail;
+            goto dealloc_sn_table;
         }
         offset += sizeof(extra);
 
@@ -260,7 +262,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
                              "Failed in write of snapshot id string at %"
                              PRIi64 " with size %d",
                              offset, id_str_size);
-            goto fail;
+            goto dealloc_sn_table;
         }
         offset += id_str_size;
 
@@ -270,7 +272,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
                              "Failed in write of snapshot name string at %"
                              PRIi64 " with size %d",
                              offset, name_size);
-            goto fail;
+            goto dealloc_sn_table;
         }
         offset += name_size;
     }
@@ -283,7 +285,18 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
     if (ret < 0) {
         error_setg_errno(errp, -ret,
                          "Failed in flush after snapshot list update");
-        goto fail;
+        goto dealloc_sn_table;
+    }
+
+    /* Start the qcow2 header update */
+    ret = bdrv_pread(bs->file, offsetof(QCowHeader, nb_snapshots),
+                     &header_data_old, sizeof(header_data_old));
+    if (ret < 0) {
+        error_setg_errno(errp, -ret,
+                         "Failed in read of image header at %zu with size %zu",
+                         offsetof(QCowHeader, nb_snapshots),
+                         sizeof(header_data_old));
+        goto dealloc_sn_table;
     }
 
     QEMU_BUILD_BUG_ON(offsetof(QCowHeader, snapshots_offset) !=
@@ -299,7 +312,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
                          "Failed in update of image header at %zu with size %zu",
                          offsetof(QCowHeader, nb_snapshots),
                          sizeof(header_data));
-        goto fail;
+        goto restore_header;
     }
 
     /* free the old snapshot table */
@@ -309,11 +322,29 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
     s->snapshots_size = snapshots_size;
     return 0;
 
-fail:
+restore_header:
+    ret0 = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, nb_snapshots),
+                            &header_data_old, sizeof(header_data_old));
+    if (ret0 < 0) {
+        error_setg_errno(errp_rollback, -ret0,
+                         "In rollback failed to restore image header at %zu "
+                         "with size %zu",
+                         offsetof(QCowHeader, nb_snapshots),
+                         sizeof(header_data));
+        goto fail;
+    }
+
+dealloc_sn_table:
     if (snapshots_offset > 0) {
-        qcow2_free_clusters(bs, snapshots_offset, snapshots_size,
-                            QCOW2_DISCARD_ALWAYS);
+        ret0 = qcow2_free_clusters(bs, snapshots_offset, snapshots_size,
+                                   QCOW2_DISCARD_ALWAYS);
+        if (ret0 < 0) {
+            error_setg_errno(errp_rollback, -ret0,
+                             "In rollback failed to free snapshot table");
+        }
     }
+
+fail:
     if (errp) {
         g_assert(error_is_set(errp));
     }
@@ -481,7 +512,7 @@ void qcow2_snapshot_create(BlockDriverState *bs,
     s->snapshots = new_snapshot_list;
     s->snapshots[s->nb_snapshots++] = *sn;
 
-    ret = qcow2_write_snapshots(bs, errp);
+    ret = qcow2_write_snapshots(bs, errp, NULL);
     if (ret < 0) {
         g_free(s->snapshots);
         s->snapshots = old_snapshot_list;
@@ -656,7 +687,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs,
             s->snapshots + snapshot_index + 1,
             (s->nb_snapshots - snapshot_index - 1) * sizeof(sn));
     s->nb_snapshots--;
-    ret = qcow2_write_snapshots(bs, errp);
+    ret = qcow2_write_snapshots(bs, errp, NULL);
     if (ret < 0) {
         return ret;
     }
-- 
1.7.1