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

[John Snow <jsnow@redhat.com>]

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 <jsnow@redhat.com>
>> ---
>>  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 <mreitz@redhat.com>
> 

Hanging around Germans too much?

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.

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?

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