[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Marc-André Lureau
Subject: Re: [PATCH-for-5.2? v3 3/9] hw/usb: reorder fields in UASStatus
Date: Thu, 14 Jan 2021 12:17:07 +0400


On Thu, Nov 19, 2020 at 8:19 PM Daniele Buono <dbuono@linux.vnet.ibm.com> wrote:
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

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

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
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) {
         length = MIN(sizeof(iu), p->iov.size);
         usb_packet_copy(p, &iu, length);

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

Is this intended?

Any update on this patch?

Marc-André Lureau

reply via email to

[Prev in Thread] Current Thread [Next in Thread]