Re: [Qemu-devel] [PATCH 23/24] qcow2: add .bdrv_remove_persistent_dirty_bitmap

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

01.02.2017 02:20, Max Reitz wrote:
> On 23.01.2017 13:10, Vladimir Sementsov-Ogievskiy wrote:
>> Realize .bdrv_remove_persistent_dirty_bitmap interface.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/qcow2-bitmap.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>   block/qcow2.c        |  1 +
>>   block/qcow2.h        |  3 +++
>>   3 files changed, 44 insertions(+)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 2687a3acd5..be026fc80e 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -1064,6 +1064,46 @@ 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) {
>> +        /* No bitmaps - nothing to do */
> Shouldn't it be an error? I.e. "bitmap not found"?
>
>> +        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;
> What about setting errp? Or do you not consider this an error?
>
> I think it should be an error.

Hmm. The following scenery should not be an error:
1. qmp create dirty bitmap with persistent = true (bitmap not exists and 
not created  in the storage, it will be saved only on storage close)
2. qmp remove dirty bitmap. persistent = true, so we will try to remove 
it from storage..

So, I see following variants:
1. as is, just add comment "remove if exists"
2. add return value to this function, return ENOENT if bitmap not found 
and ignore this ENOENT in qmp_remove_bitmap
3. add additional bool field 'exist_in_storage' to BdrvDirtyBitmap.

>
>> +    }
>> +
>> +    QSIMPLEQ_REMOVE(bm_list, bm, Qcow2Bitmap, entry);
> bm->name is leaked here.
>
> Max
>
>> +
>> +    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 76a6a2ba28..f6bac38e51 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -3544,6 +3544,7 @@ BlockDriver bdrv_qcow2 = {
>>       .bdrv_load_autoloading_dirty_bitmaps = qcow2_load_autoloading_dirty_bitmaps,
>>       .bdrv_store_persistent_dirty_bitmaps = qcow2_store_persistent_dirty_bitmaps,
>>       .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)
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index eaad34a527..a009827f60 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -627,5 +627,8 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>   int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>>                                     void **refcount_table,
>>                                     int64_t *refcount_table_size);
>> +void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> +                                          const char *name,
>> +                                          Error **errp);
>>   
>>   #endif
>>
>


-- 
Best regards,
Vladimir