[Qemu-devel] [PATCH v2 0/9] qcow2: Fix error paths for internal snapshots

[Qemu-devel] [PATCH v2 0/9] qcow2: Fix error paths for internal snapshots

[Kevin Wolf]

Kevin Wolf (9):
  qcow2: Return real error code in qcow2_read_snapshots
  qcow2: Return real error code in qcow2_write_snapshots
  qcow2: Update snapshot table information at once
  qcow2: Cleanups and memleak fix in qcow2_snapshot_create
  qcow2: Rework qcow2_snapshot_create error handling
  qcow2: Return real error in qcow2_snapshot_goto
  qcow2: Fix order of refcount updates in qcow2_snapshot_goto
  qcow2: Fix order in qcow2_snapshot_delete
  qcow2: Fix error path in qcow2_snapshot_load_tmp

 block/qcow2-refcount.c |    7 +-
 block/qcow2-snapshot.c |  330 +++++++++++++++++++++++++++++++++++-------------
 block/qcow2.c          |    5 +-
 3 files changed, 248 insertions(+), 94 deletions(-)

-- 
1.7.6.4

[Qemu-devel] [PATCH v2 1/9] qcow2: Return real error code in qcow2_read_snapshots

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-snapshot.c |   25 ++++++++++++++++++++-----
 block/qcow2.c          |    5 +++--
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index bdc33ba..4134bbc 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -68,6 +68,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
     int i, id_str_size, name_size;
     int64_t offset;
     uint32_t extra_data_size;
+    int ret;
 
     if (!s->nb_snapshots) {
         s->snapshots = NULL;
@@ -77,10 +78,15 @@ int qcow2_read_snapshots(BlockDriverState *bs)
 
     offset = s->snapshots_offset;
     s->snapshots = g_malloc0(s->nb_snapshots * sizeof(QCowSnapshot));
+
     for(i = 0; i < s->nb_snapshots; i++) {
+        /* Read statically sized part of the snapshot header */
         offset = align_offset(offset, 8);
-        if (bdrv_pread(bs->file, offset, &h, sizeof(h)) != sizeof(h))
+        ret = bdrv_pread(bs->file, offset, &h, sizeof(h));
+        if (ret < 0) {
             goto fail;
+        }
+
         offset += sizeof(h);
         sn = s->snapshots + i;
         sn->l1_table_offset = be64_to_cpu(h.l1_table_offset);
@@ -94,25 +100,34 @@ int qcow2_read_snapshots(BlockDriverState *bs)
         id_str_size = be16_to_cpu(h.id_str_size);
         name_size = be16_to_cpu(h.name_size);
 
+        /* Skip extra data */
         offset += extra_data_size;
 
+        /* Read snapshot ID */
         sn->id_str = g_malloc(id_str_size + 1);
-        if (bdrv_pread(bs->file, offset, sn->id_str, id_str_size) != id_str_size)
+        ret = bdrv_pread(bs->file, offset, sn->id_str, id_str_size);
+        if (ret < 0) {
             goto fail;
+        }
         offset += id_str_size;
         sn->id_str[id_str_size] = '\0';
 
+        /* Read snapshot name */
         sn->name = g_malloc(name_size + 1);
-        if (bdrv_pread(bs->file, offset, sn->name, name_size) != name_size)
+        ret = bdrv_pread(bs->file, offset, sn->name, name_size);
+        if (ret < 0) {
             goto fail;
+        }
         offset += name_size;
         sn->name[name_size] = '\0';
     }
+
     s->snapshots_size = offset - s->snapshots_offset;
     return 0;
- fail:
+
+fail:
     qcow2_free_snapshots(bs);
-    return -1;
+    return ret;
 }
 
 /* add at the end of the file a new list of snapshots */
diff --git a/block/qcow2.c b/block/qcow2.c
index a56b011..27cbbeb 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -272,8 +272,9 @@ static int qcow2_open(BlockDriverState *bs, int flags)
         }
         bs->backing_file[len] = '\0';
     }
-    if (qcow2_read_snapshots(bs) < 0) {
-        ret = -EINVAL;
+
+    ret = qcow2_read_snapshots(bs);
+    if (ret < 0) {
         goto fail;
     }
 
-- 
1.7.6.4

[Qemu-devel] [PATCH v2 2/9] qcow2: Return real error code in qcow2_write_snapshots

[Kevin Wolf]

Doesn't immediately fix anything as the callers don't use the return
value, but they will be fixed next.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-snapshot.c |   48 ++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 4134bbc..e91a286 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -140,6 +140,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
     uint64_t data64;
     uint32_t data32;
     int64_t offset, snapshots_offset;
+    int ret;
 
     /* compute the size of the snapshots */
     offset = 0;
@@ -152,6 +153,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
     }
     snapshots_size = offset;
 
