On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
Realize block bitmap stroing interface, to allow qcow2 images store persistent bitmaps.Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> --- block/qcow2-bitmap.c | 241 +++++++++++++++++++++++++++++++++++++++++++++++++++ block/qcow2.c | 2 + block/qcow2.h | 2 + 3 files changed, 245 insertions(+) diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 81520cd..a5be25a 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -27,6 +27,7 @@ #include "qemu/osdep.h" #include "qapi/error.h" +#include "qemu/cutils.h" #include "block/block_int.h" #include "block/qcow2.h" @@ -96,6 +97,15 @@ static inline void bitmap_table_to_cpu(uint64_t *bitmap_table, size_t size) } } +static inline void bitmap_table_to_be(uint64_t *bitmap_table, size_t size) +{ + size_t i; + + for (i = 0; i < size; ++i) { + cpu_to_be64s(&bitmap_table[i]); + } +} + static inline int calc_dir_entry_size(size_t name_size, size_t extra_data_size) { return align_offset(sizeof(Qcow2BitmapDirEntry) + @@ -564,3 +574,234 @@ out: return ret; } + +/* store_bitmap_data() + * Store bitmap to image, filling bitamp table accordingly.
+ */
+static int store_bitmap_data(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+ uint64_t *bitmap_table, uint32_t
bitmap_table_size)
+{
+ int ret;
+ BDRVQcow2State *s = bs->opaque;
+ uint64_t sector, dsc;
+ uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
+ int cl_size = s->cluster_size;
+ uint8_t *buf = NULL; + uint32_t tb_size = + size_to_clusters(s, + bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
+
+ BdrvDirtyBitmapIter *dbi;
+
+ if (tb_size != bitmap_table_size) {
+ return -EINVAL;
+ }
+
+ memset(bitmap_table, 0, bitmap_table_size * sizeof(bitmap_table[0]));
+
+ dbi = bdrv_dirty_iter_new(bitmap, 0);
+ buf = g_malloc(cl_size);
+ dsc = dirty_sectors_in_cluster(s, bitmap);
+
+ while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
+ uint64_t cluster = sector / dsc; + sector = cluster * dsc;
+ uint64_t end = MIN(bm_size, sector + dsc);
+ uint64_t write_size =
+ bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - sector);
+
+ int64_t off = qcow2_alloc_clusters(bs, cl_size);
+ if (off < 0) {
+ ret = off;
+ goto finish;
+ }
+ bitmap_table[cluster] = off;
+
+ bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end);
+ if (write_size < cl_size) {
+ memset(buf + write_size, 0, cl_size - write_size);
+ }
+
+ ret = bdrv_pwrite(bs->file, off, buf, cl_size);
+ if (ret < 0) {
+ goto finish;
+ }
+
+ if (end >= bm_size) {
+ break;
+ }
+
+ bdrv_set_dirty_iter(dbi, end);
+ }
+ ret = 0; /* writes */
What is that comment supposed to mean?
+
+finish:
+ if (ret < 0) {
+ clear_bitmap_table(bs, bitmap_table, bitmap_table_size);
+ }
+ g_free(buf);
+ bdrv_dirty_iter_free(dbi);
+
+ return ret;
In case you decide to keep BME_MAX_PHYS_SIZE, this function should check somewhere that the physical size of the bitmap does not exceed that value.
+} + +/* store_bitmap() + * Store bitmap to qcow2 and set bitmap_table. bitmap_table itself is not + * stored to qcow2.
First of all, there is no parameter called "bitmap_table", and second, yes, the bitmap table is written to the qcow2 file.
+ */
+static int store_bitmap(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ Qcow2BitmapDirEntry *entry)
+{
+ int ret;
+ BDRVQcow2State *s = bs->opaque;
+ uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
+ const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
+
+ uint64_t *tb;
+ int64_t tb_offset;
+ uint32_t tb_size =
+ size_to_clusters(s,
+ bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
As above, this variable should be of type uint64_t. Also, you have to check that it does not exceed BME_MAX_TABLE_SIZE.
+
+ tb = g_try_new(uint64_t, tb_size);
+ if (tb == NULL) {
+ return -ENOMEM;
+ }
+
+ ret = store_bitmap_data(bs, bitmap, tb, tb_size);
+ if (ret < 0) {
+ g_free(tb);
+ return ret;
+ }
+
+ tb_offset = qcow2_alloc_clusters(bs, tb_size * sizeof(tb[0]));
If you don't limit tb_size, then this multiplication can overflow on 32 bit machines.
+ if (tb_offset < 0) {
+ ret = tb_offset;
+ goto fail;
+ }
+
There should be a metadata overlap check here.
+ bitmap_table_to_be(tb, tb_size);
+ ret = bdrv_pwrite(bs->file, tb_offset, tb, tb_size * sizeof(tb[0]));
+ if (ret < 0) {
+ goto fail;
+ }
+
+ g_free(tb);
+
+ entry->bitmap_table_offset = tb_offset;
+ entry->bitmap_table_size = tb_size;
+ entry->flags = bdrv_dirty_bitmap_granularity(bitmap) ? BME_FLAG_AUTO : 0;
s/granularity/get_autoload/
+ entry->type = BT_DIRTY_TRACKING_BITMAP; + entry->granularity_bits = ctz32(bdrv_dirty_bitmap_granularity(bitmap));
You should probably check somewhere that the resulting value for
entry->granularity_bits is in the BME_{MIN,MAX}_GRANULARITY_BITS range.
+ entry->name_size = strlen(bm_name);
And that this length does not exceed BME_MAX_NAME_SIZE.
+ entry->extra_data_size = 0;
+ memcpy(entry + 1, bm_name, entry->name_size);
+
+ return 0;
+
+fail:
+ clear_bitmap_table(bs, tb, tb_size);
+
+ if (tb_offset > 0) {
+ qcow2_free_clusters(bs, tb_offset, tb_size, QCOW2_DISCARD_ALWAYS);
As before, I'd vote for QCOW2_DISCARD_OTHER.
+ }
+
+ g_free(tb);
+
+ return ret;
+}
+
+static Qcow2BitmapDirEntry *find_bitmap_by_name(uint8_t *bitmap_directory,
+ size_t size, const char *name)
+{
+ Qcow2BitmapDirEntry *e;
+
+ for_each_bitmap_dir_entry(e, bitmap_directory, size) {
+ if (strncmp((char *)(e + 1), name, e->name_size) == 0) {
+ return e;
+ }
+ }
+
+ return NULL;
+}
+
+void qcow2_store_persistent_bitmaps(BlockDriverState *bs, Error **errp)
+{
+ BdrvDirtyBitmap *bm;
+ BDRVQcow2State *s = bs->opaque;
+ uint32_t new_nb_bitmaps = s->nb_bitmaps;
+ uint64_t new_dir_size = s->bitmap_directory_size;
+ uint8_t *dir = NULL, *new_dir = NULL;
+ int ret;
+ Qcow2BitmapDirEntry *new_pos;
+
+ if (s->nb_bitmaps > 0) {
+ dir = directory_read(bs, s->bitmap_directory_offset,
+ s->bitmap_directory_size, errp);
+ if (dir == NULL) {
+ goto out;
+ }
+ }
+
+ for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm != NULL;
+ bm = bdrv_dirty_bitmap_next(bs, bm)) {
+ const char *name = bdrv_dirty_bitmap_name(bm);
+
+ if (!bdrv_dirty_bitmap_get_persistance(bm)) {
+ continue;
+ }
+
+ if (s->nb_bitmaps > 0 &&
+ find_bitmap_by_name(dir, s->bitmap_directory_size, name)) {
+ error_setg(errp,
+ "Can't store bitmap '%s' to '%s', as it already exists",
+ name, bdrv_get_device_or_node_name(bs));
+ goto out;
+ }
+
+ new_nb_bitmaps++;
+ new_dir_size += calc_dir_entry_size(strlen(name), 0);
+ }
+
+ if (s->nb_bitmaps == new_nb_bitmaps) {
+ /* No new bitmaps - nothing to do */
+ goto out;
+ }
+
+ new_dir = g_try_malloc0(new_dir_size);
+ if (new_dir == NULL) {
+ error_setg(errp, "Can't allocate space for bitmap directory.");
+ goto out;
+ }
+
+ memcpy(new_dir, dir, s->bitmap_directory_size);
+ new_pos = (Qcow2BitmapDirEntry *)(new_dir + s->bitmap_directory_size);
+
+ for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm != NULL;
+ bm = bdrv_dirty_bitmap_next(bs, bm)) {
+ if (!bdrv_dirty_bitmap_get_persistance(bm)) {
+ continue;
+ }
+
+ ret = store_bitmap(bs, bm, new_pos);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "Can't store bitmap '%s' to '%s'",
+ bdrv_dirty_bitmap_name(bm),
+ bdrv_get_device_or_node_name(bs));
+ goto out;
+ }
+ new_pos = next_dir_entry(new_pos);
+ }
+
+ ret = directory_update(bs, new_dir, new_dir_size, new_nb_bitmaps);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "Can't update bitmap directory in '%s'",
+ bdrv_get_device_or_node_name(bs));
+ goto out;
+ }
+
+out:
+ g_free(new_dir);
+ g_free(dir);
This error path leaks all the bitmaps that have been written successfully (if any). I guess this is more or less fine if directory_update() failed (because you can't really tell the state of the image header after directory_update(), so better be safe) but it's not so fine if just some store_bitmap() failed. Max
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index 02ec224..8238205 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3493,6 +3493,8 @@ BlockDriver bdrv_qcow2 = {
.bdrv_detach_aio_context = qcow2_detach_aio_context,
.bdrv_attach_aio_context = qcow2_attach_aio_context,
+
+ .bdrv_store_persistent_bitmaps = qcow2_store_persistent_bitmaps,
};
static void bdrv_qcow2_init(void)
diff --git a/block/qcow2.h b/block/qcow2.h
index 482a29f..dfcf4c6 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -627,4 +627,6 @@ int qcow2_cache_get_empty(BlockDriverState *bs, Qcow2Cache
*c, uint64_t offset,
void **table);
void qcow2_cache_put(BlockDriverState *bs, Qcow2Cache *c, void **table);
+void qcow2_store_persistent_bitmaps(BlockDriverState *bs, Error **errp);
+
#endif
signature.asc
Description: PGP signature
