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

[Alexander Ivanov]

On 26.08.2022 15:08, Denis V. Lunev wrote:
> On 25.08.2022 16:31, 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>
> > ---
> >   block/parallels.c | 59 ++++++++++++++++++++++++++++++++++-------------
> >   1 file changed, 43 insertions(+), 16 deletions(-)
> >
> > diff --git a/block/parallels.c b/block/parallels.c
> > index eea318f809..f50cd232aa 100644
> > --- a/block/parallels.c
> > +++ b/block/parallels.c
> > @@ -438,13 +438,50 @@ static void 
> > parallels_check_unclean(BlockDriverState *bs,
> >       }
> >   }
> >   +static int parallels_check_outside_image(BlockDriverState *bs,
> > +                                         BdrvCheckResult *res,
> > +                                         BdrvCheckMode fix)
> > +{
> > +    BDRVParallelsState *s = bs->opaque;
> > +    uint32_t i;
> > +    int64_t off, high_off, size;
> > +
> > +    size = bdrv_getlength(bs->file->bs);
> > +    if (size < 0) {
> > +        res->check_errors++;
> > +        return size;
> > +    }
> > +
> > +    high_off = 0;
> > +    for (i = 0; i < s->bat_size; i++) {
> > +        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> > +        if (off > size) {
>
> we need one more patch here. Correct check would be
>
> if (off >= size) {
>   bla-bla()
> }
I have such a patch in the next patchset.
> > +            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++;
> > +            }
> > +            continue;
> > +        }
> > +        if (high_off < off) {
> > +            high_off = off;
> > +        }
> > +    }
> > +
> > +    s->data_end = (high_off + s->cluster_size) >> BDRV_SECTOR_BITS;
> > +
> > +    return 0;
> > +}
> > +
> >   static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> >                                              BdrvCheckResult *res,
> >                                              BdrvCheckMode fix)
> >   {
> >       BDRVParallelsState *s = bs->opaque;
> >       int64_t size, prev_off, high_off;
> > -    int ret = 0;
> > +    int ret;
> >       uint32_t i;
> >         size = bdrv_getlength(bs->file->bs);
> > @@ -457,6 +494,11 @@ static int coroutine_fn 
> > parallels_co_check(BlockDriverState *bs,
> >         parallels_check_unclean(bs, res, fix);
> >   +    ret = parallels_check_outside_image(bs, res, fix);
> > +    if (ret < 0) {
> > +        goto out;
> > +    }
> > +
> >       res->bfi.total_clusters = s->bat_size;
> >       res->bfi.compressed_clusters = 0; /* compression is not 
> > supported */
> >   @@ -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;
> > @@ -519,8 +548,6 @@ static int coroutine_fn 
> > parallels_co_check(BlockDriverState *bs,
> >           }
> >       }
> >   -    s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
> > -
> >   out:
> >       qemu_co_mutex_unlock(&s->lock);
> Reviewed-by: Denis V. Lunev <den@openvz.org>