+    /* Allocate space for the new snapshot list */
     snapshots_offset = qcow2_alloc_clusters(bs, snapshots_size);
     bdrv_flush(bs->file);
     offset = snapshots_offset;
@@ -159,6 +161,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
         return offset;
     }
 
+    /* Write all snapshots to the new list */
     for(i = 0; i < s->nb_snapshots; i++) {
         sn = s->snapshots + i;
         memset(&h, 0, sizeof(h));
@@ -174,34 +177,59 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
         h.id_str_size = cpu_to_be16(id_str_size);
         h.name_size = cpu_to_be16(name_size);
         offset = align_offset(offset, 8);
-        if (bdrv_pwrite_sync(bs->file, offset, &h, sizeof(h)) < 0)
+
+        ret = bdrv_pwrite(bs->file, offset, &h, sizeof(h));
+        if (ret < 0) {
             goto fail;
+        }
         offset += sizeof(h);
-        if (bdrv_pwrite_sync(bs->file, offset, sn->id_str, id_str_size) < 0)
+
+        ret = bdrv_pwrite(bs->file, offset, sn->id_str, id_str_size);
+        if (ret < 0) {
             goto fail;
+        }
         offset += id_str_size;
-        if (bdrv_pwrite_sync(bs->file, offset, sn->name, name_size) < 0)
+
+        ret = bdrv_pwrite(bs->file, offset, sn->name, name_size);
+        if (ret < 0) {
             goto fail;
+        }
         offset += name_size;
     }
 
-    /* update the various header fields */
+    /*
+     * Update the header to point to the new snapshot table. This requires the
+     * new table and its refcounts to be stable on disk.
+     *
+     * FIXME This should be done with a single write
+     */
+    ret = bdrv_flush(bs);
+    if (ret < 0) {
+        goto fail;
+    }
+
     data64 = cpu_to_be64(snapshots_offset);
-    if (bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, snapshots_offset),
-                    &data64, sizeof(data64)) < 0)
+    ret = bdrv_pwrite(bs->file, offsetof(QCowHeader, snapshots_offset),
+                      &data64, sizeof(data64));
+    if (ret < 0) {
         goto fail;
+    }
+
     data32 = cpu_to_be32(s->nb_snapshots);
-    if (bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, nb_snapshots),
-                    &data32, sizeof(data32)) < 0)
+    ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, nb_snapshots),
+                           &data32, sizeof(data32));
+    if (ret < 0) {
         goto fail;
+    }
 
     /* free the old snapshot table */
     qcow2_free_clusters(bs, s->snapshots_offset, s->snapshots_size);
     s->snapshots_offset = snapshots_offset;
     s->snapshots_size = snapshots_size;
     return 0;
- fail:
-    return -1;
+
+fail:
+    return ret;
 }
 
 static void find_new_snapshot_id(BlockDriverState *bs,
-- 
1.7.6.4

[Qemu-devel] [PATCH v2 3/9] qcow2: Update snapshot table information at once

[Kevin Wolf]

Failing in the middle wouldn't help with the integrity of the image, so
doing everything in a single request seems better.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-snapshot.c |   22 ++++++++++------------
 1 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index e91a286..1976df7 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -137,8 +137,10 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
     QCowSnapshot *sn;
     QCowSnapshotHeader h;
     int i, name_size, id_str_size, snapshots_size;
-    uint64_t data64;
-    uint32_t data32;
+    struct {
+        uint32_t nb_snapshots;
+        uint64_t snapshots_offset;
+    } QEMU_PACKED header_data;
     int64_t offset, snapshots_offset;
     int ret;
 
@@ -200,24 +202,20 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
     /*
      * Update the header to point to the new snapshot table. This requires the
      * new table and its refcounts to be stable on disk.
-     *
-     * FIXME This should be done with a single write
      */
     ret = bdrv_flush(bs);
     if (ret < 0) {
         goto fail;
     }
 
-    data64 = cpu_to_be64(snapshots_offset);
-    ret = bdrv_pwrite(bs->file, offsetof(QCowHeader, snapshots_offset),
-                      &data64, sizeof(data64));
-    if (ret < 0) {
-        goto fail;
-    }
+    QEMU_BUILD_BUG_ON(offsetof(QCowHeader, snapshots_offset) !=
+        offsetof(QCowHeader, nb_snapshots) + sizeof(header_data.nb_snapshots));
+
+    header_data.nb_snapshots        = cpu_to_be32(s->nb_snapshots);
+    header_data.snapshots_offset    = cpu_to_be64(snapshots_offset);
 
-    data32 = cpu_to_be32(s->nb_snapshots);
     ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, nb_snapshots),
