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?
Any update on this patch?
thanks