[06/22] qcow2: add dirty bitmaps extension
diff mbox

Message ID 1475232808-4852-7-git-send-email-vsementsov@virtuozzo.com
State New
Headers show

Commit Message

Vladimir Sementsov-Ogievskiy Sept. 30, 2016, 10:53 a.m. UTC
Add dirty 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 <vsementsov@virtuozzo.com>
---
 block/qcow2.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.h |  4 +++
 2 files changed, 87 insertions(+)

Comments

Max Reitz Oct. 1, 2016, 2:46 p.m. UTC | #1
On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
> Add dirty 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 <vsementsov@virtuozzo.com>
> ---
>  block/qcow2.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.h |  4 +++
>  2 files changed, 87 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c079aa8..08c4ef9 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c

[...]

> @@ -162,6 +164,62 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>              }
>              break;
>  
> +        case QCOW2_EXT_MAGIC_DIRTY_BITMAPS:
> +            ret = bdrv_pread(bs->file, offset, &bitmaps_ext, ext.len);

Overflows with ext.len > sizeof(bitmaps_ext).

(ext.len < sizeof(bitmaps_ext) is also wrong, but less dramatically so.)

> +            if (ret < 0) {
> +                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
> +                                 "Could not read ext header");
> +                return ret;
> +            }
> +
> +            if (bitmaps_ext.reserved32 != 0) {
> +                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
> +                                 "Reserved field is not zero.");

Please drop the full stop at the end.

> +                return -EINVAL;
> +            }
> +
> +            be32_to_cpus(&bitmaps_ext.nb_bitmaps);
> +            be64_to_cpus(&bitmaps_ext.bitmap_directory_size);
> +            be64_to_cpus(&bitmaps_ext.bitmap_directory_offset);
> +
> +            if (bitmaps_ext.nb_bitmaps > QCOW_MAX_DIRTY_BITMAPS) {
> +                error_setg(errp, "ERROR: bitmaps_ext: "
> +                                 "too many dirty bitmaps");
> +                return -EINVAL;
> +            }
> +
> +            if (bitmaps_ext.nb_bitmaps == 0) {
> +                error_setg(errp, "ERROR: bitmaps_ext: "
> +                                 "found bitmaps extension with zero bitmaps");
> +                return -EINVAL;
> +            }
> +
> +            if (bitmaps_ext.bitmap_directory_offset & (s->cluster_size - 1)) {
> +                error_setg(errp, "ERROR: bitmaps_ext: "
> +                                 "wrong dirty bitmap directory offset");

s/wrong/invalid/

> +                return -EINVAL;
> +            }
> +
> +            if (bitmaps_ext.bitmap_directory_size >
> +                QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) {
> +                error_setg(errp, "ERROR: bitmaps_ext: "
> +                                 "too large dirty bitmap directory");
> +                return -EINVAL;
> +            }
> +
> +            s->nb_bitmaps = bitmaps_ext.nb_bitmaps;
> +            s->bitmap_directory_offset =
> +                    bitmaps_ext.bitmap_directory_offset;
> +            s->bitmap_directory_size =
> +                    bitmaps_ext.bitmap_directory_size;
> +
> +#ifdef DEBUG_EXT
> +            printf("Qcow2: Got dirty bitmaps extension:"
> +                   " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n",
> +                   s->bitmap_directory_offset, s->nb_bitmaps);
> +#endif
> +            break;
> +
>          default:
>              /* unknown magic - save it in case we need to rewrite the header */
>              {

[...]

> @@ -2509,6 +2585,13 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
>          return -ENOTSUP;
>      }
>  
> +    /* cannot proceed if image has bitmaps */
> +    if (s->nb_bitmaps) {
> +        /* FIXME */

I'd call it a TODO, but that's probably a matter of taste.

> +        error_report("Can't resize an image which has bitmaps");
> +        return -ENOTSUP;
> +    }
> +
>      /* shrinking is currently not supported */
>      if (offset < bs->total_sectors * 512) {
>          error_report("qcow2 doesn't support shrinking images yet");

Max
Vladimir Sementsov-Ogievskiy Oct. 11, 2016, 12:09 p.m. UTC | #2
On 01.10.2016 17:46, Max Reitz wrote:
> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
>> Add dirty 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 <vsementsov@virtuozzo.com>
>> ---
>>   block/qcow2.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   block/qcow2.h |  4 +++
>>   2 files changed, 87 insertions(+)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index c079aa8..08c4ef9 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
> [...]
>
>> @@ -162,6 +164,62 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>>               }
>>               break;
>>   
>> +        case QCOW2_EXT_MAGIC_DIRTY_BITMAPS:
>> +            ret = bdrv_pread(bs->file, offset, &bitmaps_ext, ext.len);
> Overflows with ext.len > sizeof(bitmaps_ext).
>
> (ext.len < sizeof(bitmaps_ext) is also wrong, but less dramatically so.)
>
>> +            if (ret < 0) {
>> +                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
>> +                                 "Could not read ext header");
>> +                return ret;
>> +            }
>> +
>> +            if (bitmaps_ext.reserved32 != 0) {
>> +                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
>> +                                 "Reserved field is not zero.");
> Please drop the full stop at the end.