-                           &data32, sizeof(data32));
+                           &header_data, sizeof(header_data));
     if (ret < 0) {
         goto fail;
     }
-- 
1.7.6.4

[Qemu-devel] [PATCH v2 4/9] qcow2: Cleanups and memleak fix in qcow2_snapshot_create

[Kevin Wolf]

sn->id_str could be leaked before this. The rest of this patch changes
comments, fixes coding style or removes checks that are unnecessary with
g_malloc.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-snapshot.c |   26 +++++++++++---------------
 1 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 1976df7..53d9233 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -284,21 +284,20 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
 
     memset(sn, 0, sizeof(*sn));
 
+    /* Generate an ID if it wasn't passed */
     if (sn_info->id_str[0] == '\0') {
-        /* compute a new id */
         find_new_snapshot_id(bs, sn_info->id_str, sizeof(sn_info->id_str));
     }
 
-    /* check that the ID is unique */
-    if (find_snapshot_by_id(bs, sn_info->id_str) >= 0)
+    /* Check that the ID is unique */
+    if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
         return -ENOENT;
+    }
 
+    /* Populate sn with passed data */
     sn->id_str = g_strdup(sn_info->id_str);
-    if (!sn->id_str)
-        goto fail;
     sn->name = g_strdup(sn_info->name);
-    if (!sn->name)
-        goto fail;
+
     sn->vm_state_size = sn_info->vm_state_size;
     sn->date_sec = sn_info->date_sec;
     sn->date_nsec = sn_info->date_nsec;
@@ -308,7 +307,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     if (ret < 0)
         goto fail;
 
-    /* create the L1 table of the snapshot */
+    /* Allocate the L1 table of the snapshot and copy the current one there. */
     l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t));
     if (l1_table_offset < 0) {
         goto fail;
@@ -318,12 +317,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     sn->l1_table_offset = l1_table_offset;
     sn->l1_size = s->l1_size;
 
-    if (s->l1_size != 0) {
-        l1_table = g_malloc(s->l1_size * sizeof(uint64_t));
-    } else {
-        l1_table = NULL;
-    }
-
+    l1_table = g_malloc(s->l1_size * sizeof(uint64_t));
     for(i = 0; i < s->l1_size; i++) {
         l1_table[i] = cpu_to_be64(s->l1_table[i]);
     }
@@ -350,7 +344,9 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     }
 #endif
     return 0;
- fail:
+
+fail:
+    g_free(sn->id_str);
     g_free(sn->name);
     g_free(l1_table);
     return -1;
-- 
1.7.6.4

[Qemu-devel] [PATCH v2 5/9] qcow2: Rework qcow2_snapshot_create error handling

[Kevin Wolf]

Increase refcounts only after allocating a new L1 table has succeeded in
order to make leaks less likely. If writing the snapshot table fails,
revert in-memory state to be consistent with that on disk.

While at it, make it return the real error codes instead of -1.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-snapshot.c |   55 +++++++++++++++++++++++++++++++++++------------
 1 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 53d9233..b433f47 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -277,7 +277,9 @@ static int find_snapshot_by_id_or_name(BlockDriverState *bs, const char *name)
 int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
 {
     BDRVQcowState *s = bs->opaque;
-    QCowSnapshot *snapshots1, sn1, *sn = &sn1;
+    QCowSnapshot *new_snapshot_list = NULL;
+    QCowSnapshot *old_snapshot_list = NULL;
+    QCowSnapshot sn1, *sn = &sn1;
     int i, ret;
     uint64_t *l1_table = NULL;
     int64_t l1_table_offset;
@@ -303,16 +305,12 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     sn->date_nsec = sn_info->date_nsec;
     sn->vm_clock_nsec = sn_info->vm_clock_nsec;
 
-    ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1);
-    if (ret < 0)
-        goto fail;
-
     /* Allocate the L1 table of the snapshot and copy the current one there. */
     l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t));
     if (l1_table_offset < 0) {
+        ret = l1_table_offset;
         goto fail;
     }
