[PATCH 0/9] block/vpc: Clean up some buffer abuse

[PATCH 0/9] block/vpc: Clean up some buffer abuse

[Markus Armbruster]

Markus Armbruster (9):
  block/vpc: Make vpc_open() read the full dynamic header
  block/vpc: Don't abuse the footer buffer as BAT sector buffer
  block/vpc: Don't abuse the footer buffer for dynamic header
  block/vpc: Make vpc_checksum() take void *
  block/vpc: Pad VHDDynDiskHeader, replace uint8_t[] buffers
  block/vpc: Use sizeof() instead of 1024 for dynamic header size
  block/vpc: Pad VHDFooter, replace uint8_t[] buffers
  block/vpc: Pass footer buffers as VHDFooter * instead of uint8_t *
  block/vpc: Use sizeof() instead of HEADER_SIZE for footer size

 block/vpc.c | 146 ++++++++++++++++++++++++++--------------------------
 1 file changed, 72 insertions(+), 74 deletions(-)

-- 
2.26.2

[PATCH 1/9] block/vpc: Make vpc_open() read the full dynamic header

[Markus Armbruster]

The dynamic header's size is 1024 bytes.

vpc_open() reads only the 512 bytes of the dynamic header into buf[].
Works, because it doesn't actually access the second half.  However, a
colleague told me that GCC 11 warns:

    ../block/vpc.c:358:51: error: array subscript 'struct VHDDynDiskHeader[0]' is partly outside array bounds of 'uint8_t[512]' [-Werror=array-bounds]

Clean up to read the full header.

Rename buf[] to dyndisk_header_buf[] while there.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/vpc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 1ab55f9287..2fcf3f6283 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -220,7 +220,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
     QemuOpts *opts = NULL;
     Error *local_err = NULL;
     bool use_chs;
-    uint8_t buf[HEADER_SIZE];
+    uint8_t dyndisk_header_buf[1024];
     uint32_t checksum;
     uint64_t computed_size;
     uint64_t pagetable_size;
@@ -340,14 +340,14 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     if (disk_type == VHD_DYNAMIC) {
-        ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf,
-                         HEADER_SIZE);
+        ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset),
+                         dyndisk_header_buf, 1024);
         if (ret < 0) {
             error_setg(errp, "Error reading dynamic VHD header");
             goto fail;
         }
 
-        dyndisk_header = (VHDDynDiskHeader *) buf;
+        dyndisk_header = (VHDDynDiskHeader *)dyndisk_header_buf;
 
         if (strncmp(dyndisk_header->magic, "cxsparse", 8)) {
             error_setg(errp, "Invalid header magic");
-- 
2.26.2

Re: [PATCH 1/9] block/vpc: Make vpc_open() read the full dynamic header

[Max Reitz]

On 17.12.20 17:19, Markus Armbruster wrote:
> The dynamic header's size is 1024 bytes.
> 
> vpc_open() reads only the 512 bytes of the dynamic header into buf[].
> Works, because it doesn't actually access the second half.  However, a
> colleague told me that GCC 11 warns:
> 
>      ../block/vpc.c:358:51: error: array subscript 'struct VHDDynDiskHeader[0]' is partly outside array bounds of 'uint8_t[512]' [-Werror=array-bounds]
> 
> Clean up to read the full header.
> 
> Rename buf[] to dyndisk_header_buf[] while there.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   block/vpc.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index 1ab55f9287..2fcf3f6283 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -220,7 +220,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>       QemuOpts *opts = NULL;
>       Error *local_err = NULL;
>       bool use_chs;
> -    uint8_t buf[HEADER_SIZE];
> +    uint8_t dyndisk_header_buf[1024];

Perhaps this should be heap-allocated, but I don’t know whether qemu has 
ever established a (perhaps just inofficial) threshold on what goes on 
the stack and what goes on the heap.

>       uint32_t checksum;
>       uint64_t computed_size;
>       uint64_t pagetable_size;
> @@ -340,14 +340,14 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>       }
>   
>       if (disk_type == VHD_DYNAMIC) {
> -        ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf,
> -                         HEADER_SIZE);
> +        ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset),
> +                         dyndisk_header_buf, 1024);

sizeof() would be better, but I see that’s what patch 6 is for.

>           if (ret < 0) {
>               error_setg(errp, "Error reading dynamic VHD header");
>               goto fail;
>           }
>   
> -        dyndisk_header = (VHDDynDiskHeader *) buf;
> +        dyndisk_header = (VHDDynDiskHeader *)dyndisk_header_buf;
>   
>           if (strncmp(dyndisk_header->magic, "cxsparse", 8)) {
>               error_setg(errp, "Invalid header magic");

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

Re: [PATCH 1/9] block/vpc: Make vpc_open() read the full dynamic header

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 17.12.20 17:19, Markus Armbruster wrote:
>> The dynamic header's size is 1024 bytes.
>> 
>> vpc_open() reads only the 512 bytes of the dynamic header into buf[].
>> Works, because it doesn't actually access the second half.  However, a
>> colleague told me that GCC 11 warns:
>> 
>>      ../block/vpc.c:358:51: error: array subscript 'struct VHDDynDiskHeader[0]' is partly outside array bounds of 'uint8_t[512]' [-Werror=array-bounds]
>> 
>> Clean up to read the full header.
>> 
>> Rename buf[] to dyndisk_header_buf[] while there.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   block/vpc.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/block/vpc.c b/block/vpc.c
>> index 1ab55f9287..2fcf3f6283 100644
>> --- a/block/vpc.c
>> +++ b/block/vpc.c
>> @@ -220,7 +220,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>>       QemuOpts *opts = NULL;
>>       Error *local_err = NULL;
>>       bool use_chs;
>> -    uint8_t buf[HEADER_SIZE];
>> +    uint8_t dyndisk_header_buf[1024];
>
> Perhaps this should be heap-allocated, but I don’t know whether qemu has 
> ever established a (perhaps just inofficial) threshold on what goes on 
> the stack and what goes on the heap.

There is no official per-function limit.  I generally don't worry about
a few kilobytes unless it's recursive.  We have many, many functions
with stack frames in the order of a kilobyte.  Our coroutine and thread
stacks are reasonable (corotine stacks are 1MiB, the default thread
stack size 2MiB on x86-64, and I can't see code that sets a different
size).

>>       uint32_t checksum;
>>       uint64_t computed_size;
>>       uint64_t pagetable_size;
>> @@ -340,14 +340,14 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>>       }
>>   
>>       if (disk_type == VHD_DYNAMIC) {
>> -        ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf,
>> -                         HEADER_SIZE);
>> +        ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset),
>> +                         dyndisk_header_buf, 1024);
>
> sizeof() would be better, but I see that’s what patch 6 is for.

