Re: [Qemu-devel] [PULL 50/85] qcow2: add .bdrv_remove_persistent_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

14.07.2017 13:42, Peter Maydell wrote:
> On 11 July 2017 at 17:07, Max Reitz <address@hidden> wrote:
> > From: Vladimir Sementsov-Ogievskiy <address@hidden>
> >
> > Realize .bdrv_remove_persistent_dirty_bitmap interface.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > Reviewed-by: Max Reitz <address@hidden>
> > Reviewed-by: John Snow <address@hidden>
> > Message-id: address@hidden
> > Signed-off-by: Max Reitz <address@hidden>
> > +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