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 <vsement...@virtuozzo.com>
>>> ---
>>> 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