what do you mean? goto to fail: here? or not stop at all, just print error?

>
>> +                return -EINVAL;
>> +            }
>> +
>> +            be32_to_cpus(&bitmaps_ext.nb_bitmaps);
>> +            be64_to_cpus(&bitmaps_ext.bitmap_directory_size);
>> +            be64_to_cpus(&bitmaps_ext.bitmap_directory_offset);
>> +
>> +            if (bitmaps_ext.nb_bitmaps > QCOW_MAX_DIRTY_BITMAPS) {
>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>> +                                 "too many dirty bitmaps");
>> +                return -EINVAL;
>> +            }
>> +
>> +            if (bitmaps_ext.nb_bitmaps == 0) {
>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>> +                                 "found bitmaps extension with zero bitmaps");
>> +                return -EINVAL;
>> +            }
>> +
>> +            if (bitmaps_ext.bitmap_directory_offset & (s->cluster_size - 1)) {
>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>> +                                 "wrong dirty bitmap directory offset");
> s/wrong/invalid/
>
>> +                return -EINVAL;
>> +            }
>> +
>> +            if (bitmaps_ext.bitmap_directory_size >
>> +                QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) {
>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>> +                                 "too large dirty bitmap directory");
>> +                return -EINVAL;
>> +            }
>> +
>> +            s->nb_bitmaps = bitmaps_ext.nb_bitmaps;
>> +            s->bitmap_directory_offset =
>> +                    bitmaps_ext.bitmap_directory_offset;
>> +            s->bitmap_directory_size =
>> +                    bitmaps_ext.bitmap_directory_size;
>> +
>> +#ifdef DEBUG_EXT
>> +            printf("Qcow2: Got dirty bitmaps extension:"
>> +                   " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n",
>> +                   s->bitmap_directory_offset, s->nb_bitmaps);
>> +#endif
>> +            break;
>> +
>>           default:
>>               /* unknown magic - save it in case we need to rewrite the header */
>>               {
> [...]
>
>> @@ -2509,6 +2585,13 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
>>           return -ENOTSUP;
>>       }
>>   
>> +    /* cannot proceed if image has bitmaps */
>> +    if (s->nb_bitmaps) {
>> +        /* FIXME */
> I'd call it a TODO, but that's probably a matter of taste.
>
>> +        error_report("Can't resize an image which has bitmaps");
>> +        return -ENOTSUP;
>> +    }
>> +
>>       /* shrinking is currently not supported */
>>       if (offset < bs->total_sectors * 512) {
>>           error_report("qcow2 doesn't support shrinking images yet");
> Max
>
Max Reitz Oct. 12, 2016, 6:21 p.m. UTC | #3
On 11.10.2016 14:09, Vladimir Sementsov-Ogievskiy wrote:
> On 01.10.2016 17:46, Max Reitz wrote:
>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
>>> Add dirty 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 <vsementsov@virtuozzo.com>
>>> ---
>>>   block/qcow2.c | 83
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   block/qcow2.h |  4 +++
>>>   2 files changed, 87 insertions(+)
>>>
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index c079aa8..08c4ef9 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>> [...]
>>
>>> @@ -162,6 +164,62 @@ static int
>>> qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>>>               }
>>>               break;
>>>   +        case QCOW2_EXT_MAGIC_DIRTY_BITMAPS:
>>> +            ret = bdrv_pread(bs->file, offset, &bitmaps_ext, ext.len);
>> Overflows with ext.len > sizeof(bitmaps_ext).
>>
>> (ext.len < sizeof(bitmaps_ext) is also wrong, but less dramatically so.)
>>
>>> +            if (ret < 0) {
>>> +                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
>>> +                                 "Could not read ext header");
>>> +                return ret;
>>> +            }
>>> +
>>> +            if (bitmaps_ext.reserved32 != 0) {
>>> +                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
>>> +                                 "Reserved field is not zero.");
>> Please drop the full stop at the end.
> 
> what do you mean? goto to fail: here? or not stop at all, just print error?