-    bdrv_flush(bs->file);
 
     sn->l1_table_offset = l1_table_offset;
     sn->l1_size = s->l1_size;
@@ -321,22 +319,50 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     for(i = 0; i < s->l1_size; i++) {
         l1_table[i] = cpu_to_be64(s->l1_table[i]);
     }
-    if (bdrv_pwrite_sync(bs->file, sn->l1_table_offset,
-                    l1_table, s->l1_size * sizeof(uint64_t)) < 0)
+
+    ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table,
+                      s->l1_size * sizeof(uint64_t));
+    if (ret < 0) {
         goto fail;
+    }
+
     g_free(l1_table);
     l1_table = NULL;
 
-    snapshots1 = g_malloc((s->nb_snapshots + 1) * sizeof(QCowSnapshot));
+    /*
+     * Increase the refcounts of all clusters and make sure everything is
+     * stable on disk before updating the snapshot table to contain a pointer
+     * to the new L1 table.
+     */
+    ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    ret = bdrv_flush(bs->file);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    /* Append the new snapshot to the snapshot list */
+    new_snapshot_list = g_malloc((s->nb_snapshots + 1) * sizeof(QCowSnapshot));
     if (s->snapshots) {
-        memcpy(snapshots1, s->snapshots, s->nb_snapshots * sizeof(QCowSnapshot));
-        g_free(s->snapshots);
+        memcpy(new_snapshot_list, s->snapshots,
+               s->nb_snapshots * sizeof(QCowSnapshot));
+        old_snapshot_list = s->snapshots;
     }
-    s->snapshots = snapshots1;
+    s->snapshots = new_snapshot_list;
     s->snapshots[s->nb_snapshots++] = *sn;
 
-    if (qcow2_write_snapshots(bs) < 0)
+    ret = qcow2_write_snapshots(bs);
+    if (ret < 0) {
+        g_free(s->snapshots);
+        s->snapshots = old_snapshot_list;
         goto fail;
+    }
+
+    g_free(old_snapshot_list);
+
 #ifdef DEBUG_ALLOC
     {
       BdrvCheckResult result = {0};
@@ -349,7 +375,8 @@ fail:
     g_free(sn->id_str);
     g_free(sn->name);
     g_free(l1_table);
-    return -1;
+
+    return ret;
 }
 
 /* copy the snapshot 'snapshot_name' into the current disk image */
-- 
1.7.6.4

[Qemu-devel] [PATCH v2 6/9] qcow2: Return real error in qcow2_snapshot_goto

[Kevin Wolf]

Besides fixing the return code, this adds some comments that make clear
how the code works and that it potentially breaks images if we fail in
the wrong place. Actually fixing this is left for the next patch.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-snapshot.c |   51 +++++++++++++++++++++++++++++++++++++----------
 1 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index b433f47..38b4ae3 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -386,17 +386,32 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     QCowSnapshot *sn;
     int i, snapshot_index;
     int cur_l1_bytes, sn_l1_bytes;
+    int ret;
 
+    /* Search the snapshot */
     snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
-    if (snapshot_index < 0)
+    if (snapshot_index < 0) {
         return -ENOENT;
+    }
     sn = &s->snapshots[snapshot_index];
 
-    if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, -1) < 0)
+    /* Decrease refcount of clusters of current L1 table.
+     * FIXME This is too early! */
+    ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
+                                         s->l1_size, -1);
+    if (ret < 0) {
         goto fail;
+    }
 
-    if (qcow2_grow_l1_table(bs, sn->l1_size, true) < 0)
+    /*
+     * Make sure that the current L1 table is big enough to contain the whole
+     * L1 table of the snapshot. If the snapshot L1 table is smaller, the
+     * current one must be padded with zeros.
+     */
+    ret = qcow2_grow_l1_table(bs, sn->l1_size, true);
+    if (ret < 0) {
         goto fail;
+    }
 
     cur_l1_bytes = s->l1_size * sizeof(uint64_t);
     sn_l1_bytes = sn->l1_size * sizeof(uint64_t);
@@ -405,19 +420,32 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
         memset(s->l1_table + sn->l1_size, 0, cur_l1_bytes - sn_l1_bytes);
     }
 
