Re: [Qemu-devel] [PATCH v4 5/5] qcow2-refcount: don't mask corruptions under internal errors

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2256 bytes --]

On 27.02.19 13:47, Vladimir Sementsov-Ogievskiy wrote:
> 27.02.2019 15:42, Max Reitz wrote:
>> On 14.12.18 14:42, Vladimir Sementsov-Ogievskiy wrote:
>>> No reasons for not reporting found corruptions as corruptions in case
>>> of some internal errors, especially in case of just failed to fix l2
>>> entry (and in this case, missed corruptions may influence comparing
>>> logic, when we calculate difference between corruptions fields of two
>>> results)
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/qcow2-refcount.c | 31 ++++++++++++++-----------------
>>>   1 file changed, 14 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>>> index 8da0e91dd3..b0e006e628 100644
>>> --- a/block/qcow2-refcount.c
>>> +++ b/block/qcow2-refcount.c
>>
>> [...]
>>
>>> @@ -1899,19 +1899,16 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
>>>                       fprintf(stderr, "%s OFLAG_COPIED data cluster: "
>>>                               "l2_entry=%" PRIx64 " refcount=%" PRIu64 "\n",
>>>                               repair ? "Repairing" : "ERROR", l2_entry, refcount);
>>> -                    if (repair) {
>>> -                        l2_table[j] = cpu_to_be64(refcount == 1
>>> -                                    ? l2_entry |  QCOW_OFLAG_COPIED
>>> -                                    : l2_entry & ~QCOW_OFLAG_COPIED);
>>> -                        l2_dirty++;
>>> -                    } else {
>>> -                        res->corruptions++;
>>> -                    }
>>> +                    l2_table[j] = cpu_to_be64(refcount == 1
>>> +                                ? l2_entry |  QCOW_OFLAG_COPIED
>>> +                                : l2_entry & ~QCOW_OFLAG_COPIED);
>>> +                    l2_dirty++;
>>
>> I found the old logic to be easier to understand, actually.  It made it
>> clear that if !repair, the L2 table is not going to be touched.
>>
>> Max
>>
> 
> so,
> 
>    if (repair) {
>       keep as is
>    }
>    res->corruptions++;
> 
> ok?

Yep, that works for me.  Or above the fprintf() as you did it in other
places in this patch.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]