qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 09/21] qcow2: add .bdrv_load_autoloading_dirty_b


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-devel] [PATCH 09/21] qcow2: add .bdrv_load_autoloading_dirty_bitmaps
Date: Wed, 14 Dec 2016 18:54:54 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1

07.12.2016 23:51, Max Reitz wrote:
On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
Auto loading bitmaps are bitmaps in Qcow2, with the AUTO flag set. They
are loaded when the image is opened and become BdrvDirtyBitmaps for the
corresponding drive.

Extra data in bitmaps is not supported for now.

Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
---
  block/Makefile.objs  |   2 +-
  block/qcow2-bitmap.c | 663 +++++++++++++++++++++++++++++++++++++++++++++++++++
  block/qcow2.c        |   2 +
  block/qcow2.h        |   3 +
  4 files changed, 669 insertions(+), 1 deletion(-)
  create mode 100644 block/qcow2-bitmap.c

[...]

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
new file mode 100644
index 0000000..0f797e6
--- /dev/null
+++ b/block/qcow2-bitmap.c
@@ -0,0 +1,663 @@
[...]

+/* Check table entry specification constraints. If cluster_size is 0, offset
+ * alignment is not checked. */
+static int check_table_entry(uint64_t entry, int cluster_size)
+{
+    uint64_t offset;
+
+    if (entry & BME_TABLE_ENTRY_RESERVED_MASK) {
+        return -EINVAL;
+    }
+
+    offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
+    if (offset != 0) {
+        /* if offset specified, bit 0 must is reserved */
-must

+        if (entry & 1) {
+            return -EINVAL;
+        }
+
+        if ((cluster_size != 0) && (entry % cluster_size != 0)) {
Why would cluster_size be 0? Also, shouldn't it be offset instead of entry?

the comment says: "If cluster_size is 0, offset alignment is not checked"
yes, should be offset.


+            return -EINVAL;
+        }
+    }
+
+    return 0;
+}
[...]

+/* bitmap table entries must satisfy specification constraints */
+static int load_bitmap_data(BlockDriverState *bs,
+                            const uint64_t *bitmap_table,
+                            uint32_t bitmap_table_size,
+                            BdrvDirtyBitmap *bitmap)
+{
+    int ret = 0;
+    BDRVQcow2State *s = bs->opaque;
+    uint64_t sector, dsc;
+    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
+    uint8_t *buf = NULL;
+    uint64_t i, tab_size =
+            size_to_clusters(s,
+                bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
+
+    if (tab_size != bitmap_table_size ||
+            tab_size > BME_MAX_TABLE_SIZE) {
This line should be aligned to the opening parenthesis.

[...]

+static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
+{
+    BDRVQcow2State *s = bs->opaque;
+    uint64_t phys_bitmap_bytes =
+            (uint64_t)entry->bitmap_table_size * s->cluster_size;
+    uint64_t max_virtual_bits =
+            (phys_bitmap_bytes * 8) << entry->granularity_bits;
I think this shift can overflow, even if bitmap_table_size, cluster_size
and granularity_bits are all within their respective limits.

Hmm. But it can't, if phys_bitmap_bytes <= MAX_PHYS_SIZE. I can just move this calculation down.


+    int64_t nb_sectors = bdrv_nb_sectors(bs);
Just using bdrv_getlength() would probably be simpler.

+
+    if (nb_sectors < 0) {
+        return nb_sectors;
+    }
+
+    int fail =
I'd prefer a boolean.

and mixed definition/code)


+            (entry->bitmap_table_size == 0) ||
+            (entry->bitmap_table_offset == 0) ||
+            (entry->bitmap_table_offset % s->cluster_size) ||
+            (entry->bitmap_table_size > BME_MAX_TABLE_SIZE) ||
+            (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) ||
+            (entry->bitmap_table_offset != 0 &&

hmm, can be dropped, as checked above

+                (nb_sectors << BDRV_SECTOR_BITS) > max_virtual_bits) ||
+            (entry->granularity_bits > BME_MAX_GRANULARITY_BITS) ||
+            (entry->granularity_bits < BME_MIN_GRANULARITY_BITS) ||
+            (entry->flags & BME_RESERVED_FLAGS) ||
+            (entry->name_size > BME_MAX_NAME_SIZE) ||
+            (entry->type != BT_DIRTY_TRACKING_BITMAP);
+
+    return fail ? -EINVAL : 0;
+}
[...]

+/* bitmap_list_load
+ * Get bitmap list from qcow2 image. Actually reads bitmap directory,
+ * checks it and convert to bitmap list.
+ */
+static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
+                                         uint64_t size, Error **errp)
+{
[...]

+    bm_list = bitmap_list_new();
+    for (e = (Qcow2BitmapDirEntry *)dir;
+             e < (Qcow2BitmapDirEntry *)dir_end; e = next_dir_entry(e)) {
This line should be aligned to the opening parenthesis.

[...]

+
+/* bitmap_list_store
+ * Store bitmap list to qcow2 image as a bitmap directory.
+ * Everything is checked.
+ */
+static int bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
+                             uint64_t *offset, uint64_t *size, bool in_place)
+{
[...]

+fail:
+    g_free(dir);
+
+    if (dir_offset > 0) {
+        qcow2_free_clusters(bs, dir_offset, dir_size, QCOW2_DISCARD_OTHER);
I think this should only be freed if in_place is false.

Reasonable.. And this also leads me to the think that we should clear autoclear bit in the header before inplace updating of bitmap directory and set it after successful update.


Max

+    }
+
+    return ret;
+}


--
Best regards,
Vladimir




reply via email to

[Prev in Thread] Current Thread [Next in Thread]