Re: [Qemu-devel] [PATCH 2/5] block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2376 bytes --]

On 6/7/19 1:10 PM, John Snow wrote:
> 
> 
> On 6/7/19 10:29 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 06.06.2019 21:41, John Snow wrote:
>>> Instead of bdrv_can_store_new_bitmap, rework this as
>>> bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry
>>> with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free
>>> to modify the driver state because we know we ARE adding a bitmap
>>> instead of simply being asked if we CAN store one.
>>>
>>> As part of this symmetry, move this function next to
>>> bdrv_remove_persistent_bitmap in block/dirty-bitmap.c.
>>>
>>> To cement this semantic distinction, use a bitmap object instead of the
>>> (name, granularity) pair as this allows us to set persistence as a
>>> condition of the "add" succeeding.
>>>
>>> Notably, the qcow2 implementation still does not actually store the new
>>> bitmap at add time, but merely ensures that it will be able to at store
>>> time.
>>>

>>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
>>> +                                     BdrvDirtyBitmap *bitmap,
>>> +                                     Error **errp)
>>
>> strange thing for me: "bitmap" in function name is _not_ the same
>> thing as @bitmap argument.. as if it the same,
>> function adds "persistent bitmap", but @bitmap is not persistent yet,
>> so may be function, don't add persistent bitmap, but marks bitmap persistent?
>>
>>
>> Ok, I can think of it like "add persistent dirty bitmap to driver. if succeed, set
>> bitmap.persistent flag"
>>
> 
> Yeah, I meant "Add bitmap to file", where the persistence is implied
> because it's part of the file. The bitmap is indeed not YET persistent,
> but becomes so as a condition of this call succeeding.
> 
> Do you have a suggestion for a better name? I liked the symmetry with
> 'remove' so that there was an obvious "add bitmap" and "remove bitmap".
> 
> Of course, when you remove, it is indeed persistent, so
> "remove_persistent_dirty_bitmap" makes sense there.
> 

Or maybe even merely 'bdrv_add_dirty_bitmap' with doc comments stating
that it associates an existing non-persistent bitmap with the bdrv
storage and marks it persistent if successful.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]