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