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

[Mike Maslenkin]

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.
>