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

[Frediano Ziglio]

> 
> 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 check for NULL as this path is supposed to initialize the
parse only if usbredirhost is not initialized.
Maybe would be wotrth a comment or another name. Or maybe just inline
it now, not very long. What do you suggest?

> > 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).

> > +        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).

> 
> Uri.
> 

Frediano