From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Realize .bdrv_remove_persistent_dirty_bitmap interface.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 20170628120530.31251-29-vsementsov@virtuozzo.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.h        |  3 +++
 block/qcow2-bitmap.c | 41 +++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c        |  1 +
 3 files changed, 45 insertions(+)

diff --git a/block/qcow2.h b/block/qcow2.h
index 084a298..2df05ac 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -653,5 +653,8 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
                                       const char *name,
                                       uint32_t granularity,
                                       Error **errp);
+void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+                                          const char *name,
+                                          Error **errp);
 
 #endif
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index a8c7e1d..3e8735a 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1237,6 +1237,47 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list,
     return NULL;
 }
 
+void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+                                          const char *name,
+                                          Error **errp)
+{
+    int ret;
+    BDRVQcow2State *s = bs->opaque;
+    Qcow2Bitmap *bm;
+    Qcow2BitmapList *bm_list;
+
+    if (s->nb_bitmaps == 0) {
+        /* Absence of the bitmap is not an error: see explanation above
+         * bdrv_remove_persistent_dirty_bitmap() definition. */
+        return;
+    }
+
+    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
+                               s->bitmap_directory_size, errp);
+    if (bm_list == NULL) {
+        return;
+    }
+
+    bm = find_bitmap_by_name(bm_list, name);
+    if (bm == NULL) {
+        goto fail;
+    }
+
+    QSIMPLEQ_REMOVE(bm_list, bm, Qcow2Bitmap, entry);
+
+    ret = update_ext_header_and_dir(bs, bm_list);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to update bitmap extension");
+        goto fail;
+    }
+
+    free_bitmap_clusters(bs, &bm->table);
+
+fail:
+    bitmap_free(bm);
+    bitmap_list_free(bm_list);
+}
+
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 {
     BdrvDirtyBitmap *bitmap;
diff --git a/block/qcow2.c b/block/qcow2.c
index d7046a3..9d3c70e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3990,6 +3990,7 @@ BlockDriver bdrv_qcow2 = {
 
     .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw,
     .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap,
+    .bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap,
 };
 
 static void bdrv_qcow2_init(void)
-- 
2.9.4

On 11 July 2017 at 17:07, Max Reitz <mreitz@redhat.com> wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Realize .bdrv_remove_persistent_dirty_bitmap interface.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> Message-id: 20170628120530.31251-29-vsementsov@virtuozzo.com
> Signed-off-by: Max Reitz <mreitz@redhat.com>

> +void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> +                                          const char *name,
> +                                          Error **errp)
> +{
> +    int ret;
> +    BDRVQcow2State *s = bs->opaque;
> +    Qcow2Bitmap *bm;
> +    Qcow2BitmapList *bm_list;
> +
> +    if (s->nb_bitmaps == 0) {
> +        /* Absence of the bitmap is not an error: see explanation above
> +         * bdrv_remove_persistent_dirty_bitmap() definition. */
> +        return;
> +    }
> +
> +    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> +                               s->bitmap_directory_size, errp);
> +    if (bm_list == NULL) {
> +        return;
> +    }
> +
> +    bm = find_bitmap_by_name(bm_list, name);
> +    if (bm == NULL) {
> +        goto fail;
> +    }
> +
> +    QSIMPLEQ_REMOVE(bm_list, bm, Qcow2Bitmap, entry);
> +
> +    ret = update_ext_header_and_dir(bs, bm_list);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "Failed to update bitmap extension");
> +        goto fail;
> +    }
> +
> +    free_bitmap_clusters(bs, &bm->table);
> +
> +fail:
> +    bitmap_free(bm);
> +    bitmap_list_free(bm_list);
> +}

Coverity points out that this can crash in the error-exit paths,
because bitmap_free() doesn't handle being passed a NULL pointer.
(CID 1377700).

Probably the best fix for this is to make bitmap_free() do
nothing when handed NULL.

thanks
-- PMM

