[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/9] block/vpc: Make vpc_open() read the full dynamic header
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 1/9] block/vpc: Make vpc_open() read the full dynamic header |
Date: |
Fri, 18 Dec 2020 14:49:26 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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!
- [PATCH 8/9] block/vpc: Pass footer buffers as VHDFooter * instead of uint8_t *, (continued)
- [PATCH 8/9] block/vpc: Pass footer buffers as VHDFooter * instead of uint8_t *, Markus Armbruster, 2020/12/17
- [PATCH 7/9] block/vpc: Pad VHDFooter, replace uint8_t[] buffers, Markus Armbruster, 2020/12/17
- [PATCH 5/9] block/vpc: Pad VHDDynDiskHeader, replace uint8_t[] buffers, Markus Armbruster, 2020/12/17
- [PATCH 9/9] block/vpc: Use sizeof() instead of HEADER_SIZE for footer size, Markus Armbruster, 2020/12/17
- [PATCH 1/9] block/vpc: Make vpc_open() read the full dynamic header, Markus Armbruster, 2020/12/17
- Re: [PATCH 0/9] block/vpc: Clean up some buffer abuse, Kevin Wolf, 2020/12/18