Re: [Qemu-block] [PATCH 13/21] qcow2: add .bdrv_store_persistent_dirty_bitmaps()

[Max Reitz]

On 09.12.2016 18:55, Vladimir Sementsov-Ogievskiy wrote:
> 09.12.2016 20:05, Max Reitz wrote:
>> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
>>> Realize block bitmap storing interface, to allow qcow2 images store
>>> persistent bitmaps.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>> ---
>>>   block/qcow2-bitmap.c | 451
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   block/qcow2.c        |   1 +
>>>   block/qcow2.h        |   1 +
>>>   3 files changed, 453 insertions(+)
>>>
>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>> index 81be1ca..a975388 100644
>>> --- a/block/qcow2-bitmap.c
> 
> [...]
> 
>>> +            return;
>>> +        }
>>> +    }
>>> +
>>> +    /* check constraints and names */
>>> +    for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
>>> +            bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) {
>> Alignment to the opening parenthesis, please.
> 
> Hmm.. without an alignment it is not so simple to distinguish for-loop
> header from its body.

I know, and it's even worse for "if". That is why I usually put the
opening { on a new line if I have to spread an if/while/for header over
multiple lines.

The usual convention for qemu code is to align at an opening parenthesis
if there is one.

Admittedly, the reasoning I gave for changing checkpatch.pl to accept
opening { on a new line in certain cases was that:

(1) We never codified exactly what to allow for multi-line if/while/for
    conditions.
(2) It was existing practice.

(1) applies in your case also; we don't have any explicitly written-out
convention for alignment of wrapped lines. (2) is more difficult, but
there are indeed a handful of cases where lines are wrapped and not
aligned to the opening parenthesis but just indented by an additional
four spaces...

So I guess since I'm insisting on putting the opening { on a new line
for multi-line conditions, you are allowed to indent the consecutive
lines by an additional level. ;-)

(It *is* against existing convention, but I'm not in a position to argue.)

> [...]
> 
>> [1] What about bitmaps that have BME_FLAG_IN_USE set but do not have a
>> corresponding BDS bitmap?
>>
>> If such a bitmap does not have BME_FLAG_AUTO set, we didn't set the
>> flag, so we should keep it unchanged. That's what this function is
>> currently doing.
>>
>> However, if such a bitmap does have BME_FLAG_AUTO set, it was definitely
>> us who set the IN_USE flag (because otherwise we would have aborted
>> loading the bitmaps, and thus also aborted bdrv_open_common()).
>> Therefore, the only explanation is that the bitmap was deleted in the
>> meantime, and that means we should also delete it in the qcow2 file.
> 
> Right. Or, alternatively, these bitmaps may be deleted on corresponding
> BdrvDirtyBitmap deletion.

Right, that would work, too.

Max

signature.asc
Description: OpenPGP digital signature