[for-5.0] vpc: Don't round up already aligned BAT sizes
diff mbox series

Message ID 20200402093603.2369-1-kwolf@redhat.com
State New
Headers show
Series
  • [for-5.0] vpc: Don't round up already aligned BAT sizes
Related show

Commit Message

Kevin Wolf April 2, 2020, 9:36 a.m. UTC
As reported on Launchpad, Azure apparently doesn't accept images for
upload that are not both aligned to 1 MB blocks and have a BAT size that
matches the image size exactly.

As far as I can tell, there is no real reason why we create a BAT that
is one entry longer than necessary for aligned image sizes, so change
that.

(Even though the condition is only mentioned as "should" in the spec and
previous products accepted larger BATs - but we'll try to maintain
compatibility with as many of Microsoft's ever-changing interpretations
of the VHD spec as possible.)

Fixes: https://bugs.launchpad.net/bugs/1870098
Reported-by: Tobias Witek
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Philippe Mathieu-Daudé April 2, 2020, 10:45 a.m. UTC | #1
On 4/2/20 11:36 AM, Kevin Wolf wrote:
> As reported on Launchpad, Azure apparently doesn't accept images for
> upload that are not both aligned to 1 MB blocks and have a BAT size that
> matches the image size exactly.
> 
> As far as I can tell, there is no real reason why we create a BAT that
> is one entry longer than necessary for aligned image sizes, so change
> that.
> 
> (Even though the condition is only mentioned as "should" in the spec and
> previous products accepted larger BATs - but we'll try to maintain
> compatibility with as many of Microsoft's ever-changing interpretations
> of the VHD spec as possible.)
> 
> Fixes: https://bugs.launchpad.net/bugs/1870098
> Reported-by: Tobias Witek
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/vpc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index 6df75e22dc..d8141b52da 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -835,7 +835,7 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
>   
>       /* Write the footer (twice: at the beginning and at the end) */
>       block_size = 0x200000;
> -    num_bat_entries = (total_sectors + block_size / 512) / (block_size / 512);
> +    num_bat_entries = DIV_ROUND_UP(total_sectors, block_size / 512);
>   
>       ret = blk_pwrite(blk, offset, buf, HEADER_SIZE, 0);
>       if (ret < 0) {
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Max Reitz April 3, 2020, 8:55 a.m. UTC | #2
On 02.04.20 11:36, Kevin Wolf wrote:
> As reported on Launchpad, Azure apparently doesn't accept images for
> upload that are not both aligned to 1 MB blocks and have a BAT size that
> matches the image size exactly.
> 
> As far as I can tell, there is no real reason why we create a BAT that
> is one entry longer than necessary for aligned image sizes, so change
> that.
> 
> (Even though the condition is only mentioned as "should" in the spec and
> previous products accepted larger BATs - but we'll try to maintain
> compatibility with as many of Microsoft's ever-changing interpretations
> of the VHD spec as possible.)

So as far as I can tell we still don’t ensure that the image is aligned
to 1 MB blocks?

Well, as long as it’s at least possible for the user to create valid
images, that’s better.

> Fixes: https://bugs.launchpad.net/bugs/1870098
> Reported-by: Tobias Witek
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/vpc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index 6df75e22dc..d8141b52da 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -835,7 +835,7 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
>  
>      /* Write the footer (twice: at the beginning and at the end) */
>      block_size = 0x200000;
> -    num_bat_entries = (total_sectors + block_size / 512) / (block_size / 512);
> +    num_bat_entries = DIV_ROUND_UP(total_sectors, block_size / 512);

And the old code just looks like a wrong DIV_ROUND_UP() attempt.  So:

Reviewed-by: Max Reitz <mreitz@redhat.com>

>  
>      ret = blk_pwrite(blk, offset, buf, HEADER_SIZE, 0);
>      if (ret < 0) {
>
Kevin Wolf April 3, 2020, 12:04 p.m. UTC | #3
Am 03.04.2020 um 10:55 hat Max Reitz geschrieben:
> On 02.04.20 11:36, Kevin Wolf wrote:
> > As reported on Launchpad, Azure apparently doesn't accept images for
> > upload that are not both aligned to 1 MB blocks and have a BAT size that
> > matches the image size exactly.
> > 
> > As far as I can tell, there is no real reason why we create a BAT that
> > is one entry longer than necessary for aligned image sizes, so change
> > that.
> > 
> > (Even though the condition is only mentioned as "should" in the spec and
> > previous products accepted larger BATs - but we'll try to maintain
> > compatibility with as many of Microsoft's ever-changing interpretations
> > of the VHD spec as possible.)
> 
> So as far as I can tell we still don’t ensure that the image is aligned
> to 1 MB blocks?
> 
> Well, as long as it’s at least possible for the user to create valid
> images, that’s better.

Yes, we still allow other image sizes because Azure is not the only
product using VHD images, but it is the only one (to my knowledge) that
makes this requirement.

We're trying to stay compatible with at least three different Microsoft
products (VirtualPC, HyperV, Azure) that all have their own
interpretation of the spec and are inconsistent with each other.

Kevin
Max Reitz April 3, 2020, 1:09 p.m. UTC | #4
On 03.04.20 14:04, Kevin Wolf wrote:
> Am 03.04.2020 um 10:55 hat Max Reitz geschrieben:
>> On 02.04.20 11:36, Kevin Wolf wrote:
>>> As reported on Launchpad, Azure apparently doesn't accept images for
>>> upload that are not both aligned to 1 MB blocks and have a BAT size that
>>> matches the image size exactly.
>>>
>>> As far as I can tell, there is no real reason why we create a BAT that
>>> is one entry longer than necessary for aligned image sizes, so change
>>> that.
>>>
>>> (Even though the condition is only mentioned as "should" in the spec and
>>> previous products accepted larger BATs - but we'll try to maintain
>>> compatibility with as many of Microsoft's ever-changing interpretations
>>> of the VHD spec as possible.)
>>
>> So as far as I can tell we still don’t ensure that the image is aligned
>> to 1 MB blocks?
>>
>> Well, as long as it’s at least possible for the user to create valid
>> images, that’s better.
> 
> Yes, we still allow other image sizes because Azure is not the only
> product using VHD images, but it is the only one (to my knowledge) that
> makes this requirement.
> 
> We're trying to stay compatible with at least three different Microsoft
> products (VirtualPC, HyperV, Azure) that all have their own
> interpretation of the spec and are inconsistent with each other.

OK, nice.  Thanks for the explanation!

Max

Patch
diff mbox series

diff --git a/block/vpc.c b/block/vpc.c
index 6df75e22dc..d8141b52da 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -835,7 +835,7 @@  static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
 
     /* Write the footer (twice: at the beginning and at the end) */
     block_size = 0x200000;
-    num_bat_entries = (total_sectors + block_size / 512) / (block_size / 512);
+    num_bat_entries = DIV_ROUND_UP(total_sectors, block_size / 512);
 
     ret = blk_pwrite(blk, offset, buf, HEADER_SIZE, 0);
     if (ret < 0) {