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

[Uri Lublin]

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.

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

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

Thanks,
     Uri.