Re: [PATCH v4 3/5] parallels: Add checking and repairing duplicate offsets in BAT

[Alexander Ivanov]

Yes, there is a bug. Thank you.

On 4/29/23 00:15, Mike Maslenkin wrote:
> There is another issue with host_cluster_index() function.
> After this patchset applied `qemu-img check -f parallels  some_disk`
> aborts for  empty (just created) disk image.
> The problem is that host_cluster_index() returns 0 and then
> bitmap_new(0) rises an abort.
>
> For default empty disk s->header->data_off=2048, and
> res->image_end_offset = 1048576, so:
> static uint32_t host_cluster_index(BDRVParallelsState *s, int64_t 1048576)
> {
>    off = 1048576  - le32_to_cpu(2048) << 9;
>    return 0 / 1048576;
> }
>
> Could you check this case?
>
> Regards,
> Mike.
>
> On Thu, Apr 27, 2023 at 3:29 PM Alexander Ivanov
> <alexander.ivanov@virtuozzo.com> wrote:
> > Good point. Thank you.
> >
> > Best regards,
> > Alexander Ivanov
> >
> > On 4/26/23 23:56, Mike Maslenkin wrote:
> > > On Mon, Apr 24, 2023 at 12:44 PM Alexander Ivanov
> > > <alexander.ivanov@virtuozzo.com> wrote:
> > > > Cluster offsets must be unique among all the BAT entries. Find duplicate
> > > > offsets in the BAT and fix it by copying the content of the relevant
> > > > cluster to a newly allocated cluster and set the new cluster offset to the
> > > > duplicated entry.
> > > >
> > > > Add host_cluster_index() helper to deduplicate the code.
> > > >
> > > > Move parallels_fix_leak() call to parallels_co_check() to fix both types
> > > > of leak: real corruption and a leak produced by allocate_clusters()
> > > > during deduplication.
> > > >
> > > > Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> > > > ---
> > > >    block/parallels.c | 134 ++++++++++++++++++++++++++++++++++++++++++++--
> > > >    1 file changed, 129 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/block/parallels.c b/block/parallels.c
> > > > index ec89ed894b..3b992e8173 100644
> > > > --- a/block/parallels.c
> > > > +++ b/block/parallels.c
> > > > @@ -136,6 +136,12 @@ static int cluster_remainder(BDRVParallelsState *s, 
> > > > int64_t sector_num,
> > > >        return MIN(nb_sectors, ret);
> > > >    }
> > > >
> > > > +static uint32_t host_cluster_index(BDRVParallelsState *s, int64_t off)
> > > > +{
> > > > +    off -= s->header->data_off << BDRV_SECTOR_BITS;
> > > > +    return off / s->cluster_size;
> > > > +}
> > > > +
> > > I guess  there should be:
> > > off -= le32_to_cpu(s->header->data_off) << BDRV_SECTOR_BITS
> > >
> > > Regards,
> > > Mike.

--
Best regards,
Alexander Ivanov