[Spice-devel] [PATCH spice-gtk v4 24/29] usb-backend: Rewrite USB emulation support

[Frediano Ziglio]

> 
> On 9/3/19 3:08 PM, Frediano Ziglio wrote:
> >>
> >> Hi,
> >>
> >> On 8/27/19 12:22 PM, Frediano Ziglio wrote:
> >>> Make initialisation easier.
> >>> Always initialise parser.
> >>> Initialise both parser and host during spice_usb_backend_channel_new.
> >>> Support not having libusb context (no physical devices).
> >>> Avoids too much state variables.
> >>> parser is always initialised after creation making sure the state
> >>> is consistent.
> >>
> >> If usbredirhost is used, why is there a need for a parser too ?
> >> Also below in initialize_parser there is a check that
> >> ch->usbredirhost is NULL (possibly because the parser is always
> >> being initialized before the host).
> >>
> > 
> > For a larger explanation see former patch from Yuri.
> > For a shorter to parse data directed to emulated devices.
> > So host devices (physical) through usbredirhost, emulated devices
> > through usbparser.
> 
> The commit log is saying the parser is always being initialized.
> I'm asking if it is needed also for physical devices.
> 

No, it's used only for emulated devices.

> > 
> > The check for NULL as this path is supposed to initialize the
> > parse only if usbredirhost is not initialize > Maybe would be worth a
> > comment or another name. Or maybe just inline
> > it now, not very long. What do you suggest?
> 
> Just leave it as is. I'll look at previous patches to better understand.
> 
> > 
> >>> Use a single state variable, data flows into/from a single parser.
> >>>
> >>> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> >>> ---
> >>>    src/usb-backend.c | 236 +++++++++++++++++++++++-----------------------
> >>>    1 file changed, 116 insertions(+), 120 deletions(-)
> >>>
> >>> diff --git a/src/usb-backend.c b/src/usb-backend.c
> >>> index 36a73a89..d614e693 100644
> >>> --- a/src/usb-backend.c
> >>> +++ b/src/usb-backend.c
> >>> @@ -78,21 +78,21 @@ struct _SpiceUsbBackend
> >>>        uint32_t own_devices_mask;
> >>>    };
> >>>    
> >>> +typedef enum {
> >>> +    USB_CHANNEL_STATE_INITIALIZING,
> >>> +    USB_CHANNEL_STATE_HOST,
> >>> +    USB_CHANNEL_STATE_PARSER,
> >>> +} SpiceUsbBackendChannelState;
> >>> +
> >>>    struct _SpiceUsbBackendChannel
> >>>    {
> >>>        struct usbredirhost *usbredirhost;
> >>> -    struct usbredirhost *hidden_host;
> >>>        struct usbredirparser *parser;
> >>> -    struct usbredirparser *hidden_parser;
> >>> +    SpiceUsbBackendChannelState state;
> >>>        uint8_t *read_buf;
> >>>        int read_buf_size;
> >>>        struct usbredirfilter_rule *rules;
> >>>        int rules_count;
> >>> -    uint32_t host_caps;
> >>> -    uint32_t parser_init_done  : 1;
> >>> -    uint32_t parser_with_hello : 1;
> >>> -    uint32_t hello_done_parser : 1;
> >>> -    uint32_t hello_sent        : 1;
> >>>        uint32_t rejected          : 1;
> >>>        uint32_t wait_disconnect_ack : 1;
> >>>        SpiceUsbBackendDevice *attached;
> >>> @@ -405,15 +405,16 @@ from both the main thread as well as from the usb
> >>> event handling thread */
> >>>    static void usbredir_write_flush_callback(void *user_data)
> >>>    {
> >>>        SpiceUsbBackendChannel *ch = user_data;
> >>> +    if (ch->parser == NULL) {
> >>> +        return;
> >>> +    }
> >>>        if (is_channel_ready(ch->user_data)) {
> >>> -        if (ch->usbredirhost) {
> >>
> >> Do you need the following g_assert, or is the ch->parser==NULL enough
> >>      g_assert(ch->state != USB_CHANNEL_STATE_INITIALIZING);
> >>
> > 
> > Which one are you referring to?
> > 
> >>> +        if (ch->state == USB_CHANNEL_STATE_HOST) {
> >>>                SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
> >>>                usbredirhost_write_guest_data(ch->usbredirhost);
> >>> -        } else if (ch->parser) {
> >>> +        } else {
> >>>                SPICE_DEBUG("%s ch %p -> parser", __FUNCTION__, ch);
> >>>                usbredirparser_do_write(ch->parser);
> >>> -        } else {
> >>> -            SPICE_DEBUG("%s ch %p (NOT ACTIVE)", __FUNCTION__, ch);
> >>>            }
> >>>        } else {
> >>>            SPICE_DEBUG("%s ch %p (not ready)", __FUNCTION__, ch);
> >>> @@ -673,21 +674,42 @@ static void usbredir_log(void *user_data, int
> >>> level,
> >>> const char *msg)
> >>>        }
> >>>    }
> >>>    
> >>> +static struct usbredirparser *create_parser(SpiceUsbBackendChannel *ch);
> >>> +
> >>>    static int usbredir_write_callback(void *user_data, uint8_t *data, int
> >>>    count)
> >>>    {
> >>>        SpiceUsbBackendChannel *ch = user_data;
> >>>        int res;
> >>>        SPICE_DEBUG("%s ch %p, %d bytes", __FUNCTION__, ch, count);
> >>> -    if (!ch->hello_sent) {
> >>> -        /* hello is short header (12) + hello struct (64) + capabilities
> >>> (4) */
> >>> -        int hello_size = sizeof(struct usb_redir_header) + sizeof(struct
> >>> usb_redir_hello_header);
> >>> -        ch->hello_sent = 1;
> >>> -        if (count == hello_size) {
> >>> -            memcpy(&ch->host_caps, data + hello_size -
> >>> sizeof(ch->host_caps),
> >>> -                   sizeof(ch->host_caps));
> >>> -            SPICE_DEBUG("%s ch %p, sending first hello, caps %08X",
> >>> -                        __FUNCTION__, ch, ch->host_caps);
> >>> +
> >>> +    // handle first packet (HELLO) creating parser from capabilities
> >>> +    if (SPICE_UNLIKELY(ch->parser == NULL)) {
> >>> +        // we are still initializing the host
> >>> +        if (ch->usbredirhost == NULL) {
> >>> +            return 0;
> >>>            }
> >>> +
> >>> +        ch->parser = create_parser(ch);
> >>> +        if (!ch->parser) {
> >>> +            return 0;
> >>> +        }
> >>> +
> >>> +        /* hello is short header (12) + hello struct (64) */
> >>> +        const int hello_size = 12 + sizeof(struct
> >>> usb_redir_hello_header);
> >>
> >> sizeof(struct usb_redir_header) instead of 12 (and btw it's 16)
> >>
> > 
> > Yes, that's why 12 is used. It's not usb_redir_header that you want but the
> > 32 bit version which is defined only internally in usbredir code (not
> > public
> > headers).
> 
> Using internal-only compatibility header is not nice.
> Why not use 64bit and public headers (check the peer supports
> it too first) ?
> (again if it was answered in previous emails I apologize)
> 

Do you mean the peer capabilities?
No, in this case this is the first message, the one with no capabilities
still decided so it's assumed for ABI compatibility to have all capabilities
disabled, that is using 32 bit id. The structure is declared in
usbredirparser/usbredirproto-compat.h and it's

struct usb_redir_header_32bit_id {
    uint32_t type;
    uint32_t length;
    uint32_t id;
} ATTR_PACKED;

Maybe would be worth if we declare it? But it would be used only for the sizeof.

> > 
> >>> +        g_assert(count >= hello_size + 4);
> >>
> >> nit: Maybe replace 4 with
> >>     const size_t caps_size = sizeof(uint32_t) * USB_REDIR_CAPS_SIZE;
> >>     g_assert(count >= hello_size + caps_size);
> >>
> > 
> > This potentially can crash in the future when USB_REDIR_CAPS_SIZE will
> > change.
> > But I suppose you can replace "4" with sizeof(uint32_t).
> 
> OK, so basically there must be at least 1 uint32_t for caps and
> in the future maybe more (according to count). If it does change
> in the future and we really want to, we can tell by peer version.
> 

I didn't probably get.
What I can say is that for compatiblity the hello message has to keep
the same structure. It's allowed to extend the capabilities fields to
include more capabilities and this should be detected looking at the
"count" field (in this patch it's the "(count - hello_size) / sizeof(uint32_t)"
formulae).

> Thanks,
>      Uri.
>