Re: [PATCH v2 3/6] block/parallels: BDRVParallelsState: add cluster_size field

[Kevin Wolf]

Am 04.03.2021 um 15:57 hat Denis V. Lunev geschrieben:
> On 3/4/21 5:24 PM, Kevin Wolf wrote:
> > Am 24.02.2021 um 11:47 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> We are going to use it in more places, calculating
> >> "s->tracks << BDRV_SECTOR_BITS" doesn't look good.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >> @@ -771,6 +770,7 @@ static int parallels_open(BlockDriverState *bs, QDict 
> >> *options, int flags,
> >>          ret = -EFBIG;
> >>          goto fail;
> >>      }
> >> +    s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
> >>  
> >>      s->bat_size = le32_to_cpu(ph.bat_entries);
> >>      if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
> > Checking the context, I saw this a few lines above:
> >
> >     if (s->tracks > INT32_MAX/513) {
> >
> > Is the 513 intentional?
> >
> > Kevin
> >
> I can not remember why I have written this at that time,
> but original comment for the commit was
> 
> commit d25d59802021a747812472780d80a0e792078f40
> Author: Denis V. Lunev <den@openvz.org>
> Date:   Mon Jul 28 20:23:55 2014 +0400
> 
>     parallels: 2TB+ parallels images support
>    
>     Parallels has released in the recent updates of Parallels Server 5/6
>     new addition to his image format. Images with signature WithouFreSpacExt
>     have offsets in the catalog coded not as offsets in sectors (multiple
>     of 512 bytes) but offsets coded in blocks (i.e. header->tracks * 512)
>    
>     In this case all 64 bits of header->nb_sectors are used for image size.
>    
>     This patch implements support of this for qemu-img and also adds
> specific
>     check for an incorrect image. Images with block size greater than
>     INT_MAX/513 are not supported. The biggest available Parallels image
>     cluster size in the field is 1 Mb. Thus this limit will not hurt
>     anyone.
>    
>     Signed-off-by: Denis V. Lunev <den@openvz.org>
>     CC: Jeff Cody <jcody@redhat.com>
>     CC: Kevin Wolf <kwolf@redhat.com>
>     CC: Stefan Hajnoczi <stefanha@redhat.com>
>     Reviewed-by: Jeff Cody <jcody@redhat.com>
>     Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Thus I believe that this is intentional.

Hm, fair. It's a weird number. I would have guessed a typo, but if it's
in the commit message as well, it might be intentional. Or just a typo
combined with copy & paste.

If we ever remember or find a new reason why it has to be 513 rather
than 512, adding a comment would be nice.

Kevin