[PULL,50/85] qcow2: add .bdrv_remove_persistent_dirty_bitmap

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"

  }
//

>
> thanks
> -- PMM

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
> 
>