[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: Daniele Buono
Subject: Re: [PATCH-for-5.2? v3 3/9] hw/usb: reorder fields in UASStatus
Date: Thu, 19 Nov 2020 11:16:52 -0500
User-agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.4.1

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 

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?

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;
/* --------------------------------------------------------------------- */

reply via email to

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