Re: [PATCH-for-5.2? v3 3/9] hw/usb: reorder fields in UASStatus

[Daniele Buono]

Hi Philippe,

On 11/6/2020 9:28 AM, Philippe Mathieu-Daudé wrote:
> On 11/5/20 11:18 PM, Daniele Buono wrote:
> > The UASStatus data structure has a variable sized field inside of type uas_iu,
> > that however is not placed at the end of the data structure.
> >
> > This placement triggers a warning with clang 11, and while not a bug right now,
> > (the status is never a uas_iu_command, which is the variable-sized case),
> > it could become one in the future.
>
> The problem is uas_iu_command::add_cdb, indeed.
>
> > ../qemu-base/hw/usb/dev-uas.c:157:31: error: field 'status' with variable sized 
> > type 'uas_iu' not at the end of a struct or class is a GNU extension 
> > [-Werror,-Wgnu-variable-sized-type-not-at-end]
>
> If possible remove the "../qemu-base/" as it does not provide
> any useful information.
Sure, will do at the next cycle
> >      uas_iu                    status;
> >                                ^
> > 1 error generated.
> >
> > Fix this by moving uas_iu at the end of the struct
>
> Your patch silents the warning, but the problem is the same.
> It would be safer/cleaner to make 'status' a pointer on the heap IMO.

I'm thinking of moving 'status' in a pointer with the following code
changes:

UASStatus is allocated in `usb_uas_alloc_status`, which currently does
not take a type or size for the union field. I'm thinking of adding
requested size for the status, like this:

static UASStatus *usb_uas_alloc_status(UASDevice *uas, uint8_t id,
uint16_t tag, size_t size);

and the common call would be
usb_uas_alloc_status([...],sizeof(uas_iu));

Also we'd need a double free when the object is freed. Right now
it's handled in the code when the object is not used anymore with a
`g_free(st);`.
I'd have to replace it with
`g_free(st->status); g_free(st);`. Would you suggest doing it place
or by adding a usb_uas_dealloc_status() function?

---

However, I am confused by the use of that variable-lenght field.
I'm looking at what seems the only place where a command is
parsed, in `usb_uas_handle_data`.

uas_iu iu;
[...]
    switch (p->ep->nr) {
    case UAS_PIPE_ID_COMMAND:
        length = MIN(sizeof(iu), p->iov.size);
        usb_packet_copy(p, &iu, length);
        [...]
        break;
[...]

It would seem that the copy is limited to at most sizeof(uas_iu),
so even if we had anything in add_cdb[], that wouldn't be copied
here?

Is this intended?

> > Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
> > ---
> >   hw/usb/dev-uas.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
> > index cec071d96c..5ef3f4fec9 100644
> > --- a/hw/usb/dev-uas.c
> > +++ b/hw/usb/dev-uas.c
> > @@ -154,9 +154,9 @@ struct UASRequest {
> >   
> >   struct UASStatus {
> >       uint32_t                  stream;
> > -    uas_iu                    status;
> >       uint32_t                  length;
> >       QTAILQ_ENTRY(UASStatus)   next;
> > +    uas_iu                    status;
> >   };
> >   
> >   /* --------------------------------------------------------------------- */