qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH 07/21] qcow2: add bitmaps extension


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

16.12.2016 17:25, Max Reitz wrote:
On 14.12.2016 13:23, Vladimir Sementsov-Ogievskiy wrote:
07.12.2016 21:25, Max Reitz wrote:
On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
Add bitmap extension as specified in docs/specs/qcow2.txt.
For now, just mirror extension header into Qcow2 state and check
constraints.

For now, disable image resize if it has bitmaps. It will be fixed later.

Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
Reviewed-by: John Snow <address@hidden>
---
   block/qcow2.c | 107
++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
   block/qcow2.h |  24 +++++++++++++
   2 files changed, 129 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 6d5689a..7908657 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -63,6 +63,7 @@ typedef struct {
   #define  QCOW2_EXT_MAGIC_END 0
   #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
   #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
+#define  QCOW2_EXT_MAGIC_BITMAPS 0x23852875
     static int qcow2_probe(const uint8_t *buf, int buf_size, const
char *filename)
   {
@@ -92,6 +93,7 @@ static int qcow2_read_extensions(BlockDriverState
*bs, uint64_t start_offset,
       QCowExtension ext;
       uint64_t offset;
       int ret;
+    Qcow2BitmapHeaderExt bitmaps_ext;
     #ifdef DEBUG_EXT
       printf("qcow2_read_extensions: start=%ld end=%ld\n",
start_offset, end_offset);
@@ -162,6 +164,81 @@ static int
qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
               }
               break;
   +        case QCOW2_EXT_MAGIC_BITMAPS:
+            if (ext.len != sizeof(bitmaps_ext)) {
+                error_setg_errno(errp, -ret, "bitmaps_ext: "
+                                 "Invalid extension length");
+                return -EINVAL;
+            }
+
+            if (!(s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS)) {
+                fprintf(stderr,
+                        "WARNING: a program lacking bitmap support
modified "
+                        "this file, so all bitmaps are now considered "
+                        "inconsistent");
+                break;
+            }
I think It would be nice if qemu-img check could remove all bitmaps and
related data if this bit is not set. Otherwise, we're practically
leaking data.
Is it safe? Can old version somehow reuse these clusters?
You're right, it can. It's not so easy then.

                                                           Actually, all
data will be removed on check, as there will be no refcounts if this bit
is not set.
The refcounts will be there, but no references, right. So an old
qemu-img check -r leaks will free the clusters.

We could still remove this extension field, right? Then the clusters are
at least really leaked so that even a new qemu-img check -r leaks will
free them.

So, IMHO, only extension header should be deleted additionally in
qemu-img check.
I think we could remove it here instead of only in qemu-img check, but
either way should be fine. Doing it in qemu-img check is probably
cleaner, actually.

Now I think it would be more deterministic to clear extension header here, as, actually, it will be cleared on any future qcow2_update_header().


If you do it there, however, the warning should probably include a hint
like "(Use 'qemu-img check -r leaks' to reclaim the space used for the
bitmaps)".

Max



--
Best regards,
Vladimir




reply via email to

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