Re: [PATCH v5 5/8] hw/usb/redirect-server: Implement control transfers

[Junjie Cao <junjie.cao@linux.dev>]

From: Junjie Cao <junjie.cao@intel.com>

Hi Jamin,

The tests below were run on v4; patches 5 and 6 are unchanged in v5.

> +    usbredir_server_do_control(priv, id, hdr, data, data_len,
> +                               USBREDIR_SERVER_REPLY_CONTROL);

The callback owns data, but do_control() copies it without freeing the
original buffer. In a test with usb-storage, 100 vendor control OUT
requests carrying 64 bytes each complete with STALL, but LSan reports
6,400 bytes leaked in 100 parser allocations.

Could we call usbredirparser_free_packet_data() after do_control()
returns, and free data on every exit from the bulk and interrupt
callbacks in patch 6?

> +    QTAILQ_REMOVE(&s->inflight, rp, next);
> +    usb_packet_cleanup(&rp->pkt);

A test device returning async IOERROR for the first of two bulk OUT
requests reproduces an assertion here. The core calls back for the
second packet with USB_RET_REMOVE_FROM_QUEUE, but leaves it QUEUED on
ep->queue. usb_packet_cleanup() then hits its
!usb_packet_is_inflight() assertion.

Could we call usb_cancel_packet() for that status before cleanup?
For a QUEUED packet it removes the core queue entry without calling
the device's cancel callback.

In the not-found case of usbredir_server_cancel_data_packet():

> +    usbredirparser_send_control_packet(s->parser, id, &resp, NULL, 0);

If the original reply is already queued when the cancel arrives, the
QEMU client consumes its cancelled-ID entry when it receives that
reply. The extra control packet then sends a second reply for the
same ID.

Could we omit this reply when the transfer is not found?
usbredirhost_cancel_data_packet() also sends no additional reply
in that case.