Re: [Qemu-block] [PATCH 7/8] block/backup: support bitmap sync modes for non-bitmap backups

[Max Reitz]

On 10.07.19 20:32, John Snow wrote:
> 
> 
> On 7/10/19 12:48 PM, Max Reitz wrote:
>> On 10.07.19 03:05, John Snow wrote:
>>> Accept bitmaps and sync policies for the other backup modes.
>>> This allows us to do things like create a bitmap synced to a full backup
>>> without a transaction, or start a resumable backup process.
>>>
>>> Some combinations don't make sense, though:
>>>
>>> - NEVER policy combined with any non-BITMAP mode doesn't do anything,
>>>   because the bitmap isn't used for input or output.
>>>   It's harmless, but is almost certainly never what the user wanted.
>>>
>>> - sync=NONE is more questionable. It can't use on-success because this
>>>   job never completes with success anyway, and the resulting artifact
>>>   of 'always' is suspect: because we start with a full bitmap and only
>>>   copy out segments that get written to, the final output bitmap will
>>>   always be ... a fully set bitmap.
>>>
>>>   Maybe there's contexts in which bitmaps make sense for sync=none,
>>>   but not without more severe changes to the current job, and omitting
>>>   it here doesn't prevent us from adding it later.
>>>
>>> Signed-off-by: John Snow <address@hidden>
>>> ---
>>>  block/backup.c       |  8 +-------
>>>  blockdev.c           | 22 ++++++++++++++++++++++
>>>  qapi/block-core.json |  6 ++++--
>>>  3 files changed, 27 insertions(+), 9 deletions(-)
>>
>> [...]
>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index f0b7da53b0..bc152f8e0d 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>
>> [...]
>>
>>> +    if (!backup->has_bitmap && backup->has_bitmap_mode) {
>>> +        error_setg(errp, "Cannot specify Bitmap sync mode without a 
>>> bitmap");
>>
>> Any reason for capitalizing the first “Bitmap”?
>>
>> With a reason or it fixed:
>>
>> Reviewed-by: Max Reitz <address@hidden>
>>
> 
> Hanging around Germans too much?

You should know then that the korrekt way to write it would be:

„Specifying Binarydigitmapsynchronizationmode without a Binarydigitmap
is absolutely verboten!“

> Actually, I can explain why: because a "bitmap" is a generic term, but
> whenever I capitalize it as "Bitmap" I am referring to a Block Dirty
> Bitmap which is a specific sort of thing. I do this unconsciously.
> 
> But in that case, I should actually be consistent in the interface (and
> docstrings and docs and error strings) and always call it that specific
> thing, which I don't.
> 
> "bitmap" is probably more correct for now, but I ought to go through all
> the interface and make it consistent in some way or another.
> 
> 
> (Actually: I'd like to see if I can't rename the "BdrvDirtyBitmap"
> object to something more generic and shorter so I can rename many of the
> functions we have something shorter.

Well, BDB is free.  That path has worked fine for BlockDriverStates.

Or what I said on IRC, but you know.

> Because the structure is "BdrvDirtyBitmap", there's some confusion when
> we name functions like bdrv_dirty_bitmap_{verb} because it's not clear
> if this is a bdrv function that does something with dirty bitmaps, or if
> it's a "BdrvDirtyBitmap" function that does something with that object.
> 
> I guess that seems like a subtle point, but it's why the naming
> conventions in dirty-bitmap.c are all over the place. I think at one
> point, the idea was that:
> 
> bdrv_{verb}_dirty_bitmap was an action applied to a BDS that happened to
> do something with dirty bitmaps. bdrv_dirty_bitmap_{verb} was an action
> that applied to a "BdrvDirtyBitmap". Crystal clear and not confusing at
> all, right?

I just thought people named their functions whatever they felt like at
the time.

> It'd be nice to have functions that operate on a node be named
> bdrv_dbitmap_foo() and functions that operate on the bitmap structure
> itself named just dbitmap_bar().
> 
> Would it be okay if I named them such a thing, I wonder?
> 
> we have "bitmap" and "hbitmap" already, I could do something like
> "dbitmap" for "dirty bitmap" or some such. Kind of an arbitrary change I
> admit, but I'm itching to do a big spring cleaning in dirty-bitmap.c
> right after this series is done.)

HBitmaps are generally used to track dirty areas, so I’d find this a
misnomer.  BDBitmap would be OK.  The “block” part should be in there
somewhere.

Max

signature.asc
Description: OpenPGP digital signature