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

[Max Reitz]

On 01.02.2017 14:57, Vladimir Sementsov-Ogievskiy wrote:
> 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 <address@hidden>
>>> ---
>>>   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..

That's a good point.

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

1 sounds the best to me. I don't think there is a reason why a bitmap
may not be found other than the scenario you've described.

It would be good if the comment included that explanation as for why
it's not an error.

Max

signature.asc
Description: OpenPGP digital signature