Yes, but sizeof what?  VHDDynDiskHeader becomes usable for that only in
PATCH 5.

I chose to separate the buffers first, and only then tidy up their use.

>>           if (ret < 0) {
>>               error_setg(errp, "Error reading dynamic VHD header");
>>               goto fail;
>>           }
>>   
>> -        dyndisk_header = (VHDDynDiskHeader *) buf;
>> +        dyndisk_header = (VHDDynDiskHeader *)dyndisk_header_buf;
>>   
>>           if (strncmp(dyndisk_header->magic, "cxsparse", 8)) {
>>               error_setg(errp, "Invalid header magic");
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Thanks!

Re: [PATCH 1/9] block/vpc: Make vpc_open() read the full dynamic header

[Max Reitz]

On 18.12.20 14:49, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> On 17.12.20 17:19, Markus Armbruster wrote:
>>> The dynamic header's size is 1024 bytes.
>>>
>>> vpc_open() reads only the 512 bytes of the dynamic header into buf[].
>>> Works, because it doesn't actually access the second half.  However, a
>>> colleague told me that GCC 11 warns:
>>>
>>>       ../block/vpc.c:358:51: error: array subscript 'struct VHDDynDiskHeader[0]' is partly outside array bounds of 'uint8_t[512]' [-Werror=array-bounds]
>>>
>>> Clean up to read the full header.
>>>
>>> Rename buf[] to dyndisk_header_buf[] while there.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>    block/vpc.c | 8 ++++----
>>>    1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/block/vpc.c b/block/vpc.c
>>> index 1ab55f9287..2fcf3f6283 100644
>>> --- a/block/vpc.c
>>> +++ b/block/vpc.c
>>> @@ -220,7 +220,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>>>        QemuOpts *opts = NULL;
>>>        Error *local_err = NULL;
>>>        bool use_chs;
>>> -    uint8_t buf[HEADER_SIZE];
>>> +    uint8_t dyndisk_header_buf[1024];
>>
>> Perhaps this should be heap-allocated, but I don’t know whether qemu has
>> ever established a (perhaps just inofficial) threshold on what goes on
>> the stack and what goes on the heap.
> 
> There is no official per-function limit.  I generally don't worry about
> a few kilobytes unless it's recursive.  We have many, many functions
> with stack frames in the order of a kilobyte.

Which doesn’t mean it’s what we want.  But I suppose in respect to my 
implicit question that means we don’t have any hard limits.

> Our coroutine and thread
> stacks are reasonable (corotine stacks are 1MiB, the default thread
> stack size 2MiB on x86-64, and I can't see code that sets a different
> size).

I’m not too worried about some on-stack buffers in functions like *open 
and *create.  It’s just that I would have put it on the heap, probably.

(Speaking of coroutine stack sizes, I remember a recent bug report on 
how long a backing chain may become, and that one of the limiting 
factors is that the coroutine stack eventually overflows. *shrug*)

>>>        uint32_t checksum;
>>>        uint64_t computed_size;
>>>        uint64_t pagetable_size;
>>> @@ -340,14 +340,14 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>>>        }
>>>    
>>>        if (disk_type == VHD_DYNAMIC) {
>>> -        ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf,
>>> -                         HEADER_SIZE);
>>> +        ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset),
>>> +                         dyndisk_header_buf, 1024);
>>
>> sizeof() would be better, but I see that’s what patch 6 is for.
> 
> Yes, but sizeof what?  VHDDynDiskHeader becomes usable for that only in
> PATCH 5.

sizeof(dyndisk_header_buf).

> I chose to separate the buffers first, and only then tidy up their use.

Understood.  It’s just that I noticed, then looked further in the patch 
series to see whether you’d clean up the magic number, and indeed you 
did in patch 6. :)

Max

[PATCH 2/9] block/vpc: Don't abuse the footer buffer as BAT sector buffer

[Markus Armbruster]

create_dynamic_disk() takes a buffer holding the footer as first
argument.  It writes out the footer (512 bytes), then reuses the
buffer to initialize and write out the dynamic header (1024 bytes),
then reuses it again to initialize and write out BAT sectors (512).

Works, because the caller passes a buffer that is large enough for all
three purposes.  I hate that.

Use a separate buffer for writing out BAT sectors.  The next commit
will do the same for the dynamic header.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/vpc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 2fcf3f6283..d18ecc3da1 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -824,6 +824,7 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
 {
     VHDDynDiskHeader *dyndisk_header =
         (VHDDynDiskHeader *) buf;
+    uint8_t bat_sector[512];
     size_t block_size, num_bat_entries;
     int i;
     int ret;
@@ -847,9 +848,9 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
     /* Write the initial BAT */
     offset = 3 * 512;
 
-    memset(buf, 0xFF, 512);
+    memset(bat_sector, 0xFF, 512);
     for (i = 0; i < DIV_ROUND_UP(num_bat_entries * 4, 512); i++) {
-        ret = blk_pwrite(blk, offset, buf, 512, 0);
+        ret = blk_pwrite(blk, offset, bat_sector, 512, 0);
         if (ret < 0) {
             goto fail;
         }
-- 
2.26.2

Re: [PATCH 2/9] block/vpc: Don't abuse the footer buffer as BAT sector buffer

[Max Reitz]

On 17.12.20 17:19, Markus Armbruster wrote:
> create_dynamic_disk() takes a buffer holding the footer as first
> argument.  It writes out the footer (512 bytes), then reuses the
> buffer to initialize and write out the dynamic header (1024 bytes),
> then reuses it again to initialize and write out BAT sectors (512).
> 
> Works, because the caller passes a buffer that is large enough for all
> three purposes.  I hate that.
> 
> Use a separate buffer for writing out BAT sectors.  The next commit
> will do the same for the dynamic header.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   block/vpc.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)

(Still wondering about whether such buffers should go on the stack, but:)

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

[PATCH 3/9] block/vpc: Don't abuse the footer buffer for dynamic header

[Markus Armbruster]

create_dynamic_disk() takes a buffer holding the footer as first
argument.  It writes out the footer (512 bytes), then reuses the
buffer to initialize and write out the dynamic header (1024 bytes).

Works, because the caller passes a buffer that is large enough for
both purposes.  I hate that.

Use a separate buffer for the dynamic header, and adjust the caller's
buffer.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/vpc.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index d18ecc3da1..34186640ee 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -822,8 +822,9 @@ static int calculate_geometry(int64_t total_sectors, uint16_t *cyls,
 static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
                                int64_t total_sectors)
 {
+    uint8_t dyndisk_header_buf[1024];
     VHDDynDiskHeader *dyndisk_header =
-        (VHDDynDiskHeader *) buf;
+        (VHDDynDiskHeader *)dyndisk_header_buf;
     uint8_t bat_sector[512];
     size_t block_size, num_bat_entries;
     int i;
@@ -858,7 +859,7 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
     }
 
     /* Prepare the Dynamic Disk Header */
-    memset(buf, 0, 1024);
+    memset(dyndisk_header_buf, 0, 1024);
 
     memcpy(dyndisk_header->magic, "cxsparse", 8);
 
@@ -872,12 +873,13 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
     dyndisk_header->block_size = cpu_to_be32(block_size);
     dyndisk_header->max_table_entries = cpu_to_be32(num_bat_entries);
 
-    dyndisk_header->checksum = cpu_to_be32(vpc_checksum(buf, 1024));
+    dyndisk_header->checksum = cpu_to_be32(vpc_checksum(dyndisk_header_buf,
+                                                        1024));
 
     /* Write the header */
     offset = 512;
 
-    ret = blk_pwrite(blk, offset, buf, 1024, 0);
+    ret = blk_pwrite(blk, offset, dyndisk_header_buf, 1024, 0);
     if (ret < 0) {
         goto fail;
     }
@@ -972,8 +974,8 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
     BlockBackend *blk = NULL;
     BlockDriverState *bs = NULL;
 
-    uint8_t buf[1024];
-    VHDFooter *footer = (VHDFooter *) buf;
+    uint8_t footer_buf[HEADER_SIZE];
+    VHDFooter *footer = (VHDFooter *)footer_buf;
     uint16_t cyls = 0;
     uint8_t heads = 0;
     uint8_t secs_per_cyl = 0;
@@ -1036,7 +1038,7 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
     }
 
     /* Prepare the Hard Disk Footer */
