Re: [PATCH 2/9] parallels: Move check of cluster outside image to a separate function

[Denis V. Lunev]

On 08.08.2022 14:19, Denis V. Lunev wrote:
> On 08.08.2022 14:07, Alexander Ivanov wrote:
> > Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> > ---
> >   block/parallels.c | 76 +++++++++++++++++++++++++++++++++--------------
> >   1 file changed, 53 insertions(+), 23 deletions(-)
> >
> > diff --git a/block/parallels.c b/block/parallels.c
> > index 108aa907b8..7b400ecdcc 100644
> > --- a/block/parallels.c
> > +++ b/block/parallels.c
> > @@ -413,6 +413,13 @@ static coroutine_fn int 
> > parallels_co_readv(BlockDriverState *bs,
> >       return ret;
> >   }
> >   +static void parallels_set_bat_entry(BDRVParallelsState *s,
> > +                                    uint32_t index, uint32_t offset)
> > +{
> > +    s->bat_bitmap[index] = offset;
> > +    bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / 
> > s->bat_dirty_block, 1);
> > +}
> > +
> >   static void parallels_check_unclean(BlockDriverState *bs,
> >                                       BdrvCheckResult *res,
> >                                       BdrvCheckMode fix)
>
> This helper seems unrelated to the patch subject and should be done 
> separately.

more specifically - you have created the helper BUT you have not used
it at ALL places where we update BAT. That is seriously wrong from the
code completness POW. If you have getter - you MUST use it everywhere,
even within allocate_clusters().

Normally you create helper, f.e. refactoring code of allocate cluster 
promising
that the code will be reused later (non-functional changes) and after that
in the separate patch you spread its usage refactoring other code, f.e.
to avoid flush_bat tracking.