14.07.2017 13:42, Peter Maydell wrote:
> On 11 July 2017 at 17:07, Max Reitz <mreitz@redhat.com> wrote:
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> Realize .bdrv_remove_persistent_dirty_bitmap interface.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>> Message-id: 20170628120530.31251-29-vsementsov@virtuozzo.com
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> +void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> +                                          const char *name,
>> +                                          Error **errp)
>> +{
>> +    int ret;
>> +    BDRVQcow2State *s = bs->opaque;
>> +    Qcow2Bitmap *bm;
>> +    Qcow2BitmapList *bm_list;
>> +
>> +    if (s->nb_bitmaps == 0) {
>> +        /* Absence of the bitmap is not an error: see explanation above
>> +         * bdrv_remove_persistent_dirty_bitmap() definition. */
>> +        return;
>> +    }
>> +
>> +    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> +                               s->bitmap_directory_size, errp);
>> +    if (bm_list == NULL) {
>> +        return;
>> +    }
>> +
>> +    bm = find_bitmap_by_name(bm_list, name);
>> +    if (bm == NULL) {
>> +        goto fail;
>> +    }
>> +
>> +    QSIMPLEQ_REMOVE(bm_list, bm, Qcow2Bitmap, entry);
>> +
>> +    ret = update_ext_header_and_dir(bs, bm_list);
>> +    if (ret < 0) {
>> +        error_setg_errno(errp, -ret, "Failed to update bitmap extension");
>> +        goto fail;
>> +    }
>> +
>> +    free_bitmap_clusters(bs, &bm->table);
>> +
>> +fail:
>> +    bitmap_free(bm);
>> +    bitmap_list_free(bm_list);
>> +}
> Coverity points out that this can crash in the error-exit paths,
> because bitmap_free() doesn't handle being passed a NULL pointer.
> (CID 1377700).
>
> Probably the best fix for this is to make bitmap_free() do
> nothing when handed NULL.

Agree, my stupid omission. Can this be fixed in flight? Just squash into 
commit "qcow2: add bitmaps extension"

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 8448bec46d..39dfe16fc0 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -487,6 +487,10 @@ static inline void bitmap_directory_to_be(uint8_t 
*dir, size_t size)

  static void bitmap_free(Qcow2Bitmap *bm)
  {
+    if (bm == NULL) {
+        return;
+    }
+
      g_free(bm->name);
      g_free(bm);
  }
//

>
> thanks
> -- PMM


-- 
Best regards,
Vladimir

On 14 July 2017 at 13:04, Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
> 14.07.2017 13:42, Peter Maydell wrote:
> Coverity points out that this can crash in the error-exit paths,
> because bitmap_free() doesn't handle being passed a NULL pointer.
> (CID 1377700).
>
> Probably the best fix for this is to make bitmap_free() do
> nothing when handed NULL.
>
>
> Agree, my stupid omission. Can this be fixed in flight?

No, this code has already hit master -- you'll need to send
a fresh patch to fix it.

thanks
-- PMM

On 07/14/2017 07:04 AM, Vladimir Sementsov-Ogievskiy wrote:
> 14.07.2017 13:42, Peter Maydell wrote:
>> On 11 July 2017 at 17:07, Max Reitz <mreitz@redhat.com> wrote:
>>> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>

>> Probably the best fix for this is to make bitmap_free() do
>> nothing when handed NULL.
> 
> Agree, my stupid omission. Can this be fixed in flight? Just squash into
> commit "qcow2: add bitmaps extension"

Your commit has already landed on master (commit 469c71e), so you'll
need to resubmit this as a new patch in a top-level thread. But given
the simplicity, feel free to add:

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 8448bec46d..39dfe16fc0 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -487,6 +487,10 @@ static inline void bitmap_directory_to_be(uint8_t
> *dir, size_t size)
> 
>  static void bitmap_free(Qcow2Bitmap *bm)
>  {
> +    if (bm == NULL) {
> +        return;
> +    }
> +
>      g_free(bm->name);
>      g_free(bm);
>  }
> //
> 
>>
>> thanks
>> -- PMM
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org