Re: [PATCH v8 07/11] parallels: Move check of cluster outside image to a separate function

[Alexander Ivanov <alexander.ivanov@virtuozzo.com>]

On 18.01.2023 15:45, Hanna Czenczek wrote:
> On 15.01.23 16:58, Alexander Ivanov wrote:
>> We will add more and more checks so we need a better code structure
>> in parallels_co_check. Let each check performs in a separate loop
>> in a separate helper.
>>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> Reviewed-by: Denis V. Lunev <den@openvz.org>
>> ---
>>   block/parallels.c | 59 ++++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 43 insertions(+), 16 deletions(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index d48b447cca..3d06623355 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>
> [...]
>
>> @@ -469,19 +511,6 @@ static int coroutine_fn 
>> parallels_co_check(BlockDriverState *bs,
>>               continue;
>>           }
>>   -        /* cluster outside the image */
>> -        if (off > size) {
>> -            fprintf(stderr, "%s cluster %u is outside image\n",
>> -                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
>> -            res->corruptions++;
>> -            if (fix & BDRV_FIX_ERRORS) {
>> -                parallels_set_bat_entry(s, i, 0);
>> -                res->corruptions_fixed++;
>> -            }
>> -            prev_off = 0;
>> -            continue;
>> -        }
>> -
>>           res->bfi.allocated_clusters++;
>>           if (off > high_off) {
>>               high_off = off;
>
> parallels_co_check() keeps the `high_off` variable, and now it is also 
> bumped for clusters that are outside the image.  This seems to go 
> against patch 2’s intentions.
>
> Consider an image whose file length is larger than all of its clusters 
> need (i.e. there’s leaked space), except for one cluster, which is 
> beyond the EOF.  This one cluster is considered an error (because it’s 
> outside the image).  Before this patch, we would have truncated the 
> image’s file length to match all the other non-error clusters (and 
> drop the leaked space).  With this patch, the error cluster, if it 
> wasn’t fixed by parallels_check_outside_image(), the image’s file 
> length is no longer truncated.  Basically, this seems to restore the 
> behavior from before patch 2.
>
> Was this intentional?
>
> Hanna
>
Good point!
I have missed the case with !BDRV_FIX_ERRORS.
Thank you!