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 <[email protected]> >>> --- >>> 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