-    /* copy the snapshot l1 table to the current l1 table */
-    if (bdrv_pread(bs->file, sn->l1_table_offset,
-                   s->l1_table, sn_l1_bytes) < 0)
+    /*
+     * Copy the snapshot L1 table to the current L1 table.
+     *
+     * Before overwriting the old current L1 table on disk, make sure to
+     * increase all refcounts for the clusters referenced by the new one.
+     */
+    ret = bdrv_pread(bs->file, sn->l1_table_offset, s->l1_table, sn_l1_bytes);
+    if (ret < 0) {
         goto fail;
-    if (bdrv_pwrite_sync(bs->file, s->l1_table_offset,
-                    s->l1_table, cur_l1_bytes) < 0)
+    }
+
+    ret = bdrv_pwrite_sync(bs->file, s->l1_table_offset, s->l1_table,
+                           cur_l1_bytes);
+    if (ret < 0) {
         goto fail;
+    }
+
     for(i = 0;i < s->l1_size; i++) {
         be64_to_cpus(&s->l1_table[i]);
     }
 
-    if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1) < 0)
+    /* FIXME This is too late! */
+    ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1);
+    if (ret < 0) {
         goto fail;
+    }
 
 #ifdef DEBUG_ALLOC
     {
@@ -426,8 +454,9 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     }
 #endif
     return 0;
- fail:
-    return -EIO;
+
+fail:
+    return ret;
 }
 
 int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
-- 
1.7.6.4

[Qemu-devel] [PATCH v2 7/9] qcow2: Fix order of refcount updates in qcow2_snapshot_goto

[Kevin Wolf]

The refcount updates must be moved so that in the worst case we can get
cluster leaks, but refcounts may never be too low.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-refcount.c |    7 ++++-
 block/qcow2-snapshot.c |   61 ++++++++++++++++++++++++++++++++++-------------
 2 files changed, 50 insertions(+), 18 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 9605367..2db2ede 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -700,6 +700,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
     l2_table = NULL;
     l1_table = NULL;
     l1_size2 = l1_size * sizeof(uint64_t);