-    memset(buf, 0, 1024);
+    memset(footer_buf, 0, HEADER_SIZE);
 
     memcpy(footer->creator, "conectix", 8);
     if (vpc_opts->force_size) {
@@ -1069,15 +1071,15 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
     qemu_uuid_generate(&uuid);
     footer->uuid = uuid;
 
-    footer->checksum = cpu_to_be32(vpc_checksum(buf, HEADER_SIZE));
+    footer->checksum = cpu_to_be32(vpc_checksum(footer_buf, HEADER_SIZE));
 
     if (disk_type == VHD_DYNAMIC) {
-        ret = create_dynamic_disk(blk, buf, total_sectors);
+        ret = create_dynamic_disk(blk, footer_buf, total_sectors);
         if (ret < 0) {
             error_setg(errp, "Unable to create or write VHD header");
         }
     } else {
-        ret = create_fixed_disk(blk, buf, total_size, errp);
+        ret = create_fixed_disk(blk, footer_buf, total_size, errp);
     }
 
 out:
-- 
2.26.2

Re: [PATCH 3/9] block/vpc: Don't abuse the footer buffer for dynamic header

[Max Reitz]

On 17.12.20 17:19, Markus Armbruster wrote:
> create_dynamic_disk() takes a buffer holding the footer as first
> argument.  It writes out the footer (512 bytes), then reuses the
> buffer to initialize and write out the dynamic header (1024 bytes).
> 
> Works, because the caller passes a buffer that is large enough for
> both purposes.  I hate that.
> 
> Use a separate buffer for the dynamic header, and adjust the caller's
> buffer.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   block/vpc.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index d18ecc3da1..34186640ee 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -822,8 +822,9 @@ static int calculate_geometry(int64_t total_sectors, uint16_t *cyls,
>   static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
>                                  int64_t total_sectors)
>   {
> +    uint8_t dyndisk_header_buf[1024];
>       VHDDynDiskHeader *dyndisk_header =
> -        (VHDDynDiskHeader *) buf;
> +        (VHDDynDiskHeader *)dyndisk_header_buf;
>       uint8_t bat_sector[512];
>       size_t block_size, num_bat_entries;
>       int i;
> @@ -858,7 +859,7 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
>       }
>   
>       /* Prepare the Dynamic Disk Header */
> -    memset(buf, 0, 1024);
> +    memset(dyndisk_header_buf, 0, 1024);
>   
>       memcpy(dyndisk_header->magic, "cxsparse", 8);

I’d prefer something like

*dyndisk_header = (VHDDynDiskHeader){
     ...
};

but that isn’t possible before patch 5.  (And can be done on top now 
anyway, especially given that Kevin probably wants to send a pull 
request today.)

[...]

> @@ -972,8 +974,8 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
>       BlockBackend *blk = NULL;
>       BlockDriverState *bs = NULL;
>   
> -    uint8_t buf[1024];
> -    VHDFooter *footer = (VHDFooter *) buf;
> +    uint8_t footer_buf[HEADER_SIZE];
> +    VHDFooter *footer = (VHDFooter *)footer_buf;
>       uint16_t cyls = 0;
>       uint8_t heads = 0;
>       uint8_t secs_per_cyl = 0;
> @@ -1036,7 +1038,7 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
>       }
>   
>       /* Prepare the Hard Disk Footer */
> -    memset(buf, 0, 1024);
> +    memset(footer_buf, 0, HEADER_SIZE);
>   
>       memcpy(footer->creator, "conectix", 8);

Same here, except here it’s patch 7.

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

Re: [PATCH 3/9] block/vpc: Don't abuse the footer buffer for dynamic header

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 17.12.20 17:19, Markus Armbruster wrote:
>> create_dynamic_disk() takes a buffer holding the footer as first
>> argument.  It writes out the footer (512 bytes), then reuses the
>> buffer to initialize and write out the dynamic header (1024 bytes).
>> Works, because the caller passes a buffer that is large enough for
>> both purposes.  I hate that.
>> Use a separate buffer for the dynamic header, and adjust the
>> caller's
>> buffer.
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   block/vpc.c | 22 ++++++++++++----------
>>   1 file changed, 12 insertions(+), 10 deletions(-)
>> diff --git a/block/vpc.c b/block/vpc.c
>> index d18ecc3da1..34186640ee 100644
>> --- a/block/vpc.c
>> +++ b/block/vpc.c
>> @@ -822,8 +822,9 @@ static int calculate_geometry(int64_t total_sectors, uint16_t *cyls,
>>   static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
>>                                  int64_t total_sectors)
>>   {
>> +    uint8_t dyndisk_header_buf[1024];
>>       VHDDynDiskHeader *dyndisk_header =
>> -        (VHDDynDiskHeader *) buf;
>> +        (VHDDynDiskHeader *)dyndisk_header_buf;
>>       uint8_t bat_sector[512];
>>       size_t block_size, num_bat_entries;
>>       int i;
>> @@ -858,7 +859,7 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
>>       }
>>         /* Prepare the Dynamic Disk Header */
>> -    memset(buf, 0, 1024);
>> +    memset(dyndisk_header_buf, 0, 1024);
>>         memcpy(dyndisk_header->magic, "cxsparse", 8);
>
> I’d prefer something like
>
> *dyndisk_header = (VHDDynDiskHeader){
>     ...
> };
>
> but that isn’t possible before patch 5.

