From 42deb29fed92577db57c54e844f852481600b756 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 16 Nov 2011 11:43:28 +0100 Subject: qcow2: Return real error code in qcow2_read_snapshots Signed-off-by: Kevin Wolf Reviewed-by: Stefan Hajnoczi --- block/qcow2-snapshot.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) (limited to 'block/qcow2-snapshot.c') diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index bdc33ba94c..4134bbc253 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 */ -- cgit v1.2.3 From 07fd8779001c3e6ff3ac25416a31381ff8aca308 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 16 Nov 2011 12:00:59 +0100 Subject: qcow2: Return real error code in qcow2_write_snapshots 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 Reviewed-by: Stefan Hajnoczi --- block/qcow2-snapshot.c | 48 ++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 38 insertions(+), 10 deletions(-) (limited to 'block/qcow2-snapshot.c') diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index 4134bbc253..e91a286333 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, -- cgit v1.2.3 From d69969c4046a81af106e723ff1bc2740365e67c1 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 18 Nov 2011 18:27:00 +0100 Subject: qcow2: Update snapshot table information at once 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 Reviewed-by: Stefan Hajnoczi --- block/qcow2-snapshot.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) (limited to 'block/qcow2-snapshot.c') diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index e91a286333..1976df7f00 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; } -- cgit v1.2.3 From 03343166f703d5c8f02b8519f8493c56e5541ae7 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 16 Nov 2011 17:46:29 +0100 Subject: qcow2: Cleanups and memleak fix in qcow2_snapshot_create 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 Reviewed-by: Stefan Hajnoczi --- block/qcow2-snapshot.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) (limited to 'block/qcow2-snapshot.c') diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index 1976df7f00..53d9233ba9 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; -- cgit v1.2.3 From d1ea98d56dc2485b4637a1ca19feef786b3aee8f Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 16 Nov 2011 12:43:59 +0100 Subject: qcow2: Rework qcow2_snapshot_create error handling 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 Reviewed-by: Stefan Hajnoczi --- block/qcow2-snapshot.c | 55 +++++++++++++++++++++++++++++++++++++------------- 1 file changed, 41 insertions(+), 14 deletions(-) (limited to 'block/qcow2-snapshot.c') diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index 53d9233ba9..a8add9c857 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); + 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 */ -- cgit v1.2.3 From 589f284b7690bbb4ed37170590bae9527ed31b42 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 16 Nov 2011 15:04:11 +0100 Subject: qcow2: Return real error in qcow2_snapshot_goto 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 Reviewed-by: Stefan Hajnoczi --- block/qcow2-snapshot.c | 51 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 40 insertions(+), 11 deletions(-) (limited to 'block/qcow2-snapshot.c') diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index a8add9c857..1ee22eb28a 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) -- cgit v1.2.3 From 43a0cac4658bbee9c9e84554712a94daa092c1cd Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 16 Nov 2011 15:20:45 +0100 Subject: qcow2: Fix order of refcount updates in qcow2_snapshot_goto 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 Reviewed-by: Stefan Hajnoczi --- block/qcow2-snapshot.c | 61 ++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 17 deletions(-) (limited to 'block/qcow2-snapshot.c') diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index 1ee22eb28a..9fb3ff0253 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; } -- cgit v1.2.3 From 9a4767809fe9ac184806bef38be2e2a84e451a65 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 16 Nov 2011 17:22:10 +0100 Subject: qcow2: Fix order in qcow2_snapshot_delete First the snapshot must be deleted and only then the refcounts can be decreased. Signed-off-by: Kevin Wolf Reviewed-by: Stefan Hajnoczi --- block/qcow2-snapshot.c | 48 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 33 insertions(+), 15 deletions(-) (limited to 'block/qcow2-snapshot.c') diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index 9fb3ff0253..e959ef263e 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}; -- cgit v1.2.3 From e3f652b33228e16e117a93fb919c4e1e4753f5a5 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 16 Nov 2011 17:30:33 +0100 Subject: qcow2: Fix error path in qcow2_snapshot_load_tmp 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 Reviewed-by: Stefan Hajnoczi --- block/qcow2-snapshot.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) (limited to 'block/qcow2-snapshot.c') diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index e959ef263e..c3112bf71a 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; } -- cgit v1.2.3