Max Reitz <mre...@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 <arm...@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 <mre...@redhat.com> Thanks!