The "." at the end of the message. :-)

(https://en.wikipedia.org/wiki/Full_stop)

Max
Vladimir Sementsov-Ogievskiy Oct. 13, 2016, 12:18 p.m. UTC | #4
On 12.10.2016 21:21, Max Reitz wrote:
> On 11.10.2016 14:09, Vladimir Sementsov-Ogievskiy wrote:
>> On 01.10.2016 17:46, Max Reitz wrote:
>>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
>>>> Add dirty 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 <vsementsov@virtuozzo.com>
>>>> ---
>>>>    block/qcow2.c | 83
>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    block/qcow2.h |  4 +++
>>>>    2 files changed, 87 insertions(+)
>>>>
>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>> index c079aa8..08c4ef9 100644
>>>> --- a/block/qcow2.c
>>>> +++ b/block/qcow2.c
>>> [...]
>>>
>>>> @@ -162,6 +164,62 @@ static int
>>>> qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>>>>                }
>>>>                break;
>>>>    +        case QCOW2_EXT_MAGIC_DIRTY_BITMAPS:
>>>> +            ret = bdrv_pread(bs->file, offset, &bitmaps_ext, ext.len);
>>> Overflows with ext.len > sizeof(bitmaps_ext).
>>>
>>> (ext.len < sizeof(bitmaps_ext) is also wrong, but less dramatically so.)
>>>
>>>> +            if (ret < 0) {
>>>> +                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
>>>> +                                 "Could not read ext header");
>>>> +                return ret;
>>>> +            }
>>>> +
>>>> +            if (bitmaps_ext.reserved32 != 0) {
>>>> +                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
>>>> +                                 "Reserved field is not zero.");
>>> Please drop the full stop at the end.
>> what do you mean? goto to fail: here? or not stop at all, just print error?
> The "." at the end of the message. :-)
>
> (https://en.wikipedia.org/wiki/Full_stop)
>
> Max
>
aha, cool. didn't know this. )

Patch
diff mbox

diff --git a/block/qcow2.c b/block/qcow2.c
index c079aa8..08c4ef9 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_DIRTY_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,62 @@  static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
             }
             break;
 