Yes.  And afterwards, using an initializer would be even simpler, I
guess.

>                                          (And can be done on top now
> anyway, especially given that Kevin probably wants to send a pull 
> request today.)

Yes, would be good to get this wrapped before the break.

> [...]
>
>> @@ -972,8 +974,8 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
>>       BlockBackend *blk = NULL;
>>       BlockDriverState *bs = NULL;
>>   -    uint8_t buf[1024];
>> -    VHDFooter *footer = (VHDFooter *) buf;
>> +    uint8_t footer_buf[HEADER_SIZE];
>> +    VHDFooter *footer = (VHDFooter *)footer_buf;
>>       uint16_t cyls = 0;
>>       uint8_t heads = 0;
>>       uint8_t secs_per_cyl = 0;
>> @@ -1036,7 +1038,7 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
>>       }
>>         /* Prepare the Hard Disk Footer */
>> -    memset(buf, 0, 1024);
>> +    memset(footer_buf, 0, HEADER_SIZE);
>>         memcpy(footer->creator, "conectix", 8);
>
> Same here, except here it’s patch 7.
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Thanks!

[PATCH 4/9] block/vpc: Make vpc_checksum() take void *

[Markus Armbruster]

Some of the next commits will checksum structs.  Change vpc_checksum()
to take void * instead of uint8_t, to save us pointless casts to
uint8_t *.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/vpc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/vpc.c b/block/vpc.c
index 34186640ee..5af9837806 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -172,8 +172,9 @@ static QemuOptsList vpc_runtime_opts = {
 
 static QemuOptsList vpc_create_opts;
 
-static uint32_t vpc_checksum(uint8_t *buf, size_t size)
+static uint32_t vpc_checksum(void *p, size_t size)
 {
+    uint8_t *buf = p;
     uint32_t res = 0;
     int i;
 
-- 
2.26.2

Re: [PATCH 4/9] block/vpc: Make vpc_checksum() take void *

[Max Reitz]

On 17.12.20 17:19, Markus Armbruster wrote:
> Some of the next commits will checksum structs.  Change vpc_checksum()
> to take void * instead of uint8_t, to save us pointless casts to
> uint8_t *.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   block/vpc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index 34186640ee..5af9837806 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -172,8 +172,9 @@ static QemuOptsList vpc_runtime_opts = {
>   
>   static QemuOptsList vpc_create_opts;
>   
> -static uint32_t vpc_checksum(uint8_t *buf, size_t size)
> +static uint32_t vpc_checksum(void *p, size_t size)
>   {
> +    uint8_t *buf = p;
>       uint32_t res = 0;
>       int i;

Using this opportunity to add consts would be nice, but not necessary:

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

Re: [PATCH 4/9] block/vpc: Make vpc_checksum() take void *

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 17.12.20 17:19, Markus Armbruster wrote:
>> Some of the next commits will checksum structs.  Change vpc_checksum()
>> to take void * instead of uint8_t, to save us pointless casts to
>> uint8_t *.
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   block/vpc.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>> diff --git a/block/vpc.c b/block/vpc.c
>> index 34186640ee..5af9837806 100644
>> --- a/block/vpc.c
>> +++ b/block/vpc.c
>> @@ -172,8 +172,9 @@ static QemuOptsList vpc_runtime_opts = {
>>     static QemuOptsList vpc_create_opts;
>>   -static uint32_t vpc_checksum(uint8_t *buf, size_t size)
>> +static uint32_t vpc_checksum(void *p, size_t size)
>>   {
>> +    uint8_t *buf = p;
>>       uint32_t res = 0;
>>       int i;
>
> Using this opportunity to add consts would be nice, but not necessary:

Didn't think of it, missed opportunity.  Not worth a respin all by
itself, I think.  I'm fine with the maintainer adding const.

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

Thanks!

[PATCH 5/9] block/vpc: Pad VHDDynDiskHeader, replace uint8_t[] buffers

[Markus Armbruster]

Pad VHDDynDiskHeader as specified in the "Virtual Hard Disk Image
Format Specification" version 1.0[*].  Change dynamic disk header
buffers from uint8_t[1024] to VHDDynDiskHeader.  Their size remains
the same.

The VHDDynDiskHeader * variables pointing to a VHDDynDiskHeader
variable right next to it are now silly.  Eliminate them.

[*] http://download.microsoft.com/download/f/f/e/ffef50a5-07dd-4cf8-aaa3-442c0673a029/Virtual%20Hard%20Disk%20Format%20Spec_10_18_06.doc

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/vpc.c | 41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 5af9837806..08a0f710ad 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -127,8 +127,11 @@ typedef struct vhd_dyndisk_header {
         uint32_t    reserved;
         uint64_t    data_offset;
     } parent_locator[8];
+    uint8_t     reserved2[256];
 } QEMU_PACKED VHDDynDiskHeader;
 
+QEMU_BUILD_BUG_ON(sizeof(VHDDynDiskHeader) != 1024);
+
 typedef struct BDRVVPCState {
     CoMutex lock;
     uint8_t footer_buf[HEADER_SIZE];
@@ -217,11 +220,10 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVVPCState *s = bs->opaque;
     int i;
     VHDFooter *footer;
-    VHDDynDiskHeader *dyndisk_header;
     QemuOpts *opts = NULL;
     Error *local_err = NULL;
     bool use_chs;
-    uint8_t dyndisk_header_buf[1024];
+    VHDDynDiskHeader dyndisk_header;
     uint32_t checksum;
     uint64_t computed_size;
     uint64_t pagetable_size;
@@ -342,21 +344,19 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
 
     if (disk_type == VHD_DYNAMIC) {
         ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset),
-                         dyndisk_header_buf, 1024);
+                         &dyndisk_header, 1024);
         if (ret < 0) {
             error_setg(errp, "Error reading dynamic VHD header");
             goto fail;
         }
 
-        dyndisk_header = (VHDDynDiskHeader *)dyndisk_header_buf;
-
-        if (strncmp(dyndisk_header->magic, "cxsparse", 8)) {
+        if (strncmp(dyndisk_header.magic, "cxsparse", 8)) {
             error_setg(errp, "Invalid header magic");
             ret = -EINVAL;
             goto fail;
         }
 
-        s->block_size = be32_to_cpu(dyndisk_header->block_size);
+        s->block_size = be32_to_cpu(dyndisk_header.block_size);
         if (!is_power_of_2(s->block_size) || s->block_size < BDRV_SECTOR_SIZE) {
             error_setg(errp, "Invalid block size %" PRIu32, s->block_size);
             ret = -EINVAL;
@@ -364,7 +364,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
         }
         s->bitmap_size = ((s->block_size / (8 * 512)) + 511) & ~511;
 
-        s->max_table_entries = be32_to_cpu(dyndisk_header->max_table_entries);
+        s->max_table_entries = be32_to_cpu(dyndisk_header.max_table_entries);
 
         if ((bs->total_sectors * 512) / s->block_size > 0xffffffffU) {
             error_setg(errp, "Too many blocks");
@@ -396,7 +396,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
             goto fail;
         }
 
-        s->bat_offset = be64_to_cpu(dyndisk_header->table_offset);
+        s->bat_offset = be64_to_cpu(dyndisk_header.table_offset);
 
         ret = bdrv_pread(bs->file, s->bat_offset, s->pagetable,
                          pagetable_size);
@@ -823,9 +823,7 @@ static int calculate_geometry(int64_t total_sectors, uint16_t *cyls,
 static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
                                int64_t total_sectors)
 {
-    uint8_t dyndisk_header_buf[1024];
-    VHDDynDiskHeader *dyndisk_header =
-        (VHDDynDiskHeader *)dyndisk_header_buf;
+    VHDDynDiskHeader dyndisk_header;
     uint8_t bat_sector[512];
     size_t block_size, num_bat_entries;
     int i;
@@ -860,27 +858,26 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
     }
 
     /* Prepare the Dynamic Disk Header */
-    memset(dyndisk_header_buf, 0, 1024);
+    memset(&dyndisk_header, 0, 1024);
 
-    memcpy(dyndisk_header->magic, "cxsparse", 8);
+    memcpy(dyndisk_header.magic, "cxsparse", 8);
 
     /*
      * Note: The spec is actually wrong here for data_offset, it says
      * 0xFFFFFFFF, but MS tools expect all 64 bits to be set.
      */
-    dyndisk_header->data_offset = cpu_to_be64(0xFFFFFFFFFFFFFFFFULL);
-    dyndisk_header->table_offset = cpu_to_be64(3 * 512);
-    dyndisk_header->version = cpu_to_be32(0x00010000);
-    dyndisk_header->block_size = cpu_to_be32(block_size);
-    dyndisk_header->max_table_entries = cpu_to_be32(num_bat_entries);
+    dyndisk_header.data_offset = cpu_to_be64(0xFFFFFFFFFFFFFFFFULL);
+    dyndisk_header.table_offset = cpu_to_be64(3 * 512);
+    dyndisk_header.version = cpu_to_be32(0x00010000);
+    dyndisk_header.block_size = cpu_to_be32(block_size);
+    dyndisk_header.max_table_entries = cpu_to_be32(num_bat_entries);
 
-    dyndisk_header->checksum = cpu_to_be32(vpc_checksum(dyndisk_header_buf,
-                                                        1024));
+    dyndisk_header.checksum = cpu_to_be32(vpc_checksum(&dyndisk_header, 1024));
 
     /* Write the header */
     offset = 512;
 
-    ret = blk_pwrite(blk, offset, dyndisk_header_buf, 1024, 0);
+    ret = blk_pwrite(blk, offset, &dyndisk_header, 1024, 0);
     if (ret < 0) {
         goto fail;
     }
-- 
2.26.2

Re: [PATCH 5/9] block/vpc: Pad VHDDynDiskHeader, replace uint8_t[] buffers

[Max Reitz]

On 17.12.20 17:19, Markus Armbruster wrote:
> Pad VHDDynDiskHeader as specified in the "Virtual Hard Disk Image
> Format Specification" version 1.0[*].  Change dynamic disk header
> buffers from uint8_t[1024] to VHDDynDiskHeader.  Their size remains
> the same.
> 
> The VHDDynDiskHeader * variables pointing to a VHDDynDiskHeader
> variable right next to it are now silly.  Eliminate them.
> 
> [*] http://download.microsoft.com/download/f/f/e/ffef50a5-07dd-4cf8-aaa3-442c0673a029/Virtual%20Hard%20Disk%20Format%20Spec_10_18_06.doc
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   block/vpc.c | 41 +++++++++++++++++++----------------------
>   1 file changed, 19 insertions(+), 22 deletions(-)

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

[PATCH 6/9] block/vpc: Use sizeof() instead of 1024 for dynamic header size

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/vpc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 08a0f710ad..6cb656ac82 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -344,7 +344,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
 
     if (disk_type == VHD_DYNAMIC) {
         ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset),
-                         &dyndisk_header, 1024);
+                         &dyndisk_header, sizeof(dyndisk_header));
         if (ret < 0) {
             error_setg(errp, "Error reading dynamic VHD header");
             goto fail;
@@ -858,7 +858,7 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
     }
 
     /* Prepare the Dynamic Disk Header */
-    memset(&dyndisk_header, 0, 1024);
+    memset(&dyndisk_header, 0, sizeof(dyndisk_header));
 
     memcpy(dyndisk_header.magic, "cxsparse", 8);
 
@@ -872,12 +872,13 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
     dyndisk_header.block_size = cpu_to_be32(block_size);
     dyndisk_header.max_table_entries = cpu_to_be32(num_bat_entries);
 
-    dyndisk_header.checksum = cpu_to_be32(vpc_checksum(&dyndisk_header, 1024));
+    dyndisk_header.checksum = cpu_to_be32(
+        vpc_checksum(&dyndisk_header, sizeof(dyndisk_header)));
 
     /* Write the header */
     offset = 512;
 
-    ret = blk_pwrite(blk, offset, &dyndisk_header, 1024, 0);
+    ret = blk_pwrite(blk, offset, &dyndisk_header, sizeof(dyndisk_header), 0);
     if (ret < 0) {
         goto fail;
     }
-- 
2.26.2

Re: [PATCH 6/9] block/vpc: Use sizeof() instead of 1024 for dynamic header size

[Max Reitz]

On 17.12.20 17:20, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   block/vpc.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)

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

[PATCH 7/9] block/vpc: Pad VHDFooter, replace uint8_t[] buffers

[Markus Armbruster]

Pad VHDFooter as specified in the "Virtual Hard Disk Image Format
Specification" version 1.0[*].  Change footer buffers from
uint8_t[HEADER_SIZE] to VHDFooter.  Their size remains the same.

The VHDFooter * variables pointing to a VHDFooter variable right next
to it are now silly.  Eliminate them, and shorten the remaining
variables' names.

Most variables pointing to s->footer are now also silly.  Eliminate
them, too.

[*] http://download.microsoft.com/download/f/f/e/ffef50a5-07dd-4cf8-aaa3-442c0673a029/Virtual%20Hard%20Disk%20Format%20Spec_10_18_06.doc

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/vpc.c | 77 +++++++++++++++++++++++++----------------------------
 1 file changed, 37 insertions(+), 40 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 6cb656ac82..f3ea92dcb0 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -95,8 +95,11 @@ typedef struct vhd_footer {
     QemuUUID    uuid;
 
     uint8_t     in_saved_state;
+    uint8_t     reserved[427];
 } QEMU_PACKED VHDFooter;
 
+QEMU_BUILD_BUG_ON(sizeof(VHDFooter) != 512);
+
 typedef struct vhd_dyndisk_header {
     char        magic[8]; /* "cxsparse" */
 
@@ -134,7 +137,7 @@ QEMU_BUILD_BUG_ON(sizeof(VHDDynDiskHeader) != 1024);
 
 typedef struct BDRVVPCState {
     CoMutex lock;
-    uint8_t footer_buf[HEADER_SIZE];
+    VHDFooter footer;
     uint64_t free_data_block_offset;
     int max_table_entries;
     uint32_t *pagetable;
@@ -250,13 +253,13 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    ret = bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE);
+    ret = bdrv_pread(bs->file, 0, &s->footer, HEADER_SIZE);
     if (ret < 0) {
         error_setg(errp, "Unable to read VHD header");
         goto fail;
     }
 
-    footer = (VHDFooter *) s->footer_buf;
+    footer = &s->footer;
     if (strncmp(footer->creator, "conectix", 8)) {
         int64_t offset = bdrv_getlength(bs->file->bs);
         if (offset < 0) {
@@ -270,7 +273,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
         }
 
         /* If a fixed disk, the footer is found only at the end of the file */
-        ret = bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf,
+        ret = bdrv_pread(bs->file, offset - HEADER_SIZE, footer,
                          HEADER_SIZE);
         if (ret < 0) {
             goto fail;
@@ -285,7 +288,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
 
     checksum = be32_to_cpu(footer->checksum);
     footer->checksum = 0;
-    if (vpc_checksum(s->footer_buf, HEADER_SIZE) != checksum) {
+    if (vpc_checksum(footer, HEADER_SIZE) != checksum) {
         error_setg(errp, "Incorrect header checksum");
         ret = -EINVAL;
         goto fail;
@@ -535,7 +538,7 @@ static int rewrite_footer(BlockDriverState *bs)
     BDRVVPCState *s = bs->opaque;
     int64_t offset = s->free_data_block_offset;
 
-    ret = bdrv_pwrite_sync(bs->file, offset, s->footer_buf, HEADER_SIZE);
+    ret = bdrv_pwrite_sync(bs->file, offset, &s->footer, HEADER_SIZE);
     if (ret < 0)
         return ret;
 
@@ -598,9 +601,8 @@ fail:
 static int vpc_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 {
     BDRVVPCState *s = (BDRVVPCState *)bs->opaque;
-    VHDFooter *footer = (VHDFooter *) s->footer_buf;
 
-    if (be32_to_cpu(footer->type) != VHD_FIXED) {
+    if (be32_to_cpu(s->footer.type) != VHD_FIXED) {
         bdi->cluster_size = s->block_size;
     }
 
@@ -616,10 +618,9 @@ vpc_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
     int64_t image_offset;
     int64_t n_bytes;
     int64_t bytes_done = 0;
-    VHDFooter *footer = (VHDFooter *) s->footer_buf;
     QEMUIOVector local_qiov;
 
-    if (be32_to_cpu(footer->type) == VHD_FIXED) {
+    if (be32_to_cpu(s->footer.type) == VHD_FIXED) {
         return bdrv_co_preadv(bs->file, offset, bytes, qiov, 0);
     }
 
@@ -667,10 +668,9 @@ vpc_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
     int64_t n_bytes;
     int64_t bytes_done = 0;
     int ret = 0;
-    VHDFooter *footer =  (VHDFooter *) s->footer_buf;
     QEMUIOVector local_qiov;
 
-    if (be32_to_cpu(footer->type) == VHD_FIXED) {
+    if (be32_to_cpu(s->footer.type) == VHD_FIXED) {
         return bdrv_co_pwritev(bs->file, offset, bytes, qiov, 0);
     }
 
@@ -724,13 +724,12 @@ static int coroutine_fn vpc_co_block_status(BlockDriverState *bs,
                                             BlockDriverState **file)
 {
     BDRVVPCState *s = bs->opaque;
-    VHDFooter *footer = (VHDFooter*) s->footer_buf;
     int64_t image_offset;
     bool allocated;
     int ret;
     int64_t n;
 
-    if (be32_to_cpu(footer->type) == VHD_FIXED) {
+    if (be32_to_cpu(s->footer.type) == VHD_FIXED) {
         *pnum = bytes;
         *map = offset;
         *file = bs->file->bs;
@@ -973,8 +972,7 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
     BlockBackend *blk = NULL;
     BlockDriverState *bs = NULL;
 
-    uint8_t footer_buf[HEADER_SIZE];
-    VHDFooter *footer = (VHDFooter *)footer_buf;
+    VHDFooter footer;
     uint16_t cyls = 0;
     uint8_t heads = 0;
     uint8_t secs_per_cyl = 0;
@@ -1037,48 +1035,48 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
     }
 
     /* Prepare the Hard Disk Footer */
-    memset(footer_buf, 0, HEADER_SIZE);
+    memset(&footer, 0, HEADER_SIZE);
 
-    memcpy(footer->creator, "conectix", 8);
+    memcpy(footer.creator, "conectix", 8);
     if (vpc_opts->force_size) {
-        memcpy(footer->creator_app, "qem2", 4);
+        memcpy(footer.creator_app, "qem2", 4);
     } else {
-        memcpy(footer->creator_app, "qemu", 4);
+        memcpy(footer.creator_app, "qemu", 4);
     }
-    memcpy(footer->creator_os, "Wi2k", 4);
+    memcpy(footer.creator_os, "Wi2k", 4);
 
-    footer->features = cpu_to_be32(0x02);
-    footer->version = cpu_to_be32(0x00010000);
+    footer.features = cpu_to_be32(0x02);
+    footer.version = cpu_to_be32(0x00010000);
     if (disk_type == VHD_DYNAMIC) {
-        footer->data_offset = cpu_to_be64(HEADER_SIZE);
+        footer.data_offset = cpu_to_be64(HEADER_SIZE);
     } else {
-        footer->data_offset = cpu_to_be64(0xFFFFFFFFFFFFFFFFULL);
+        footer.data_offset = cpu_to_be64(0xFFFFFFFFFFFFFFFFULL);
     }
-    footer->timestamp = cpu_to_be32(time(NULL) - VHD_TIMESTAMP_BASE);
+    footer.timestamp = cpu_to_be32(time(NULL) - VHD_TIMESTAMP_BASE);
 
     /* Version of Virtual PC 2007 */
-    footer->major = cpu_to_be16(0x0005);
-    footer->minor = cpu_to_be16(0x0003);
-    footer->orig_size = cpu_to_be64(total_size);
-    footer->current_size = cpu_to_be64(total_size);
-    footer->cyls = cpu_to_be16(cyls);
-    footer->heads = heads;
-    footer->secs_per_cyl = secs_per_cyl;
+    footer.major = cpu_to_be16(0x0005);
+    footer.minor = cpu_to_be16(0x0003);
+    footer.orig_size = cpu_to_be64(total_size);
+    footer.current_size = cpu_to_be64(total_size);
+    footer.cyls = cpu_to_be16(cyls);
+    footer.heads = heads;
+    footer.secs_per_cyl = secs_per_cyl;
 
-    footer->type = cpu_to_be32(disk_type);
+    footer.type = cpu_to_be32(disk_type);
 
     qemu_uuid_generate(&uuid);
-    footer->uuid = uuid;
+    footer.uuid = uuid;
 
-    footer->checksum = cpu_to_be32(vpc_checksum(footer_buf, HEADER_SIZE));
+    footer.checksum = cpu_to_be32(vpc_checksum(&footer, HEADER_SIZE));
 
     if (disk_type == VHD_DYNAMIC) {
-        ret = create_dynamic_disk(blk, footer_buf, total_sectors);
+        ret = create_dynamic_disk(blk, (uint8_t *)&footer, total_sectors);
         if (ret < 0) {
             error_setg(errp, "Unable to create or write VHD header");
         }
     } else {
-        ret = create_fixed_disk(blk, footer_buf, total_size, errp);
+        ret = create_fixed_disk(blk, (uint8_t *)&footer, total_size, errp);
     }
 
 out:
@@ -1172,9 +1170,8 @@ fail:
 static int vpc_has_zero_init(BlockDriverState *bs)
 {
     BDRVVPCState *s = bs->opaque;
-    VHDFooter *footer =  (VHDFooter *) s->footer_buf;
 
-    if (be32_to_cpu(footer->type) == VHD_FIXED) {
+    if (be32_to_cpu(s->footer.type) == VHD_FIXED) {
         return bdrv_has_zero_init(bs->file->bs);
     } else {
         return 1;
-- 
2.26.2

Re: [PATCH 7/9] block/vpc: Pad VHDFooter, replace uint8_t[] buffers

[Max Reitz]

On 17.12.20 17:20, Markus Armbruster wrote:
> Pad VHDFooter as specified in the "Virtual Hard Disk Image Format
> Specification" version 1.0[*].  Change footer buffers from
> uint8_t[HEADER_SIZE] to VHDFooter.  Their size remains the same.
> 
> The VHDFooter * variables pointing to a VHDFooter variable right next
> to it are now silly.  Eliminate them, and shorten the remaining
> variables' names.
> 
> Most variables pointing to s->footer are now also silly.  Eliminate
> them, too.
> 
> [*] http://download.microsoft.com/download/f/f/e/ffef50a5-07dd-4cf8-aaa3-442c0673a029/Virtual%20Hard%20Disk%20Format%20Spec_10_18_06.doc
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   block/vpc.c | 77 +++++++++++++++++++++++++----------------------------
>   1 file changed, 37 insertions(+), 40 deletions(-)

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

[PATCH 8/9] block/vpc: Pass footer buffers as VHDFooter * instead of uint8_t *

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/vpc.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index f3ea92dcb0..aac13788df 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -819,7 +819,7 @@ static int calculate_geometry(int64_t total_sectors, uint16_t *cyls,
     return 0;
 }
 
-static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
+static int create_dynamic_disk(BlockBackend *blk, VHDFooter *footer,
                                int64_t total_sectors)
 {
     VHDDynDiskHeader dyndisk_header;
@@ -833,13 +833,13 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
     block_size = 0x200000;
     num_bat_entries = DIV_ROUND_UP(total_sectors, block_size / 512);
 
-    ret = blk_pwrite(blk, offset, buf, HEADER_SIZE, 0);
+    ret = blk_pwrite(blk, offset, footer, HEADER_SIZE, 0);
     if (ret < 0) {
         goto fail;
     }
 
     offset = 1536 + ((num_bat_entries * 4 + 511) & ~511);
-    ret = blk_pwrite(blk, offset, buf, HEADER_SIZE, 0);
+    ret = blk_pwrite(blk, offset, footer, HEADER_SIZE, 0);
     if (ret < 0) {
         goto fail;
     }
@@ -887,7 +887,7 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
     return ret;
 }
 
-static int create_fixed_disk(BlockBackend *blk, uint8_t *buf,
+static int create_fixed_disk(BlockBackend *blk, VHDFooter *footer,
                              int64_t total_size, Error **errp)
 {
     int ret;
@@ -900,7 +900,7 @@ static int create_fixed_disk(BlockBackend *blk, uint8_t *buf,
         return ret;
     }
 
-    ret = blk_pwrite(blk, total_size - HEADER_SIZE, buf, HEADER_SIZE, 0);
+    ret = blk_pwrite(blk, total_size - HEADER_SIZE, footer, HEADER_SIZE, 0);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Unable to write VHD header");
         return ret;
@@ -1071,12 +1071,12 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
     footer.checksum = cpu_to_be32(vpc_checksum(&footer, HEADER_SIZE));
 
     if (disk_type == VHD_DYNAMIC) {
-        ret = create_dynamic_disk(blk, (uint8_t *)&footer, total_sectors);
+        ret = create_dynamic_disk(blk, &footer, total_sectors);
         if (ret < 0) {
             error_setg(errp, "Unable to create or write VHD header");
         }
     } else {
-        ret = create_fixed_disk(blk, (uint8_t *)&footer, total_size, errp);
+        ret = create_fixed_disk(blk, &footer, total_size, errp);
     }
 
 out:
-- 
2.26.2

Re: [PATCH 8/9] block/vpc: Pass footer buffers as VHDFooter * instead of uint8_t *

[Max Reitz]

On 17.12.20 17:20, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   block/vpc.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)

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

(Some mumbling about const pointers in the distance)

[PATCH 9/9] block/vpc: Use sizeof() instead of HEADER_SIZE for footer size

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/vpc.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index aac13788df..17a705b482 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -39,8 +39,6 @@
 
 /**************************************************************/
 
-#define HEADER_SIZE 512
-
 //#define CACHE
 
 enum vhd_type {
@@ -253,7 +251,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    ret = bdrv_pread(bs->file, 0, &s->footer, HEADER_SIZE);
+    ret = bdrv_pread(bs->file, 0, &s->footer, sizeof(s->footer));
     if (ret < 0) {
         error_setg(errp, "Unable to read VHD header");
         goto fail;
@@ -266,15 +264,15 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
             ret = offset;
             error_setg(errp, "Invalid file size");
             goto fail;
-        } else if (offset < HEADER_SIZE) {
+        } else if (offset < sizeof(*footer)) {
             ret = -EINVAL;
             error_setg(errp, "File too small for a VHD header");
             goto fail;
         }
 
         /* If a fixed disk, the footer is found only at the end of the file */
-        ret = bdrv_pread(bs->file, offset - HEADER_SIZE, footer,
-                         HEADER_SIZE);
+        ret = bdrv_pread(bs->file, offset - sizeof(*footer),
+                         footer, sizeof(*footer));
         if (ret < 0) {
             goto fail;
         }
@@ -288,7 +286,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
 
     checksum = be32_to_cpu(footer->checksum);
     footer->checksum = 0;
-    if (vpc_checksum(footer, HEADER_SIZE) != checksum) {
+    if (vpc_checksum(footer, sizeof(*footer)) != checksum) {
         error_setg(errp, "Incorrect header checksum");
         ret = -EINVAL;
         goto fail;
@@ -538,7 +536,7 @@ static int rewrite_footer(BlockDriverState *bs)
     BDRVVPCState *s = bs->opaque;
     int64_t offset = s->free_data_block_offset;
 
-    ret = bdrv_pwrite_sync(bs->file, offset, &s->footer, HEADER_SIZE);
+    ret = bdrv_pwrite_sync(bs->file, offset, &s->footer, sizeof(s->footer));
     if (ret < 0)
         return ret;
 
@@ -833,13 +831,13 @@ static int create_dynamic_disk(BlockBackend *blk, VHDFooter *footer,
     block_size = 0x200000;
     num_bat_entries = DIV_ROUND_UP(total_sectors, block_size / 512);
 
-    ret = blk_pwrite(blk, offset, footer, HEADER_SIZE, 0);
+    ret = blk_pwrite(blk, offset, footer, sizeof(*footer), 0);
     if (ret < 0) {
         goto fail;
     }
 
     offset = 1536 + ((num_bat_entries * 4 + 511) & ~511);
-    ret = blk_pwrite(blk, offset, footer, HEADER_SIZE, 0);
+    ret = blk_pwrite(blk, offset, footer, sizeof(*footer), 0);
     if (ret < 0) {
         goto fail;
     }
@@ -893,14 +891,15 @@ static int create_fixed_disk(BlockBackend *blk, VHDFooter *footer,
     int ret;
 
     /* Add footer to total size */
-    total_size += HEADER_SIZE;
+    total_size += sizeof(*footer);
 
     ret = blk_truncate(blk, total_size, false, PREALLOC_MODE_OFF, 0, errp);
     if (ret < 0) {
         return ret;
     }
 
-    ret = blk_pwrite(blk, total_size - HEADER_SIZE, footer, HEADER_SIZE, 0);
+    ret = blk_pwrite(blk, total_size - sizeof(*footer),
+                     footer, sizeof(*footer), 0);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Unable to write VHD header");
         return ret;
@@ -1035,7 +1034,7 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
     }
 
     /* Prepare the Hard Disk Footer */
-    memset(&footer, 0, HEADER_SIZE);
+    memset(&footer, 0, sizeof(footer));
 
     memcpy(footer.creator, "conectix", 8);
     if (vpc_opts->force_size) {
@@ -1048,7 +1047,7 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
     footer.features = cpu_to_be32(0x02);
     footer.version = cpu_to_be32(0x00010000);
     if (disk_type == VHD_DYNAMIC) {
-        footer.data_offset = cpu_to_be64(HEADER_SIZE);
+        footer.data_offset = cpu_to_be64(sizeof(footer));
     } else {
         footer.data_offset = cpu_to_be64(0xFFFFFFFFFFFFFFFFULL);
     }
@@ -1068,7 +1067,7 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
     qemu_uuid_generate(&uuid);
     footer.uuid = uuid;
 
-    footer.checksum = cpu_to_be32(vpc_checksum(&footer, HEADER_SIZE));
+    footer.checksum = cpu_to_be32(vpc_checksum(&footer, sizeof(footer)));
 
     if (disk_type == VHD_DYNAMIC) {
         ret = create_dynamic_disk(blk, &footer, total_sectors);
-- 
2.26.2

Re: [PATCH 9/9] block/vpc: Use sizeof() instead of HEADER_SIZE for footer size

[Max Reitz]

On 17.12.20 17:20, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   block/vpc.c | 29 ++++++++++++++---------------
>   1 file changed, 14 insertions(+), 15 deletions(-)

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

Re: [PATCH 0/9] block/vpc: Clean up some buffer abuse

[Kevin Wolf]

Am 17.12.2020 um 17:19 hat Markus Armbruster geschrieben:
> Markus Armbruster (9):
>   block/vpc: Make vpc_open() read the full dynamic header
>   block/vpc: Don't abuse the footer buffer as BAT sector buffer
>   block/vpc: Don't abuse the footer buffer for dynamic header
>   block/vpc: Make vpc_checksum() take void *
>   block/vpc: Pad VHDDynDiskHeader, replace uint8_t[] buffers
>   block/vpc: Use sizeof() instead of 1024 for dynamic header size
>   block/vpc: Pad VHDFooter, replace uint8_t[] buffers
>   block/vpc: Pass footer buffers as VHDFooter * instead of uint8_t *
>   block/vpc: Use sizeof() instead of HEADER_SIZE for footer size

Thanks, applied to the block branch.

Kevin