Re: [Qemu-devel] [PATCH 1/2] qapi: support external bitmaps in block-dirty-bitmap-merge

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

17.05.2019 16:50, Eric Blake wrote:
> On 5/16/19 7:32 PM, John Snow wrote:
>>
>>
>> On 5/16/19 8:27 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Add new optional parameter making possible to merge bitmaps from
>>> different nodes. It is needed to maintain external snapshots during
>>> incremental backup chain history.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   qapi/block-core.json | 13 ++++++++++---
>>>   block/dirty-bitmap.c |  9 ++++++---
>>>   blockdev.c           | 46 ++++++++++++++++++++++++++++++--------------
>>>   3 files changed, 48 insertions(+), 20 deletions(-)
> 
>>> -# @bitmaps: name(s) of the source dirty bitmap(s)
>>> +# @bitmaps: name(s) of the source dirty bitmap(s). The field is optional
>>> +#           since 4.1.
>>> +#
>>> +# @external-bitmaps: additional list of source dirty bitmaps with specified
>>> +#                    nodes, which allows merging bitmaps between different
>>> +#                    nodes. (Since: 4.1)
>>>   #
>>>   # Since: 4.0
>>>   ##
>>>   { 'struct': 'BlockDirtyBitmapMerge',
>>> -  'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } }
>>> +  'data': { 'node': 'str', 'target': 'str', '*bitmaps': ['str'],
>>> +            '*external-bitmaps': ['BlockDirtyBitmap'] } }
>>>   
>>
>> I guess you can specify one, or both, or maybe neither! Seems fine.
> 
> 
>>
>> I don't think I like the name "external-bitmaps" but I guess I don't
>> really have a better suggestion.
> 
> I do - we could use an alternate type instead:
> 
> { 'alternate': 'BitmapSource',
>    'data': { 'local': 'str',
>               'external': 'BlockDirtyBitmap' } }
> 
> then use 'bitmaps': ['BitmapSource']
> 
> so that the caller can pass:
> 
> "bitmaps": [ "bitmap1", { "node": "other", "name", "bitmap2" } ]
> 
> and we only have to deal with one array at all times, and not have the
> name 'external-bitmaps' to worry about.
> 

Oh, I wanted to do something like this, but looked at union type,  which also needs some
discriminator field, and decided that it's impossible to make it backward-compatible.

Will resend.



-- 
Best regards,
Vladimir