+        case QCOW2_EXT_MAGIC_DIRTY_BITMAPS:
+            ret = bdrv_pread(bs->file, offset, &bitmaps_ext, ext.len);
+            if (ret < 0) {
+                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
+                                 "Could not read ext header");
+                return ret;
+            }
+
+            if (bitmaps_ext.reserved32 != 0) {
+                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
+                                 "Reserved field is not zero.");
+                return -EINVAL;
+            }
+
+            be32_to_cpus(&bitmaps_ext.nb_bitmaps);
+            be64_to_cpus(&bitmaps_ext.bitmap_directory_size);
+            be64_to_cpus(&bitmaps_ext.bitmap_directory_offset);
+
+            if (bitmaps_ext.nb_bitmaps > QCOW_MAX_DIRTY_BITMAPS) {
+                error_setg(errp, "ERROR: bitmaps_ext: "
+                                 "too many dirty bitmaps");
+                return -EINVAL;
+            }
+
+            if (bitmaps_ext.nb_bitmaps == 0) {
+                error_setg(errp, "ERROR: bitmaps_ext: "
+                                 "found bitmaps extension with zero bitmaps");
+                return -EINVAL;
+            }
+
+            if (bitmaps_ext.bitmap_directory_offset & (s->cluster_size - 1)) {
+                error_setg(errp, "ERROR: bitmaps_ext: "
+                                 "wrong dirty bitmap directory offset");
+                return -EINVAL;
+            }
+
+            if (bitmaps_ext.bitmap_directory_size >
+                QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) {
+                error_setg(errp, "ERROR: bitmaps_ext: "
+                                 "too large dirty bitmap directory");
+                return -EINVAL;
+            }
+
+            s->nb_bitmaps = bitmaps_ext.nb_bitmaps;
+            s->bitmap_directory_offset =
+                    bitmaps_ext.bitmap_directory_offset;
+            s->bitmap_directory_size =
+                    bitmaps_ext.bitmap_directory_size;
+
+#ifdef DEBUG_EXT
+            printf("Qcow2: Got dirty bitmaps extension:"
+                   " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n",
+                   s->bitmap_directory_offset, s->nb_bitmaps);
+#endif
+            break;
+
         default:
             /* unknown magic - save it in case we need to rewrite the header */
             {
@@ -1939,6 +1997,24 @@  int qcow2_update_header(BlockDriverState *bs)
         buflen -= ret;
     }
 
+    if (s->nb_bitmaps > 0) {
+        Qcow2BitmapHeaderExt bitmaps_header = {
+            .nb_bitmaps = cpu_to_be32(s->nb_bitmaps),
+            .bitmap_directory_size =
+                    cpu_to_be64(s->bitmap_directory_size),
+            .bitmap_directory_offset =
+                    cpu_to_be64(s->bitmap_directory_offset)
+        };
+        ret = header_ext_add(buf, QCOW2_EXT_MAGIC_DIRTY_BITMAPS,
+                             &bitmaps_header, sizeof(bitmaps_header),
+                             buflen);
+        if (ret < 0) {
+            goto fail;
+        }
+        buf += ret;
+        buflen -= ret;
+    }
+
     /* Keep unknown header extensions */
     QLIST_FOREACH(uext, &s->unknown_header_ext, next) {
         ret = header_ext_add(buf, uext->magic, uext->data, uext->len, buflen);
@@ -2509,6 +2585,13 @@  static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
         return -ENOTSUP;
     }
 
+    /* cannot proceed if image has bitmaps */
+    if (s->nb_bitmaps) {
+        /* FIXME */
+        error_report("Can't resize an image which has bitmaps");
+        return -ENOTSUP;
+    }
+
     /* shrinking is currently not supported */
     if (offset < bs->total_sectors * 512) {
         error_report("qcow2 doesn't support shrinking images yet");
diff --git a/block/qcow2.h b/block/qcow2.h
index 0480b8b..c068b2c 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -292,6 +292,10 @@  typedef struct BDRVQcow2State {
     unsigned int nb_snapshots;
     QCowSnapshot *snapshots;
 
+    uint64_t bitmap_directory_offset;
+    uint64_t bitmap_directory_size;
+    uint32_t nb_bitmaps;
+
     int flags;
     int qcow_version;
     bool use_lazy_refcounts;