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

[Alexander Ivanov <alexander.ivanov@virtuozzo.com>]

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