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

[Hanna Czenczek]

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