+
+    /* WARNING: qcow2_snapshot_goto relies on this function not using the
+     * l1_table_offset when it is the current s->l1_table_offset! Be careful
+     * when changing this! */
     if (l1_table_offset != s->l1_table_offset) {
         if (l1_size2 != 0) {
             l1_table = g_malloc0(align_offset(l1_size2, 512));
@@ -819,7 +823,8 @@ fail:
     qcow2_cache_set_writethrough(bs, s->refcount_block_cache,
         old_refcount_writethrough);
 
-    if (l1_modified) {
+    /* Update L1 only if it isn't deleted anyway (addend = -1) */
+    if (addend >= 0 && l1_modified) {
         for(i = 0; i < l1_size; i++)
             cpu_to_be64s(&l1_table[i]);
         if (bdrv_pwrite_sync(bs->file, l1_table_offset, l1_table,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 38b4ae3..20cb66e 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -387,6 +387,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     int i, snapshot_index;
     int cur_l1_bytes, sn_l1_bytes;
     int ret;
+    uint64_t *sn_l1_table = NULL;
 
     /* Search the snapshot */
     snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
@@ -395,14 +396,6 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     }
     sn = &s->snapshots[snapshot_index];
 
-    /* Decrease refcount of clusters of current L1 table.
-     * FIXME This is too early! */
-    ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
-                                         s->l1_size, -1);
-    if (ret < 0) {
-        goto fail;
-    }
-
     /*
      * Make sure that the current L1 table is big enough to contain the whole
      * L1 table of the snapshot. If the snapshot L1 table is smaller, the
@@ -416,33 +409,66 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     cur_l1_bytes = s->l1_size * sizeof(uint64_t);
     sn_l1_bytes = sn->l1_size * sizeof(uint64_t);
 
-    if (cur_l1_bytes > sn_l1_bytes) {
-        memset(s->l1_table + sn->l1_size, 0, cur_l1_bytes - sn_l1_bytes);
-    }
-
     /*
      * Copy the snapshot L1 table to the current L1 table.
      *
      * Before overwriting the old current L1 table on disk, make sure to
      * increase all refcounts for the clusters referenced by the new one.
+     * Decrease the refcount referenced by the old one only when the L1
+     * table is overwritten.
      */
-    ret = bdrv_pread(bs->file, sn->l1_table_offset, s->l1_table, sn_l1_bytes);
+    sn_l1_table = g_malloc0(cur_l1_bytes);
+
+    ret = bdrv_pread(bs->file, sn->l1_table_offset, sn_l1_table, sn_l1_bytes);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    ret = qcow2_update_snapshot_refcount(bs, sn->l1_table_offset,
+                                         sn->l1_size, 1);
     if (ret < 0) {
         goto fail;
     }
 
-    ret = bdrv_pwrite_sync(bs->file, s->l1_table_offset, s->l1_table,
+    ret = bdrv_pwrite_sync(bs->file, s->l1_table_offset, sn_l1_table,
                            cur_l1_bytes);
     if (ret < 0) {
         goto fail;
     }
 
+    /*
+     * Decrease refcount of clusters of current L1 table.
+     *
+     * At this point, the in-memory s->l1_table points to the old L1 table,
+     * whereas on disk we already have the new one.
+     *
+     * qcow2_update_snapshot_refcount special cases the current L1 table to use
+     * the in-memory data instead of really using the offset to load a new one,
+     * which is why this works.
+     */
+    ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
+                                         s->l1_size, -1);
+
+    /*
+     * Now update the in-memory L1 table to be in sync with the on-disk one. We
+     * need to do this even if updating refcounts failed.
+     */
     for(i = 0;i < s->l1_size; i++) {
-        be64_to_cpus(&s->l1_table[i]);
+        s->l1_table[i] = be64_to_cpu(sn_l1_table[i]);
     }
 
-    /* FIXME This is too late! */
-    ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    g_free(sn_l1_table);
+    sn_l1_table = NULL;
+
+    /*
+     * Update QCOW_OFLAG_COPIED in the active L1 table (it may have changed
+     * when we decreased the refcount of the old snapshot.
+     */
+    ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 0);
     if (ret < 0) {
         goto fail;
     }
@@ -456,6 +482,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     return 0;
 
 fail:
+    g_free(sn_l1_table);
     return ret;
 }
 
-- 
1.7.6.4

[Qemu-devel] [PATCH v2 8/9] qcow2: Fix order in qcow2_snapshot_delete

[Kevin Wolf]

First the snapshot must be deleted and only then the refcounts can be
decreased.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-snapshot.c |   48 +++++++++++++++++++++++++++++++++---------------
 1 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 20cb66e..d4fbcb9 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -489,32 +489,50 @@ fail:
 int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
 {
     BDRVQcowState *s = bs->opaque;
-    QCowSnapshot *sn;
+    QCowSnapshot sn;
     int snapshot_index, ret;
 
+    /* Search the snapshot */
     snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
-    if (snapshot_index < 0)
+    if (snapshot_index < 0) {
         return -ENOENT;
-    sn = &s->snapshots[snapshot_index];
+    }
+    sn = s->snapshots[snapshot_index];
 
-    ret = qcow2_update_snapshot_refcount(bs, sn->l1_table_offset, sn->l1_size, -1);
-    if (ret < 0)
+    /* Remove it from the snapshot list */
+    memmove(s->snapshots + snapshot_index,
+            s->snapshots + snapshot_index + 1,
+            (s->nb_snapshots - snapshot_index - 1) * sizeof(sn));
+    s->nb_snapshots--;
+    ret = qcow2_write_snapshots(bs);
+    if (ret < 0) {
         return ret;
-    /* 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)
+    }
+
+    /*
+     * The snapshot is now unused, clean up. If we fail after this point, we
+     * won't recover but just leak clusters.
+     */
+    g_free(sn.id_str);
+    g_free(sn.name);
+
+    /*
+     * Now decrease the refcounts of clusters referenced by the snapshot and
+     * free the L1 table.
+     */
+    ret = qcow2_update_snapshot_refcount(bs, sn.l1_table_offset,
+                                         sn.l1_size, -1);
+    if (ret < 0) {
         return ret;
-    qcow2_free_clusters(bs, sn->l1_table_offset, sn->l1_size * sizeof(uint64_t));
+    }
+    qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * sizeof(uint64_t));
 
-    g_free(sn->id_str);
-    g_free(sn->name);
-    memmove(sn, sn + 1, (s->nb_snapshots - snapshot_index - 1) * sizeof(*sn));
-    s->nb_snapshots--;
-    ret = qcow2_write_snapshots(bs);
+    /* 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) {
-        /* XXX: restore snapshot if error ? */
         return ret;
     }
+
 #ifdef DEBUG_ALLOC
     {
         BdrvCheckResult result = {0};
-- 
1.7.6.4

[Qemu-devel] [PATCH v2 9/9] qcow2: Fix error path in qcow2_snapshot_load_tmp

[Kevin Wolf]

If the bdrv_read() of the snapshot's L1 table fails, return the right
error code and make sure that the old L1 table is still loaded and we
don't break the BlockDriverState completely.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-snapshot.c |   34 ++++++++++++++++++++++------------
 1 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index d4fbcb9..b2d1a56 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -573,32 +573,42 @@ int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
 
 int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name)
 {
-    int i, snapshot_index, l1_size2;
+    int i, snapshot_index;
     BDRVQcowState *s = bs->opaque;
     QCowSnapshot *sn;
+    uint64_t *new_l1_table;
+    int new_l1_bytes;
+    int ret;
 
+    assert(bs->read_only);
+
+    /* Search the snapshot */
     snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_name);
     if (snapshot_index < 0) {
         return -ENOENT;
     }
-
     sn = &s->snapshots[snapshot_index];
-    s->l1_size = sn->l1_size;
-    l1_size2 = s->l1_size * sizeof(uint64_t);
-    if (s->l1_table != NULL) {
-        g_free(s->l1_table);
-    }
 
-    s->l1_table_offset = sn->l1_table_offset;
-    s->l1_table = g_malloc0(align_offset(l1_size2, 512));
+    /* Allocate and read in the snapshot's L1 table */
+    new_l1_bytes = s->l1_size * sizeof(uint64_t);
+    new_l1_table = g_malloc0(align_offset(new_l1_bytes, 512));
 
-    if (bdrv_pread(bs->file, sn->l1_table_offset,
-                   s->l1_table, l1_size2) != l1_size2) {
-        return -1;
+    ret = bdrv_pread(bs->file, sn->l1_table_offset, new_l1_table, new_l1_bytes);
+    if (ret < 0) {
+        g_free(new_l1_table);
+        return ret;
     }
 
+    /* Switch the L1 table */
+    g_free(s->l1_table);
+
+    s->l1_size = sn->l1_size;
+    s->l1_table_offset = sn->l1_table_offset;
+    s->l1_table = new_l1_table;
+
     for(i = 0;i < s->l1_size; i++) {
         be64_to_cpus(&s->l1_table[i]);
     }
+
     return 0;
 }
-- 
1.7.6.4

Re: [Qemu-devel] [PATCH v2 5/9] qcow2: Rework qcow2_snapshot_create error handling

[Stefan Hajnoczi]

On Fri, Nov 18, 2011 at 6:29 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> +    /*
> +     * Increase the refcounts of all clusters and make sure everything is
> +     * stable on disk before updating the snapshot table to contain a pointer
> +     * to the new L1 table.
> +     */
> +    ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
> +    ret = bdrv_flush(bs->file);

Do we need to explicitly flush the qcow2 cache to ensure metadata
reaches the disk?

Stefan

Re: [Qemu-devel] [PATCH v2 0/9] qcow2: Fix error paths for internal snapshots

[Stefan Hajnoczi]

On Fri, Nov 18, 2011 at 6:28 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Kevin Wolf (9):
>  qcow2: Return real error code in qcow2_read_snapshots
>  qcow2: Return real error code in qcow2_write_snapshots
>  qcow2: Update snapshot table information at once
>  qcow2: Cleanups and memleak fix in qcow2_snapshot_create
>  qcow2: Rework qcow2_snapshot_create error handling
>  qcow2: Return real error in qcow2_snapshot_goto
>  qcow2: Fix order of refcount updates in qcow2_snapshot_goto
>  qcow2: Fix order in qcow2_snapshot_delete
>  qcow2: Fix error path in qcow2_snapshot_load_tmp
>
>  block/qcow2-refcount.c |    7 +-
>  block/qcow2-snapshot.c |  330 +++++++++++++++++++++++++++++++++++-------------
>  block/qcow2.c          |    5 +-
>  3 files changed, 248 insertions(+), 94 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

I left a comment asking about bdrv_flush(bs->file) versus explicitly
flushing the qcow2 cache, but it's a wider question that later patches
could address, if necessary.

Re: [Qemu-devel] [PATCH v2 5/9] qcow2: Rework qcow2_snapshot_create error handling

[Kevin Wolf]

Am 21.11.2011 17:47, schrieb Stefan Hajnoczi:
> On Fri, Nov 18, 2011 at 6:29 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> +    /*
>> +     * Increase the refcounts of all clusters and make sure everything is
>> +     * stable on disk before updating the snapshot table to contain a pointer
>> +     * to the new L1 table.
>> +     */
>> +    ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1);
>> +    if (ret < 0) {
>> +        goto fail;
>> +    }
>> +
>> +    ret = bdrv_flush(bs->file);
> 
> Do we need to explicitly flush the qcow2 cache to ensure metadata
> reaches the disk?

Yes, I think this should be a bdrv_flush(bs). I'm not completely sure if
it is really required, but I couldn't immediately tell why it's safe and
this isn't a fast path anyway, so I'll replace this before merging the
series (won't send out a v3 for this).

Kevin

Re: [Qemu-devel] [PATCH v2 5/9] qcow2: Rework qcow2_snapshot_create error handling

[Stefan Hajnoczi]

On Tue, Nov 22, 2011 at 9:14 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 21.11.2011 17:47, schrieb Stefan Hajnoczi:
>> On Fri, Nov 18, 2011 at 6:29 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>>> +    /*
>>> +     * Increase the refcounts of all clusters and make sure everything is
>>> +     * stable on disk before updating the snapshot table to contain a pointer
>>> +     * to the new L1 table.
>>> +     */
>>> +    ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1);
>>> +    if (ret < 0) {
>>> +        goto fail;
>>> +    }
>>> +
>>> +    ret = bdrv_flush(bs->file);
>>
>> Do we need to explicitly flush the qcow2 cache to ensure metadata
>> reaches the disk?
>
> Yes, I think this should be a bdrv_flush(bs). I'm not completely sure if
> it is really required, but I couldn't immediately tell why it's safe and
> this isn't a fast path anyway, so I'll replace this before merging the
> series (won't send out a v3 for this).

Okay.  I think it's useful to include flushes in functions that are
rarely called and only manipulate metadata.  It guarantees that
updates made by the function will happen before whatever the caller
decides to do next :).

Stefan

Re: [Qemu-devel] [PATCH v2 0/9] qcow2: Fix error paths for internal snapshots

[Kevin Wolf]

Am 21.11.2011 17:58, schrieb Stefan Hajnoczi:
> On Fri, Nov 18, 2011 at 6:28 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Kevin Wolf (9):
>>  qcow2: Return real error code in qcow2_read_snapshots
>>  qcow2: Return real error code in qcow2_write_snapshots
>>  qcow2: Update snapshot table information at once
>>  qcow2: Cleanups and memleak fix in qcow2_snapshot_create
>>  qcow2: Rework qcow2_snapshot_create error handling
>>  qcow2: Return real error in qcow2_snapshot_goto
>>  qcow2: Fix order of refcount updates in qcow2_snapshot_goto
>>  qcow2: Fix order in qcow2_snapshot_delete
>>  qcow2: Fix error path in qcow2_snapshot_load_tmp
>>
>>  block/qcow2-refcount.c |    7 +-
>>  block/qcow2-snapshot.c |  330 +++++++++++++++++++++++++++++++++++-------------
>>  block/qcow2.c          |    5 +-
>>  3 files changed, 248 insertions(+), 94 deletions(-)
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Applied to the block branch (for 1.1)

Do you think any of the patches could/should be merged for 1.0?

Kevin

Re: [Qemu-devel] [PATCH v2 0/9] qcow2: Fix error paths for internal snapshots

[Stefan Hajnoczi]

On Tue, Nov 22, 2011 at 2:12 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 21.11.2011 17:58, schrieb Stefan Hajnoczi:
>> On Fri, Nov 18, 2011 at 6:28 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>>> Kevin Wolf (9):
>>>  qcow2: Return real error code in qcow2_read_snapshots
>>>  qcow2: Return real error code in qcow2_write_snapshots
>>>  qcow2: Update snapshot table information at once
>>>  qcow2: Cleanups and memleak fix in qcow2_snapshot_create
>>>  qcow2: Rework qcow2_snapshot_create error handling
>>>  qcow2: Return real error in qcow2_snapshot_goto
>>>  qcow2: Fix order of refcount updates in qcow2_snapshot_goto
>>>  qcow2: Fix order in qcow2_snapshot_delete
>>>  qcow2: Fix error path in qcow2_snapshot_load_tmp
>>>
>>>  block/qcow2-refcount.c |    7 +-
>>>  block/qcow2-snapshot.c |  330 +++++++++++++++++++++++++++++++++++-------------
>>>  block/qcow2.c          |    5 +-
>>>  3 files changed, 248 insertions(+), 94 deletions(-)
>>
>> Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>
> Applied to the block branch (for 1.1)
>
> Do you think any of the patches could/should be merged for 1.0?

I would leave them and continue testing them over the 1.1 release
cycle.  No users have reported these issues AFAIK and they are not
regressions from the last QEMU release.  It's getting late in the 1.0
cycle and changing image format code is always delicate because of
what happens if something goes wrong.

My 2 cents.

Stefan