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

Submitted by Frediano Ziglio on Aug. 27, 2019, 9:22 a.m.

Details

Message ID 20190827092246.10276-25-fziglio@redhat.com
State Superseded
Headers show
Series "added feature of sharing CD image" ( rev: 6 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Aug. 27, 2019, 9:22 a.m.
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.
Use a single state variable, data flows into/from a single parser.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 src/usb-backend.c | 236 +++++++++++++++++++++++-----------------------
 1 file changed, 116 insertions(+), 120 deletions(-)

Patch hide | download patch | download mbox

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) {
+        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);
+        g_assert(count >= hello_size + 4);
+        g_assert(SPICE_ALIGNED_CAST(struct usb_redir_header *, data)->type == usb_redir_hello);
+
+        const uint32_t flags =
+            usbredirparser_fl_write_cb_owns_buffer | usbredirparser_fl_usb_host |
+            usbredirparser_fl_no_hello;
+
+        usbredirparser_init(ch->parser,
+                            PACKAGE_STRING,
+                            SPICE_ALIGNED_CAST(uint32_t *, data + hello_size),
+                            (count - hello_size) / sizeof(uint32_t),
+                            flags);
+
+        return 0;
     }
     res = spice_usbredir_write_callback(ch->user_data, data, count);
     return res;
@@ -707,31 +729,33 @@  int spice_usb_backend_read_guest_data(SpiceUsbBackendChannel *ch, uint8_t *data,
 
     ch->read_buf = data;
     ch->read_buf_size = count;
-    if (ch->usbredirhost) {
-        res = usbredirhost_read_guest_data(ch->usbredirhost);
-        if (!ch->hello_done_parser) {
-            int res_parser;
+    if (SPICE_UNLIKELY(ch->state == USB_CHANNEL_STATE_INITIALIZING)) {
+        if (ch->usbredirhost != NULL) {
+            res = usbredirhost_read_guest_data(ch->usbredirhost);
+            if (res != 0) {
+                return res;
+            }
+            ch->state = USB_CHANNEL_STATE_HOST;
+
             /* usbredirhost should consume hello response */
             g_return_val_if_fail(ch->read_buf == NULL, USB_REDIR_ERROR_READ_PARSE);
+        } else {
+            ch->state = USB_CHANNEL_STATE_PARSER;
+        }
 
-            ch->read_buf = data;
-            ch->read_buf_size = count;
-            ch->hello_done_parser = 1;
-            if (ch->attached && ch->attached->edev) {
-                /* case of CD sharing on connect */
-                ch->usbredirhost = NULL;
-                ch->parser = ch->hidden_parser;
-                SPICE_DEBUG("%s: switch %p to parser", __FUNCTION__, ch);
-            }
-            res_parser = usbredirparser_do_read(ch->hidden_parser);
-            if (res >= 0) {
-                res = res_parser;
-            }
+        ch->read_buf = data;
+        ch->read_buf_size = count;
+        if (ch->attached && ch->attached->edev) {
+            /* case of CD sharing on connect */
+            ch->state = USB_CHANNEL_STATE_PARSER;
+            SPICE_DEBUG("%s: switch %p to parser", __FUNCTION__, ch);
         }
-    } else if (ch->parser) {
-        res = usbredirparser_do_read(ch->parser);
+        return usbredirparser_do_read(ch->parser);
+    }
+    if (ch->state == USB_CHANNEL_STATE_HOST) {
+        res = usbredirhost_read_guest_data(ch->usbredirhost);
     } else {
-        res = USB_REDIR_ERROR_IO;
+        res = usbredirparser_do_read(ch->parser);
     }
     switch (res)
     {
@@ -783,14 +807,12 @@  GError *spice_usb_backend_get_error_details(int error_code, gchar *desc)
 
 void spice_usb_backend_return_write_data(SpiceUsbBackendChannel *ch, void *data)
 {
-    if (ch->usbredirhost) {
+    if (ch->state == USB_CHANNEL_STATE_HOST) {
         SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
         usbredirhost_free_write_buffer(ch->usbredirhost, data);
-    } else if (ch->parser) {
+    } else {
         SPICE_DEBUG("%s ch %p -> parser", __FUNCTION__, ch);
         usbredirparser_free_write_buffer(ch->parser, data);
-    } else {
-        SPICE_DEBUG("%s ch %p - NOBODY TO CALL", __FUNCTION__, ch);
     }
 }
 
@@ -1005,10 +1027,10 @@  static void usbredir_device_disconnect_ack(void *priv)
 {
     SpiceUsbBackendChannel *ch = priv;
     SPICE_DEBUG("%s ch %p", __FUNCTION__, ch);
-    if (ch->parser && ch->wait_disconnect_ack) {
-        ch->parser = NULL;
+    if (ch->state == USB_CHANNEL_STATE_PARSER && ch->usbredirhost != NULL &&
+        ch->wait_disconnect_ack) {
+        ch->state = USB_CHANNEL_STATE_HOST;
         SPICE_DEBUG("%s switch to usbredirhost", __FUNCTION__);
-        ch->usbredirhost = ch->hidden_host;
     }
     ch->wait_disconnect_ack = 0;
 }
@@ -1027,9 +1049,6 @@  usbredir_hello(void *priv, struct usb_redir_hello_header *hello)
     SPICE_DEBUG("%s %p %sattached %s", __FUNCTION__, ch,
         edev ? "" : "not ",  hello ? "" : "(internal)");
 
-    if (hello) {
-        ch->hello_done_parser = 1;
-    }
     if (!edev) {
         return;
     }
@@ -1079,53 +1098,24 @@  usbredir_hello(void *priv, struct usb_redir_hello_header *hello)
     usbredir_write_flush_callback(ch);
 }
 
-static void initialize_parser(SpiceUsbBackendChannel *ch, gboolean do_hello)
+static void initialize_parser(SpiceUsbBackendChannel *ch)
 {
     uint32_t flags, caps[USB_REDIR_CAPS_SIZE] = { 0 };
 
-    g_return_if_fail(ch->hidden_parser != NULL);
-    if (ch->parser_init_done) {
-        if (ch->parser_with_hello != do_hello) {
-            g_return_if_reached();
-        }
-        return;
-    }
+    g_assert(ch->usbredirhost == NULL);
 
-    ch->parser_init_done = 1;
-    ch->parser_with_hello = do_hello;
     flags = usbredirparser_fl_write_cb_owns_buffer | usbredirparser_fl_usb_host;
 
-    if (do_hello) {
-        ch->hello_sent = 1;
-        ch->host_caps |= 1 << usb_redir_cap_connect_device_version;
-        ch->host_caps |= 1 << usb_redir_cap_device_disconnect_ack;
-        ch->host_caps |= 1 << usb_redir_cap_ep_info_max_packet_size;
-        ch->host_caps |= 1 << usb_redir_cap_64bits_ids;
-        ch->host_caps |= 1 << usb_redir_cap_32bits_bulk_length;
-    } else {
-        flags |= usbredirparser_fl_no_hello;
-    }
-
-    if (ch->host_caps & (1 << usb_redir_cap_connect_device_version)) {
-        usbredirparser_caps_set_cap(caps, usb_redir_cap_connect_device_version);
-    }
+    usbredirparser_caps_set_cap(caps, usb_redir_cap_connect_device_version);
     usbredirparser_caps_set_cap(caps, usb_redir_cap_filter);
-    if (ch->host_caps & (1 << usb_redir_cap_device_disconnect_ack)) {
-        usbredirparser_caps_set_cap(caps, usb_redir_cap_device_disconnect_ack);
-    }
-    if (ch->host_caps & (1 << usb_redir_cap_ep_info_max_packet_size)) {
-        usbredirparser_caps_set_cap(caps, usb_redir_cap_ep_info_max_packet_size);
-    }
-    if (ch->host_caps & (1 << usb_redir_cap_64bits_ids)) {
-        usbredirparser_caps_set_cap(caps, usb_redir_cap_64bits_ids);
-    }
-    if (ch->host_caps & (1 << usb_redir_cap_32bits_bulk_length)) {
-        usbredirparser_caps_set_cap(caps, usb_redir_cap_32bits_bulk_length);
-    }
-    if (ch->host_caps & (1 << usb_redir_cap_bulk_streams)) {
-        usbredirparser_caps_set_cap(caps, usb_redir_cap_bulk_streams);
-    }
-    usbredirparser_init(ch->hidden_parser, PACKAGE_STRING, caps, USB_REDIR_CAPS_SIZE, flags);
+    usbredirparser_caps_set_cap(caps, usb_redir_cap_device_disconnect_ack);
+    usbredirparser_caps_set_cap(caps, usb_redir_cap_ep_info_max_packet_size);
+    usbredirparser_caps_set_cap(caps, usb_redir_cap_64bits_ids);
+    usbredirparser_caps_set_cap(caps, usb_redir_cap_32bits_bulk_length);
+    usbredirparser_caps_set_cap(caps, usb_redir_cap_bulk_receiving);
+    usbredirparser_caps_set_cap(caps, usb_redir_cap_bulk_streams);
+
+    usbredirparser_init(ch->parser, PACKAGE_STRING, caps, USB_REDIR_CAPS_SIZE, flags);
 }
 
 /*
@@ -1180,7 +1170,7 @@  static gboolean attach_edev(SpiceUsbBackendChannel *ch,
            _("Failed to redirect device %d"), 1);
         return FALSE;
     }
-    if (ch->usbredirhost && !ch->hello_done_parser) {
+    if (ch->state == USB_CHANNEL_STATE_INITIALIZING) {
         /*
             we can't initialize parser until we see hello from usbredir
             and the parser can't work until it sees the hello response.
@@ -1190,15 +1180,13 @@  static gboolean attach_edev(SpiceUsbBackendChannel *ch,
         SPICE_DEBUG("%s waiting until the channel is ready", __FUNCTION__);
 
     } else {
-        initialize_parser(ch, ch->hidden_host == NULL);
-        ch->usbredirhost = NULL;
-        ch->parser = ch->hidden_parser;
+        ch->state = USB_CHANNEL_STATE_PARSER;
     }
     ch->wait_disconnect_ack = 0;
     ch->attached = dev;
     dev->attached_to = ch;
-    device_ops(dev->edev)->attach(dev->edev, ch->hidden_parser);
-    if (ch->hello_done_parser) {
+    device_ops(dev->edev)->attach(dev->edev, ch->parser);
+    if (ch->state == USB_CHANNEL_STATE_PARSER) {
         /* send device info */
         usbredir_hello(ch, NULL);
     }
@@ -1218,9 +1206,15 @@  gboolean spice_usb_backend_channel_attach(SpiceUsbBackendChannel *ch,
         return attach_edev(ch, dev, error);
     }
 
+    // no physical device enabled
+    if (ch->usbredirhost == NULL) {
+        return FALSE;
+    }
+
     libusb_device_handle *handle = NULL;
-    ch->usbredirhost = ch->hidden_host;
-    ch->parser = NULL;
+    if (ch->state != USB_CHANNEL_STATE_INITIALIZING) {
+        ch->state = USB_CHANNEL_STATE_HOST;
+    }
 
     /*
        Under Windows we need to avoid updating
@@ -1261,10 +1255,10 @@  void spice_usb_backend_channel_detach(SpiceUsbBackendChannel *ch)
         SPICE_DEBUG("%s: nothing to detach", __FUNCTION__);
         return;
     }
-    if (ch->usbredirhost) {
+    if (ch->state == USB_CHANNEL_STATE_HOST) {
         /* it will call libusb_close internally */
         usbredirhost_set_device(ch->usbredirhost, NULL);
-    } else if (ch->parser) {
+    } else {
         if (edev) {
             device_ops(edev)->detach(edev);
         }
@@ -1272,9 +1266,8 @@  void spice_usb_backend_channel_detach(SpiceUsbBackendChannel *ch)
         usbredir_write_flush_callback(ch);
         ch->wait_disconnect_ack =
             usbredirparser_peer_has_cap(ch->parser, usb_redir_cap_device_disconnect_ack);
-        if (!ch->wait_disconnect_ack) {
-            ch->usbredirhost = ch->hidden_host;
-            ch->parser = NULL;
+        if (!ch->wait_disconnect_ack && ch->usbredirhost != NULL) {
+            ch->state = USB_CHANNEL_STATE_HOST;
         }
     }
     SPICE_DEBUG("%s ch %p, detach done", __FUNCTION__, ch);
@@ -1311,16 +1304,22 @@  SpiceUsbBackendChannel *spice_usb_backend_channel_new(SpiceUsbBackend *be,
                                         usbredirparser_warning,
                                    usbredirhost_fl_write_cb_owns_buffer);
         g_warn_if_fail(ch->usbredirhost != NULL);
+        if (ch->usbredirhost != NULL) {
+            usbredirhost_set_buffered_output_size_cb(ch->usbredirhost,
+                                                     usbredir_buffered_output_size_callback);
+            // force flush of HELLO packet and creation of parser
+            usbredirhost_write_guest_data(ch->usbredirhost);
+        }
+    } else {
+        // no physical device support, only emulated, create the
+        // parser
+        ch->parser = create_parser(ch);
+        if (ch->parser != NULL) {
+            initialize_parser(ch);
+        }
     }
 
-    if (ch->usbredirhost) {
-        ch->hidden_parser = create_parser(ch);
-        ch->hidden_host = ch->usbredirhost;
-        usbredirhost_set_buffered_output_size_cb(ch->usbredirhost,
-                                                 usbredir_buffered_output_size_callback);
-    }
-
-    if (!ch->hidden_parser) {
+    if (!ch->parser) {
         spice_usb_backend_channel_delete(ch);
         ch = NULL;
     }
@@ -1332,12 +1331,9 @@  SpiceUsbBackendChannel *spice_usb_backend_channel_new(SpiceUsbBackend *be,
 void spice_usb_backend_channel_flush_writes(SpiceUsbBackendChannel *ch)
 {
     SPICE_DEBUG("%s %p is up", __FUNCTION__, ch);
-    if (ch->usbredirhost) {
+    if (ch->state != USB_CHANNEL_STATE_PARSER && ch->usbredirhost != NULL) {
         usbredirhost_write_guest_data(ch->usbredirhost);
-        initialize_parser(ch, FALSE);
     } else {
-        initialize_parser(ch, TRUE);
-        ch->parser = ch->hidden_parser;
         usbredirparser_do_write(ch->parser);
     }
 }
@@ -1348,11 +1344,11 @@  void spice_usb_backend_channel_delete(SpiceUsbBackendChannel *ch)
     if (!ch) {
         return;
     }
-    if (ch->hidden_host) {
-        usbredirhost_close(ch->hidden_host);
+    if (ch->usbredirhost) {
+        usbredirhost_close(ch->usbredirhost);
     }
-    if (ch->hidden_parser) {
-        usbredirparser_destroy(ch->hidden_parser);
+    if (ch->parser) {
+        usbredirparser_destroy(ch->parser);
     }
 
     if (ch->rules) {
@@ -1372,7 +1368,7 @@  spice_usb_backend_channel_get_guest_filter(SpiceUsbBackendChannel *ch,
     int i;
     *r = NULL;
     *count = 0;
-    if (ch->usbredirhost) {
+    if (ch->usbredirhost != NULL) {
         usbredirhost_get_guest_filter(ch->usbredirhost, r, count);
     }
     if (*r == NULL) {

Comments

Frediano Ziglio writes:

> Make initialisation easier.
> Always initialise parser.

My spellchecker seems to choke on "initialise", suggests
"initialize" (maybe a UK/US difference).

> Initialise both parser and host during spice_usb_backend_channel_new.
> Support not having libusb context (no physical devices).
> Avoids too much state variables.

"Reduce number of state variables"?

Based on the code, I'd go as far as stating "Simplify state machine logic".

> parser is always initialised after creation making sure the state
> is consistent.

That seems redundant with "Always initialize parser".


> Use a single state variable, data flows into/from a single parser.
>
> Signed-off-by: Frediano Ziglio <fziglio@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,

(You seem to have used "initialize" with a "z" here)

> +    USB_CHANNEL_STATE_HOST,
> +    USB_CHANNEL_STATE_PARSER,

I can't say that "STATE_HOST" and "STATE_PARSER" make much sense to me.
Maybe something like "STATE_USBREDIRHOST" and "STATE_PARSING" or
"STATE_PARSER_ACTIVE"?

> +} 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;

Nice simplification!

Would it make sense to use "bool" for flags?

>      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) {
> +        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);
> +        g_assert(count >= hello_size + 4);
> +        g_assert(SPICE_ALIGNED_CAST(struct usb_redir_header *, data)->type == usb_redir_hello);
> +
> +        const uint32_t flags =
> +            usbredirparser_fl_write_cb_owns_buffer | usbredirparser_fl_usb_host |
> +            usbredirparser_fl_no_hello;
> +
> +        usbredirparser_init(ch->parser,
> +                            PACKAGE_STRING,
> +                            SPICE_ALIGNED_CAST(uint32_t *, data + hello_size),
> +                            (count - hello_size) / sizeof(uint32_t),
> +                            flags);
> +
> +        return 0;

Why return 0 and not just fall through?
If a write retry is necessary, could you add a comment explaining why?

Side comment: usbredir_write_callback used to be a mere wrapper around
spice_usbredir_write_callback. Now, it has a whole lot of logic in it.
Is it intentional, or should some of that logic be moved to shared code?


>      }
>      res = spice_usbredir_write_callback(ch->user_data, data, count);
>      return res;
> @@ -707,31 +729,33 @@ int spice_usb_backend_read_guest_data(SpiceUsbBackendChannel *ch, uint8_t *data,
>
>      ch->read_buf = data;
>      ch->read_buf_size = count;
> -    if (ch->usbredirhost) {
> -        res = usbredirhost_read_guest_data(ch->usbredirhost);
> -        if (!ch->hello_done_parser) {
> -            int res_parser;
> +    if (SPICE_UNLIKELY(ch->state == USB_CHANNEL_STATE_INITIALIZING)) {
> +        if (ch->usbredirhost != NULL) {
> +            res = usbredirhost_read_guest_data(ch->usbredirhost);
> +            if (res != 0) {
> +                return res;
> +            }
> +            ch->state = USB_CHANNEL_STATE_HOST;
> +
>              /* usbredirhost should consume hello response */
>              g_return_val_if_fail(ch->read_buf == NULL, USB_REDIR_ERROR_READ_PARSE);
> +        } else {
> +            ch->state = USB_CHANNEL_STATE_PARSER;
> +        }
>
> -            ch->read_buf = data;
> -            ch->read_buf_size = count;
> -            ch->hello_done_parser = 1;
> -            if (ch->attached && ch->attached->edev) {
> -                /* case of CD sharing on connect */
> -                ch->usbredirhost = NULL;
> -                ch->parser = ch->hidden_parser;
> -                SPICE_DEBUG("%s: switch %p to parser", __FUNCTION__, ch);
> -            }
> -            res_parser = usbredirparser_do_read(ch->hidden_parser);
> -            if (res >= 0) {
> -                res = res_parser;
> -            }
> +        ch->read_buf = data;
> +        ch->read_buf_size = count;
> +        if (ch->attached && ch->attached->edev) {
> +            /* case of CD sharing on connect */
> +            ch->state = USB_CHANNEL_STATE_PARSER;
> +            SPICE_DEBUG("%s: switch %p to parser", __FUNCTION__, ch);
>          }
> -    } else if (ch->parser) {
> -        res = usbredirparser_do_read(ch->parser);
> +        return usbredirparser_do_read(ch->parser);
> +    }
> +    if (ch->state == USB_CHANNEL_STATE_HOST) {
> +        res = usbredirhost_read_guest_data(ch->usbredirhost);
>      } else {
> -        res = USB_REDIR_ERROR_IO;
> +        res = usbredirparser_do_read(ch->parser);
>      }
>      switch (res)
>      {
> @@ -783,14 +807,12 @@ GError *spice_usb_backend_get_error_details(int error_code, gchar *desc)
>
>  void spice_usb_backend_return_write_data(SpiceUsbBackendChannel *ch, void *data)
>  {
> -    if (ch->usbredirhost) {
> +    if (ch->state == USB_CHANNEL_STATE_HOST) {
>          SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
>          usbredirhost_free_write_buffer(ch->usbredirhost, data);
> -    } else if (ch->parser) {
> +    } else {
>          SPICE_DEBUG("%s ch %p -> parser", __FUNCTION__, ch);
>          usbredirparser_free_write_buffer(ch->parser, data);
> -    } else {
> -        SPICE_DEBUG("%s ch %p - NOBODY TO CALL", __FUNCTION__, ch);
>      }
>  }
>
> @@ -1005,10 +1027,10 @@ static void usbredir_device_disconnect_ack(void *priv)
>  {
>      SpiceUsbBackendChannel *ch = priv;
>      SPICE_DEBUG("%s ch %p", __FUNCTION__, ch);
> -    if (ch->parser && ch->wait_disconnect_ack) {
> -        ch->parser = NULL;
> +    if (ch->state == USB_CHANNEL_STATE_PARSER && ch->usbredirhost != NULL &&
> +        ch->wait_disconnect_ack) {
> +        ch->state = USB_CHANNEL_STATE_HOST;
>          SPICE_DEBUG("%s switch to usbredirhost", __FUNCTION__);
> -        ch->usbredirhost = ch->hidden_host;
>      }
>      ch->wait_disconnect_ack = 0;
>  }
> @@ -1027,9 +1049,6 @@ usbredir_hello(void *priv, struct usb_redir_hello_header *hello)
>      SPICE_DEBUG("%s %p %sattached %s", __FUNCTION__, ch,
>          edev ? "" : "not ",  hello ? "" : "(internal)");
>
> -    if (hello) {
> -        ch->hello_done_parser = 1;
> -    }
>      if (!edev) {
>          return;
>      }
> @@ -1079,53 +1098,24 @@ usbredir_hello(void *priv, struct usb_redir_hello_header *hello)
>      usbredir_write_flush_callback(ch);
>  }
>
> -static void initialize_parser(SpiceUsbBackendChannel *ch, gboolean do_hello)
> +static void initialize_parser(SpiceUsbBackendChannel *ch)
>  {
>      uint32_t flags, caps[USB_REDIR_CAPS_SIZE] = { 0 };
>
> -    g_return_if_fail(ch->hidden_parser != NULL);
> -    if (ch->parser_init_done) {
> -        if (ch->parser_with_hello != do_hello) {
> -            g_return_if_reached();
> -        }
> -        return;
> -    }
> +    g_assert(ch->usbredirhost == NULL);
>
> -    ch->parser_init_done = 1;
> -    ch->parser_with_hello = do_hello;
>      flags = usbredirparser_fl_write_cb_owns_buffer | usbredirparser_fl_usb_host;
>
> -    if (do_hello) {
> -        ch->hello_sent = 1;
> -        ch->host_caps |= 1 << usb_redir_cap_connect_device_version;
> -        ch->host_caps |= 1 << usb_redir_cap_device_disconnect_ack;
> -        ch->host_caps |= 1 << usb_redir_cap_ep_info_max_packet_size;
> -        ch->host_caps |= 1 << usb_redir_cap_64bits_ids;
> -        ch->host_caps |= 1 << usb_redir_cap_32bits_bulk_length;
> -    } else {
> -        flags |= usbredirparser_fl_no_hello;
> -    }
> -
> -    if (ch->host_caps & (1 << usb_redir_cap_connect_device_version)) {
> -        usbredirparser_caps_set_cap(caps, usb_redir_cap_connect_device_version);
> -    }
> +    usbredirparser_caps_set_cap(caps, usb_redir_cap_connect_device_version);
>      usbredirparser_caps_set_cap(caps, usb_redir_cap_filter);
> -    if (ch->host_caps & (1 << usb_redir_cap_device_disconnect_ack)) {
> -        usbredirparser_caps_set_cap(caps, usb_redir_cap_device_disconnect_ack);
> -    }
> -    if (ch->host_caps & (1 << usb_redir_cap_ep_info_max_packet_size)) {
> -        usbredirparser_caps_set_cap(caps, usb_redir_cap_ep_info_max_packet_size);
> -    }
> -    if (ch->host_caps & (1 << usb_redir_cap_64bits_ids)) {
> -        usbredirparser_caps_set_cap(caps, usb_redir_cap_64bits_ids);
> -    }
> -    if (ch->host_caps & (1 << usb_redir_cap_32bits_bulk_length)) {
> -        usbredirparser_caps_set_cap(caps, usb_redir_cap_32bits_bulk_length);
> -    }
> -    if (ch->host_caps & (1 << usb_redir_cap_bulk_streams)) {
> -        usbredirparser_caps_set_cap(caps, usb_redir_cap_bulk_streams);
> -    }
> -    usbredirparser_init(ch->hidden_parser, PACKAGE_STRING, caps, USB_REDIR_CAPS_SIZE, flags);
> +    usbredirparser_caps_set_cap(caps, usb_redir_cap_device_disconnect_ack);
> +    usbredirparser_caps_set_cap(caps, usb_redir_cap_ep_info_max_packet_size);
> +    usbredirparser_caps_set_cap(caps, usb_redir_cap_64bits_ids);
> +    usbredirparser_caps_set_cap(caps, usb_redir_cap_32bits_bulk_length);
> +    usbredirparser_caps_set_cap(caps, usb_redir_cap_bulk_receiving);
> +    usbredirparser_caps_set_cap(caps, usb_redir_cap_bulk_streams);
> +
> +    usbredirparser_init(ch->parser, PACKAGE_STRING, caps, USB_REDIR_CAPS_SIZE, flags);
>  }
>
>  /*
> @@ -1180,7 +1170,7 @@ static gboolean attach_edev(SpiceUsbBackendChannel *ch,
>             _("Failed to redirect device %d"), 1);
>          return FALSE;
>      }
> -    if (ch->usbredirhost && !ch->hello_done_parser) {
> +    if (ch->state == USB_CHANNEL_STATE_INITIALIZING) {
>          /*
>              we can't initialize parser until we see hello from usbredir
>              and the parser can't work until it sees the hello response.
> @@ -1190,15 +1180,13 @@ static gboolean attach_edev(SpiceUsbBackendChannel *ch,
>          SPICE_DEBUG("%s waiting until the channel is ready", __FUNCTION__);
>
>      } else {
> -        initialize_parser(ch, ch->hidden_host == NULL);
> -        ch->usbredirhost = NULL;
> -        ch->parser = ch->hidden_parser;
> +        ch->state = USB_CHANNEL_STATE_PARSER;
>      }
>      ch->wait_disconnect_ack = 0;
>      ch->attached = dev;
>      dev->attached_to = ch;
> -    device_ops(dev->edev)->attach(dev->edev, ch->hidden_parser);
> -    if (ch->hello_done_parser) {
> +    device_ops(dev->edev)->attach(dev->edev, ch->parser);
> +    if (ch->state == USB_CHANNEL_STATE_PARSER) {
>          /* send device info */
>          usbredir_hello(ch, NULL);
>      }
> @@ -1218,9 +1206,15 @@ gboolean spice_usb_backend_channel_attach(SpiceUsbBackendChannel *ch,
>          return attach_edev(ch, dev, error);
>      }
>
> +    // no physical device enabled
> +    if (ch->usbredirhost == NULL) {
> +        return FALSE;
> +    }
> +
>      libusb_device_handle *handle = NULL;
> -    ch->usbredirhost = ch->hidden_host;
> -    ch->parser = NULL;
> +    if (ch->state != USB_CHANNEL_STATE_INITIALIZING) {
> +        ch->state = USB_CHANNEL_STATE_HOST;
> +    }
>
>      /*
>         Under Windows we need to avoid updating
> @@ -1261,10 +1255,10 @@ void spice_usb_backend_channel_detach(SpiceUsbBackendChannel *ch)
>          SPICE_DEBUG("%s: nothing to detach", __FUNCTION__);
>          return;
>      }
> -    if (ch->usbredirhost) {
> +    if (ch->state == USB_CHANNEL_STATE_HOST) {
>          /* it will call libusb_close internally */
>          usbredirhost_set_device(ch->usbredirhost, NULL);
> -    } else if (ch->parser) {
> +    } else {
>          if (edev) {
>              device_ops(edev)->detach(edev);
>          }
> @@ -1272,9 +1266,8 @@ void spice_usb_backend_channel_detach(SpiceUsbBackendChannel *ch)
>          usbredir_write_flush_callback(ch);
>          ch->wait_disconnect_ack =
>              usbredirparser_peer_has_cap(ch->parser, usb_redir_cap_device_disconnect_ack);
> -        if (!ch->wait_disconnect_ack) {
> -            ch->usbredirhost = ch->hidden_host;
> -            ch->parser = NULL;
> +        if (!ch->wait_disconnect_ack && ch->usbredirhost != NULL) {
> +            ch->state = USB_CHANNEL_STATE_HOST;
>          }
>      }
>      SPICE_DEBUG("%s ch %p, detach done", __FUNCTION__, ch);
> @@ -1311,16 +1304,22 @@ SpiceUsbBackendChannel *spice_usb_backend_channel_new(SpiceUsbBackend *be,
>                                          usbredirparser_warning,
>                                     usbredirhost_fl_write_cb_owns_buffer);
>          g_warn_if_fail(ch->usbredirhost != NULL);
> +        if (ch->usbredirhost != NULL) {
> +            usbredirhost_set_buffered_output_size_cb(ch->usbredirhost,
> +                                                     usbredir_buffered_output_size_callback);
> +            // force flush of HELLO packet and creation of parser
> +            usbredirhost_write_guest_data(ch->usbredirhost);
> +        }
> +    } else {
> +        // no physical device support, only emulated, create the
> +        // parser
> +        ch->parser = create_parser(ch);
> +        if (ch->parser != NULL) {
> +            initialize_parser(ch);
> +        }
>      }
>
> -    if (ch->usbredirhost) {
> -        ch->hidden_parser = create_parser(ch);
> -        ch->hidden_host = ch->usbredirhost;
> -        usbredirhost_set_buffered_output_size_cb(ch->usbredirhost,
> -                                                 usbredir_buffered_output_size_callback);
> -    }
> -
> -    if (!ch->hidden_parser) {
> +    if (!ch->parser) {
>          spice_usb_backend_channel_delete(ch);
>          ch = NULL;
>      }
> @@ -1332,12 +1331,9 @@ SpiceUsbBackendChannel *spice_usb_backend_channel_new(SpiceUsbBackend *be,
>  void spice_usb_backend_channel_flush_writes(SpiceUsbBackendChannel *ch)
>  {
>      SPICE_DEBUG("%s %p is up", __FUNCTION__, ch);
> -    if (ch->usbredirhost) {
> +    if (ch->state != USB_CHANNEL_STATE_PARSER && ch->usbredirhost != NULL) {
>          usbredirhost_write_guest_data(ch->usbredirhost);
> -        initialize_parser(ch, FALSE);
>      } else {
> -        initialize_parser(ch, TRUE);
> -        ch->parser = ch->hidden_parser;
>          usbredirparser_do_write(ch->parser);
>      }
>  }
> @@ -1348,11 +1344,11 @@ void spice_usb_backend_channel_delete(SpiceUsbBackendChannel *ch)
>      if (!ch) {
>          return;
>      }
> -    if (ch->hidden_host) {
> -        usbredirhost_close(ch->hidden_host);
> +    if (ch->usbredirhost) {
> +        usbredirhost_close(ch->usbredirhost);
>      }
> -    if (ch->hidden_parser) {
> -        usbredirparser_destroy(ch->hidden_parser);
> +    if (ch->parser) {
> +        usbredirparser_destroy(ch->parser);
>      }
>
>      if (ch->rules) {
> @@ -1372,7 +1368,7 @@ spice_usb_backend_channel_get_guest_filter(SpiceUsbBackendChannel *ch,
>      int i;
>      *r = NULL;
>      *count = 0;
> -    if (ch->usbredirhost) {
> +    if (ch->usbredirhost != NULL) {
>          usbredirhost_get_guest_filter(ch->usbredirhost, r, count);
>      }
>      if (*r == NULL) {


--
Cheers,
Christophe de Dinechin (IRC c3d)
> 
> Frediano Ziglio writes:
> 
> > Make initialisation easier.
> > Always initialise parser.
> 
> My spellchecker seems to choke on "initialise", suggests
> "initialize" (maybe a UK/US difference).
> 

Yes, mine complains the opposite :-)
Moved to US for consistency with code.

> > Initialise both parser and host during spice_usb_backend_channel_new.
> > Support not having libusb context (no physical devices).
> > Avoids too much state variables.
> 
> "Reduce number of state variables"?
> 

Yes

> Based on the code, I'd go as far as stating "Simplify state machine logic".
> 
> > parser is always initialised after creation making sure the state
> > is consistent.
> 
> That seems redundant with "Always initialize parser".
> 

Indeed, improved commit message

> 
> > Use a single state variable, data flows into/from a single parser.
> >
> > Signed-off-by: Frediano Ziglio <fziglio@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,
> 
> (You seem to have used "initialize" with a "z" here)
> 
> > +    USB_CHANNEL_STATE_HOST,
> > +    USB_CHANNEL_STATE_PARSER,
> 
> I can't say that "STATE_HOST" and "STATE_PARSER" make much sense to me.
> Maybe something like "STATE_USBREDIRHOST" and "STATE_PARSING" or
> "STATE_PARSER_ACTIVE"?
> 

I was thinking about having a "bool initialized" and a "use usbredirhost/parser"
instead of just one. Technically both usbredirhost and parser are parsing the
packets and also during initialization so "STATE_PARSING" does not make sense.
The difference is that usbredirhost consumes the parsed messages "by himself"
(it implements the parser callbacks) while for emulated device we use another
parser and we provides the callbacks.
The reason for the "initializing" state is that the initial hello packet is
queued in either usbredirhost (we support physical devices) or parser (no
physical devices).

> > +} 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;
> 
> Nice simplification!
> 
> Would it make sense to use "bool" for flags?
> 

I proposed too to use bools.

> >      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) {
> > +        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);
> > +        g_assert(count >= hello_size + 4);
> > +        g_assert(SPICE_ALIGNED_CAST(struct usb_redir_header *, data)->type
> > == usb_redir_hello);
> > +
> > +        const uint32_t flags =
> > +            usbredirparser_fl_write_cb_owns_buffer |
> > usbredirparser_fl_usb_host |
> > +            usbredirparser_fl_no_hello;
> > +
> > +        usbredirparser_init(ch->parser,
> > +                            PACKAGE_STRING,
> > +                            SPICE_ALIGNED_CAST(uint32_t *, data +
> > hello_size),
> > +                            (count - hello_size) / sizeof(uint32_t),
> > +                            flags);
> > +
> > +        return 0;
> 
> Why return 0 and not just fall through?
> If a write retry is necessary, could you add a comment explaining why?
> 

added a comment

> Side comment: usbredir_write_callback used to be a mere wrapper around
> spice_usbredir_write_callback. Now, it has a whole lot of logic in it.
> Is it intentional, or should some of that logic be moved to shared code?
> 

Yes, intentional (otherwise why changing the code?).
This code is not shared with anything. The only reason to put in a separate
function is separation, not sharing.
I have the feeling I didn't get what you wanted to say.

> 
> >      }
> >      res = spice_usbredir_write_callback(ch->user_data, data, count);
> >      return res;
> > @@ -707,31 +729,33 @@ int
> > spice_usb_backend_read_guest_data(SpiceUsbBackendChannel *ch, uint8_t
> > *data,
> >
> >      ch->read_buf = data;
> >      ch->read_buf_size = count;
> > -    if (ch->usbredirhost) {
> > -        res = usbredirhost_read_guest_data(ch->usbredirhost);
> > -        if (!ch->hello_done_parser) {
> > -            int res_parser;
> > +    if (SPICE_UNLIKELY(ch->state == USB_CHANNEL_STATE_INITIALIZING)) {
> > +        if (ch->usbredirhost != NULL) {
> > +            res = usbredirhost_read_guest_data(ch->usbredirhost);
> > +            if (res != 0) {
> > +                return res;
> > +            }
> > +            ch->state = USB_CHANNEL_STATE_HOST;
> > +
> >              /* usbredirhost should consume hello response */
> >              g_return_val_if_fail(ch->read_buf == NULL,
> >              USB_REDIR_ERROR_READ_PARSE);
> > +        } else {
> > +            ch->state = USB_CHANNEL_STATE_PARSER;
> > +        }
> >
> > -            ch->read_buf = data;
> > -            ch->read_buf_size = count;
> > -            ch->hello_done_parser = 1;
> > -            if (ch->attached && ch->attached->edev) {
> > -                /* case of CD sharing on connect */
> > -                ch->usbredirhost = NULL;
> > -                ch->parser = ch->hidden_parser;
> > -                SPICE_DEBUG("%s: switch %p to parser", __FUNCTION__, ch);
> > -            }
> > -            res_parser = usbredirparser_do_read(ch->hidden_parser);
> > -            if (res >= 0) {
> > -                res = res_parser;
> > -            }
> > +        ch->read_buf = data;
> > +        ch->read_buf_size = count;
> > +        if (ch->attached && ch->attached->edev) {
> > +            /* case of CD sharing on connect */
> > +            ch->state = USB_CHANNEL_STATE_PARSER;
> > +            SPICE_DEBUG("%s: switch %p to parser", __FUNCTION__, ch);
> >          }
> > -    } else if (ch->parser) {
> > -        res = usbredirparser_do_read(ch->parser);
> > +        return usbredirparser_do_read(ch->parser);
> > +    }
> > +    if (ch->state == USB_CHANNEL_STATE_HOST) {
> > +        res = usbredirhost_read_guest_data(ch->usbredirhost);
> >      } else {
> > -        res = USB_REDIR_ERROR_IO;
> > +        res = usbredirparser_do_read(ch->parser);
> >      }
> >      switch (res)
> >      {
> > @@ -783,14 +807,12 @@ GError *spice_usb_backend_get_error_details(int
> > error_code, gchar *desc)
> >
> >  void spice_usb_backend_return_write_data(SpiceUsbBackendChannel *ch, void
> >  *data)
> >  {
> > -    if (ch->usbredirhost) {
> > +    if (ch->state == USB_CHANNEL_STATE_HOST) {
> >          SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
> >          usbredirhost_free_write_buffer(ch->usbredirhost, data);
> > -    } else if (ch->parser) {
> > +    } else {
> >          SPICE_DEBUG("%s ch %p -> parser", __FUNCTION__, ch);
> >          usbredirparser_free_write_buffer(ch->parser, data);
> > -    } else {
> > -        SPICE_DEBUG("%s ch %p - NOBODY TO CALL", __FUNCTION__, ch);
> >      }
> >  }
> >
> > @@ -1005,10 +1027,10 @@ static void usbredir_device_disconnect_ack(void
> > *priv)
> >  {
> >      SpiceUsbBackendChannel *ch = priv;
> >      SPICE_DEBUG("%s ch %p", __FUNCTION__, ch);
> > -    if (ch->parser && ch->wait_disconnect_ack) {
> > -        ch->parser = NULL;
> > +    if (ch->state == USB_CHANNEL_STATE_PARSER && ch->usbredirhost != NULL
> > &&
> > +        ch->wait_disconnect_ack) {
> > +        ch->state = USB_CHANNEL_STATE_HOST;
> >          SPICE_DEBUG("%s switch to usbredirhost", __FUNCTION__);
> > -        ch->usbredirhost = ch->hidden_host;
> >      }
> >      ch->wait_disconnect_ack = 0;
> >  }
> > @@ -1027,9 +1049,6 @@ usbredir_hello(void *priv, struct
> > usb_redir_hello_header *hello)
> >      SPICE_DEBUG("%s %p %sattached %s", __FUNCTION__, ch,
> >          edev ? "" : "not ",  hello ? "" : "(internal)");
> >
> > -    if (hello) {
> > -        ch->hello_done_parser = 1;
> > -    }
> >      if (!edev) {
> >          return;
> >      }
> > @@ -1079,53 +1098,24 @@ usbredir_hello(void *priv, struct
> > usb_redir_hello_header *hello)
> >      usbredir_write_flush_callback(ch);
> >  }
> >
> > -static void initialize_parser(SpiceUsbBackendChannel *ch, gboolean
> > do_hello)
> > +static void initialize_parser(SpiceUsbBackendChannel *ch)
> >  {
> >      uint32_t flags, caps[USB_REDIR_CAPS_SIZE] = { 0 };
> >
> > -    g_return_if_fail(ch->hidden_parser != NULL);
> > -    if (ch->parser_init_done) {
> > -        if (ch->parser_with_hello != do_hello) {
> > -            g_return_if_reached();
> > -        }
> > -        return;
> > -    }
> > +    g_assert(ch->usbredirhost == NULL);
> >
> > -    ch->parser_init_done = 1;
> > -    ch->parser_with_hello = do_hello;
> >      flags = usbredirparser_fl_write_cb_owns_buffer |
> >      usbredirparser_fl_usb_host;
> >
> > -    if (do_hello) {
> > -        ch->hello_sent = 1;
> > -        ch->host_caps |= 1 << usb_redir_cap_connect_device_version;
> > -        ch->host_caps |= 1 << usb_redir_cap_device_disconnect_ack;
> > -        ch->host_caps |= 1 << usb_redir_cap_ep_info_max_packet_size;
> > -        ch->host_caps |= 1 << usb_redir_cap_64bits_ids;
> > -        ch->host_caps |= 1 << usb_redir_cap_32bits_bulk_length;
> > -    } else {
> > -        flags |= usbredirparser_fl_no_hello;
> > -    }
> > -
> > -    if (ch->host_caps & (1 << usb_redir_cap_connect_device_version)) {
> > -        usbredirparser_caps_set_cap(caps,
> > usb_redir_cap_connect_device_version);
> > -    }
> > +    usbredirparser_caps_set_cap(caps,
> > usb_redir_cap_connect_device_version);
> >      usbredirparser_caps_set_cap(caps, usb_redir_cap_filter);
> > -    if (ch->host_caps & (1 << usb_redir_cap_device_disconnect_ack)) {
> > -        usbredirparser_caps_set_cap(caps,
> > usb_redir_cap_device_disconnect_ack);
> > -    }
> > -    if (ch->host_caps & (1 << usb_redir_cap_ep_info_max_packet_size)) {
> > -        usbredirparser_caps_set_cap(caps,
> > usb_redir_cap_ep_info_max_packet_size);
> > -    }
> > -    if (ch->host_caps & (1 << usb_redir_cap_64bits_ids)) {
> > -        usbredirparser_caps_set_cap(caps, usb_redir_cap_64bits_ids);
> > -    }
> > -    if (ch->host_caps & (1 << usb_redir_cap_32bits_bulk_length)) {
> > -        usbredirparser_caps_set_cap(caps,
> > usb_redir_cap_32bits_bulk_length);
> > -    }
> > -    if (ch->host_caps & (1 << usb_redir_cap_bulk_streams)) {
> > -        usbredirparser_caps_set_cap(caps, usb_redir_cap_bulk_streams);
> > -    }
> > -    usbredirparser_init(ch->hidden_parser, PACKAGE_STRING, caps,
> > USB_REDIR_CAPS_SIZE, flags);
> > +    usbredirparser_caps_set_cap(caps,
> > usb_redir_cap_device_disconnect_ack);
> > +    usbredirparser_caps_set_cap(caps,
> > usb_redir_cap_ep_info_max_packet_size);
> > +    usbredirparser_caps_set_cap(caps, usb_redir_cap_64bits_ids);
> > +    usbredirparser_caps_set_cap(caps, usb_redir_cap_32bits_bulk_length);
> > +    usbredirparser_caps_set_cap(caps, usb_redir_cap_bulk_receiving);
> > +    usbredirparser_caps_set_cap(caps, usb_redir_cap_bulk_streams);
> > +
> > +    usbredirparser_init(ch->parser, PACKAGE_STRING, caps,
> > USB_REDIR_CAPS_SIZE, flags);
> >  }
> >
> >  /*
> > @@ -1180,7 +1170,7 @@ static gboolean attach_edev(SpiceUsbBackendChannel
> > *ch,
> >             _("Failed to redirect device %d"), 1);
> >          return FALSE;
> >      }
> > -    if (ch->usbredirhost && !ch->hello_done_parser) {
> > +    if (ch->state == USB_CHANNEL_STATE_INITIALIZING) {
> >          /*
> >              we can't initialize parser until we see hello from usbredir
> >              and the parser can't work until it sees the hello response.
> > @@ -1190,15 +1180,13 @@ static gboolean attach_edev(SpiceUsbBackendChannel
> > *ch,
> >          SPICE_DEBUG("%s waiting until the channel is ready",
> >          __FUNCTION__);
> >
> >      } else {
> > -        initialize_parser(ch, ch->hidden_host == NULL);
> > -        ch->usbredirhost = NULL;
> > -        ch->parser = ch->hidden_parser;
> > +        ch->state = USB_CHANNEL_STATE_PARSER;
> >      }
> >      ch->wait_disconnect_ack = 0;
> >      ch->attached = dev;
> >      dev->attached_to = ch;
> > -    device_ops(dev->edev)->attach(dev->edev, ch->hidden_parser);
> > -    if (ch->hello_done_parser) {
> > +    device_ops(dev->edev)->attach(dev->edev, ch->parser);
> > +    if (ch->state == USB_CHANNEL_STATE_PARSER) {
> >          /* send device info */
> >          usbredir_hello(ch, NULL);
> >      }
> > @@ -1218,9 +1206,15 @@ gboolean
> > spice_usb_backend_channel_attach(SpiceUsbBackendChannel *ch,
> >          return attach_edev(ch, dev, error);
> >      }
> >
> > +    // no physical device enabled
> > +    if (ch->usbredirhost == NULL) {
> > +        return FALSE;
> > +    }
> > +
> >      libusb_device_handle *handle = NULL;
> > -    ch->usbredirhost = ch->hidden_host;
> > -    ch->parser = NULL;
> > +    if (ch->state != USB_CHANNEL_STATE_INITIALIZING) {
> > +        ch->state = USB_CHANNEL_STATE_HOST;
> > +    }
> >
> >      /*
> >         Under Windows we need to avoid updating
> > @@ -1261,10 +1255,10 @@ void
> > spice_usb_backend_channel_detach(SpiceUsbBackendChannel *ch)
> >          SPICE_DEBUG("%s: nothing to detach", __FUNCTION__);
> >          return;
> >      }
> > -    if (ch->usbredirhost) {
> > +    if (ch->state == USB_CHANNEL_STATE_HOST) {
> >          /* it will call libusb_close internally */
> >          usbredirhost_set_device(ch->usbredirhost, NULL);
> > -    } else if (ch->parser) {
> > +    } else {
> >          if (edev) {
> >              device_ops(edev)->detach(edev);
> >          }
> > @@ -1272,9 +1266,8 @@ void
> > spice_usb_backend_channel_detach(SpiceUsbBackendChannel *ch)
> >          usbredir_write_flush_callback(ch);
> >          ch->wait_disconnect_ack =
> >              usbredirparser_peer_has_cap(ch->parser,
> >              usb_redir_cap_device_disconnect_ack);
> > -        if (!ch->wait_disconnect_ack) {
> > -            ch->usbredirhost = ch->hidden_host;
> > -            ch->parser = NULL;
> > +        if (!ch->wait_disconnect_ack && ch->usbredirhost != NULL) {
> > +            ch->state = USB_CHANNEL_STATE_HOST;
> >          }
> >      }
> >      SPICE_DEBUG("%s ch %p, detach done", __FUNCTION__, ch);
> > @@ -1311,16 +1304,22 @@ SpiceUsbBackendChannel
> > *spice_usb_backend_channel_new(SpiceUsbBackend *be,
> >                                          usbredirparser_warning,
> >                                     usbredirhost_fl_write_cb_owns_buffer);
> >          g_warn_if_fail(ch->usbredirhost != NULL);
> > +        if (ch->usbredirhost != NULL) {
> > +            usbredirhost_set_buffered_output_size_cb(ch->usbredirhost,
> > +
> > usbredir_buffered_output_size_callback);
> > +            // force flush of HELLO packet and creation of parser
> > +            usbredirhost_write_guest_data(ch->usbredirhost);
> > +        }
> > +    } else {
> > +        // no physical device support, only emulated, create the
> > +        // parser
> > +        ch->parser = create_parser(ch);
> > +        if (ch->parser != NULL) {
> > +            initialize_parser(ch);
> > +        }
> >      }
> >
> > -    if (ch->usbredirhost) {
> > -        ch->hidden_parser = create_parser(ch);
> > -        ch->hidden_host = ch->usbredirhost;
> > -        usbredirhost_set_buffered_output_size_cb(ch->usbredirhost,
> > -
> > usbredir_buffered_output_size_callback);
> > -    }
> > -
> > -    if (!ch->hidden_parser) {
> > +    if (!ch->parser) {
> >          spice_usb_backend_channel_delete(ch);
> >          ch = NULL;
> >      }
> > @@ -1332,12 +1331,9 @@ SpiceUsbBackendChannel
> > *spice_usb_backend_channel_new(SpiceUsbBackend *be,
> >  void spice_usb_backend_channel_flush_writes(SpiceUsbBackendChannel *ch)
> >  {
> >      SPICE_DEBUG("%s %p is up", __FUNCTION__, ch);
> > -    if (ch->usbredirhost) {
> > +    if (ch->state != USB_CHANNEL_STATE_PARSER && ch->usbredirhost != NULL)
> > {
> >          usbredirhost_write_guest_data(ch->usbredirhost);
> > -        initialize_parser(ch, FALSE);
> >      } else {
> > -        initialize_parser(ch, TRUE);
> > -        ch->parser = ch->hidden_parser;
> >          usbredirparser_do_write(ch->parser);
> >      }
> >  }
> > @@ -1348,11 +1344,11 @@ void
> > spice_usb_backend_channel_delete(SpiceUsbBackendChannel *ch)
> >      if (!ch) {
> >          return;
> >      }
> > -    if (ch->hidden_host) {
> > -        usbredirhost_close(ch->hidden_host);
> > +    if (ch->usbredirhost) {
> > +        usbredirhost_close(ch->usbredirhost);
> >      }
> > -    if (ch->hidden_parser) {
> > -        usbredirparser_destroy(ch->hidden_parser);
> > +    if (ch->parser) {
> > +        usbredirparser_destroy(ch->parser);
> >      }
> >
> >      if (ch->rules) {
> > @@ -1372,7 +1368,7 @@
> > spice_usb_backend_channel_get_guest_filter(SpiceUsbBackendChannel *ch,
> >      int i;
> >      *r = NULL;
> >      *count = 0;
> > -    if (ch->usbredirhost) {
> > +    if (ch->usbredirhost != NULL) {
> >          usbredirhost_get_guest_filter(ch->usbredirhost, r, count);
> >      }
> >      if (*r == NULL) {
> 

There are still some minor weirdness in the initial patch.
Like why usbredir_hello is called with a NULL parameter instead of having
a separate "initialize_edev" or similar.
Or why parser code calls a lot usbredir_write_flush_callback which was 
previously called only by usbredirhost and is supposed to dispatch between
usbredirhost or parser why from the parser is called only to flush from
parser if channel is ready.

Frediano


Preamble: Apologies for screwing up the formatting so badly in my last
reply. Trying to repair a little manually here.

Frediano Ziglio writes:

> On 28 Aug 2019, at 12:16, Frediano Ziglio <fziglio@redhat.com> wrote:
> > > I have the feeling I didn't get what you wanted to say.
>
> > I meant: what is the separation of concerns between
> > usbredir_write_callback and spice_usbredir_write_callback?
> ABI. usbredir_write_callback is a callback for usbredir layer, spice_usbredir_write_callback
> expects a SpiceUsbredirChannel.

Well, this is not really a "separation of concerns" explanation, more
like information about how the implementation evolved. My question was
more about what responsibility each component had.

That does not invalidate your answer, which I read as "it's not
practical to change the ABI for that change". But I think that
the real answer to my question is at the end, please check if I
understood correctly.


> > In what context would spice_usbredir_write_callback be used
> > where the new logic in usbredir_write_callback is not necessary?
>
> You need to adjust to the ABI of the two. spice_usbredir_write_callback is supposed
> to write the packet to the guest (or handle the packet anyway).

Yes, but I'm trying to understand what happens for example if there is a
version mismatch. Who deals with the HELLO message?


> The new logic (added by Yuri patch, not changed here by this patch) is here to handle
> the initial HELLO packet.

True, but it's while reviewing your patch that I noticed all the added
logic and tried to understand where this was going, hence my questions
;-)


> > If the way you organized the code is somehow better, given that
> > usbredir_write_callback is no longer a simple wrapper, it may
> > indicate that additional comments are required to explain what
> > each does. Or maybe it's perfectly clear to everyone but me ;-)

> I suppose the "handle first packet (HELLO) creating parser from capabilities"
> is not enough. Would:
>
>     // Handle first packet (HELLO) creating parser from capabilities.
>     // If we are initializing and we don't have the parser we get the
>     // capabilities from the usbredirhost and use them to initialize
>     // the parser.
>
> be better?

It's slightly better, but I was more thinking about adding (later) a
comment to spice_usbredir_write_callback, in order to indicate that it
does NOT deal with the HELLO packet, i.e. that the data processing logic
is split between the two layers.


> > Hmmm. That tends to confirm the impression above that some
> > boundary is subtly moving between the components. But I'm not
> > really familiar enough with usbredir to understand the intent just from
> > the patches ;-)

> Mainly before the flow was a single one (guest <-> usbredirhost), now data can flow
> to/from the "parser" to support emulated devices.

I think that it's "to support emulated devices" that is really important
here. In terms of separation of concerns, that would mean that the
library does not care about that, and that only the GTK client provides the
logic for emulating devices. Is that a good way to describe it?

Would another theoretical client using the library never run into the
case because it would not expose the capabilities? Or is it just a case
that nobody cares about?

(Also, to be clear, I don't think this invalidates the patch at all, I'm
really asking questions to make sure I understand the logic and that the
way the code is organized is fully intentional).

--
Cheers,
Christophe de Dinechin (IRC c3d)
> 
> Preamble: Apologies for screwing up the formatting so badly in my last
> reply. Trying to repair a little manually here.
> 
> Frediano Ziglio writes:
> 
> > On 28 Aug 2019, at 12:16, Frediano Ziglio <fziglio@redhat.com> wrote:
> > > > I have the feeling I didn't get what you wanted to say.
> >
> > > I meant: what is the separation of concerns between
> > > usbredir_write_callback and spice_usbredir_write_callback?
> > ABI. usbredir_write_callback is a callback for usbredir layer,
> > spice_usbredir_write_callback
> > expects a SpiceUsbredirChannel.
> 
> Well, this is not really a "separation of concerns" explanation, more
> like information about how the implementation evolved. My question was
> more about what responsibility each component had.
> 
> That does not invalidate your answer, which I read as "it's not
> practical to change the ABI for that change". But I think that
> the real answer to my question is at the end, please check if I
> understood correctly.
> 
> 
> > > In what context would spice_usbredir_write_callback be used
> > > where the new logic in usbredir_write_callback is not necessary?
> >
> > You need to adjust to the ABI of the two. spice_usbredir_write_callback is
> > supposed
> > to write the packet to the guest (or handle the packet anyway).
> 
> Yes, but I'm trying to understand what happens for example if there is a
> version mismatch. Who deals with the HELLO message?
> 
> 
> > The new logic (added by Yuri patch, not changed here by this patch) is here
> > to handle
> > the initial HELLO packet.
> 
> True, but it's while reviewing your patch that I noticed all the added
> logic and tried to understand where this was going, hence my questions
> ;-)
> 
> 
> > > If the way you organized the code is somehow better, given that
> > > usbredir_write_callback is no longer a simple wrapper, it may
> > > indicate that additional comments are required to explain what
> > > each does. Or maybe it's perfectly clear to everyone but me ;-)
> 
> > I suppose the "handle first packet (HELLO) creating parser from
> > capabilities"
> > is not enough. Would:
> >
> >     // Handle first packet (HELLO) creating parser from capabilities.
> >     // If we are initializing and we don't have the parser we get the
> >     // capabilities from the usbredirhost and use them to initialize
> >     // the parser.
> >
> > be better?
> 
> It's slightly better, but I was more thinking about adding (later) a
> comment to spice_usbredir_write_callback, in order to indicate that it
> does NOT deal with the HELLO packet, i.e. that the data processing logic
> is split between the two layers.
> 

Oh... now I understand the confusion (I hope).
The if code returns always 0. So the message is not removed from the queue.
On the next call parser won't be NULL and the same HELLO packet will be
send to spice_usbredir_write_callback.
May be the "handle" in the comment is suggesting that the if code is also
consuming entirely the packet. Suggestions to improve the comment?

> 
> > > Hmmm. That tends to confirm the impression above that some
> > > boundary is subtly moving between the components. But I'm not
> > > really familiar enough with usbredir to understand the intent just from
> > > the patches ;-)
> 
> > Mainly before the flow was a single one (guest <-> usbredirhost), now data
> > can flow
> > to/from the "parser" to support emulated devices.
> 
> I think that it's "to support emulated devices" that is really important
> here. In terms of separation of concerns, that would mean that the
> library does not care about that, and that only the GTK client provides the
> logic for emulating devices. Is that a good way to describe it?
> 

The code is in spice-client-glib so not technically only GTK, all clients
that will use spice-client-glib. This "emulation" is transparent, for the
guest is like a remote physical device.

> Would another theoretical client using the library never run into the
> case because it would not expose the capabilities? Or is it just a case
> that nobody cares about?
> 

I don't understand. Do you mean the capabilities inside the HELLO packet?
In this case it's just the confusion about the "handle" above, the HELLO
is sent to the guest.

> (Also, to be clear, I don't think this invalidates the patch at all, I'm
> really asking questions to make sure I understand the logic and that the
> way the code is organized is fully intentional).
> 
> --
> Cheers,
> Christophe de Dinechin (IRC c3d)
> 

Frediano
Frediano Ziglio writes:

>>
>> It's slightly better, but I was more thinking about adding (later) a
>> comment to spice_usbredir_write_callback, in order to indicate that it
>> does NOT deal with the HELLO packet, i.e. that the data processing logic
>> is split between the two layers.
>>
>
> Oh... now I understand the confusion (I hope).

Actually, I think the confusing was me inferring too much from the name
spice_usbredir_write_callback, see below.


> The if code returns always 0. So the message is not removed from the queue.
> On the next call parser won't be NULL and the same HELLO packet will be
> send to spice_usbredir_write_callback.
> May be the "handle" in the comment is suggesting that the if code is also
> consuming entirely the packet. Suggestions to improve the comment?

This brings me back to my earlier question, why cannot you fall through, what
is the point of the "return 0"?

You replied "added a comment", but without sharing what the comment was,
so I still don't know ;-) Have you sent a follow-up patch that I missed?

>
>>
>> > > Hmmm. That tends to confirm the impression above that some
>> > > boundary is subtly moving between the components. But I'm not
>> > > really familiar enough with usbredir to understand the intent just from
>> > > the patches ;-)
>>
>> > Mainly before the flow was a single one (guest <-> usbredirhost), now data
>> > can flow
>> > to/from the "parser" to support emulated devices.
>>
>> I think that it's "to support emulated devices" that is really important
>> here. In terms of separation of concerns, that would mean that the
>> library does not care about that, and that only the GTK client provides the
>> logic for emulating devices. Is that a good way to describe it?
>>
>
> The code is in spice-client-glib so not technically only GTK, all clients
> that will use spice-client-glib. This "emulation" is transparent, for the
> guest is like a remote physical device.

Ah, I think I figured out what confused me. I assumed from the spice_
prefix that spice_usbredir_write_callback was a public API. So it did
not make sense to me to add some logic around it that would not be
done by the API itself. But spice_usbredir_write_callback is really just
an internal helper function that is neither public nor a callback, so it
does not really matter how you split the logic, it's invisible outside.


>
>> Would another theoretical client using the library never run into the
>> case because it would not expose the capabilities? Or is it just a case
>> that nobody cares about?
>>
>
> I don't understand. Do you mean the capabilities inside the HELLO packet?
> In this case it's just the confusion about the "handle" above, the HELLO
> is sent to the guest.

Actually, I was simply incorrectly assuming some other client could
refer to spice_usbredir_write_callback directly. I find it's not
appropriately named, since it's not really used as a callback, but
called directly, and because it has a "spice_" prefix that (to me)
implies it his "closer to the public API" than usbredir_write_callback,
when in fact it's further away from the public interface.


>
>> (Also, to be clear, I don't think this invalidates the patch at all, I'm
>> really asking questions to make sure I understand the logic and that the
>> way the code is organized is fully intentional).
>>
>> --
>> Cheers,
>> Christophe de Dinechin (IRC c3d)
>>
>
> Frediano


--
Cheers,
Christophe de Dinechin (IRC c3d)
> 
> Frediano Ziglio writes:
> 
> >>
> >> It's slightly better, but I was more thinking about adding (later) a
> >> comment to spice_usbredir_write_callback, in order to indicate that it
> >> does NOT deal with the HELLO packet, i.e. that the data processing logic
> >> is split between the two layers.
> >>
> >
> > Oh... now I understand the confusion (I hope).
> 
> Actually, I think the confusing was me inferring too much from the name
> spice_usbredir_write_callback, see below.
> 
> 
> > The if code returns always 0. So the message is not removed from the queue.
> > On the next call parser won't be NULL and the same HELLO packet will be
> > send to spice_usbredir_write_callback.
> > May be the "handle" in the comment is suggesting that the if code is also
> > consuming entirely the packet. Suggestions to improve the comment?
> 
> This brings me back to my earlier question, why cannot you fall through, what
> is the point of the "return 0"?
> 
> You replied "added a comment", but without sharing what the comment was,
> so I still don't know ;-) Have you sent a follow-up patch that I missed?
> 

Last patch I sent (maybe I sent later) has:

    // handle first packet (HELLO) creating parser from capabilities
    if (SPICE_UNLIKELY(ch->parser == NULL)) {
        // Here we return 0 from this function to keep the packet in
        // the queue. The packet will then be sent to the guest.
        // We are initializing SpiceUsbBackendChannel, the
        // SpiceUsbredirChannel is not ready to receive packets.

that last "SpiceUsbredirChannel is not ready to receive packets" is
the reason why not falling through.

> >
> >>
> >> > > Hmmm. That tends to confirm the impression above that some
> >> > > boundary is subtly moving between the components. But I'm not
> >> > > really familiar enough with usbredir to understand the intent just
> >> > > from
> >> > > the patches ;-)
> >>
> >> > Mainly before the flow was a single one (guest <-> usbredirhost), now
> >> > data
> >> > can flow
> >> > to/from the "parser" to support emulated devices.
> >>
> >> I think that it's "to support emulated devices" that is really important
> >> here. In terms of separation of concerns, that would mean that the
> >> library does not care about that, and that only the GTK client provides
> >> the
> >> logic for emulating devices. Is that a good way to describe it?
> >>
> >
> > The code is in spice-client-glib so not technically only GTK, all clients
> > that will use spice-client-glib. This "emulation" is transparent, for the
> > guest is like a remote physical device.
> 
> Ah, I think I figured out what confused me. I assumed from the spice_
> prefix that spice_usbredir_write_callback was a public API. So it did
> not make sense to me to add some logic around it that would not be
> done by the API itself. But spice_usbredir_write_callback is really just
> an internal helper function that is neither public nor a callback, so it
> does not really matter how you split the logic, it's invisible outside.
> 

I think the "spice_" prefix came from the fact that the class is public.
Why "_callback" suffix I have no idea, maybe was used directly. Maybe
is related to the fact the SpiceUsbredirChannel pointer in
SpiceUsbBackendChannel is "user_data".

> 
> >
> >> Would another theoretical client using the library never run into the
> >> case because it would not expose the capabilities? Or is it just a case
> >> that nobody cares about?
> >>
> >
> > I don't understand. Do you mean the capabilities inside the HELLO packet?
> > In this case it's just the confusion about the "handle" above, the HELLO
> > is sent to the guest.
> 
> Actually, I was simply incorrectly assuming some other client could
> refer to spice_usbredir_write_callback directly. I find it's not
> appropriately named, since it's not really used as a callback, but
> called directly, and because it has a "spice_" prefix that (to me)
> implies it his "closer to the public API" than usbredir_write_callback,
> when in fact it's further away from the public interface.
> 
> 
> >
> >> (Also, to be clear, I don't think this invalidates the patch at all, I'm
> >> really asking questions to make sure I understand the logic and that the
> >> way the code is organized is fully intentional).
> >>
> 
> --
> Cheers,
> Christophe de Dinechin (IRC c3d)
>
Frediano Ziglio writes:

>>
>> Frediano Ziglio writes:
>>
>> >>
>> >> It's slightly better, but I was more thinking about adding (later) a
>> >> comment to spice_usbredir_write_callback, in order to indicate that it
>> >> does NOT deal with the HELLO packet, i.e. that the data processing logic
>> >> is split between the two layers.
>> >>
>> >
>> > Oh... now I understand the confusion (I hope).
>>
>> Actually, I think the confusing was me inferring too much from the name
>> spice_usbredir_write_callback, see below.
>>
>>
>> > The if code returns always 0. So the message is not removed from the queue.
>> > On the next call parser won't be NULL and the same HELLO packet will be
>> > send to spice_usbredir_write_callback.
>> > May be the "handle" in the comment is suggesting that the if code is also
>> > consuming entirely the packet. Suggestions to improve the comment?
>>
>> This brings me back to my earlier question, why cannot you fall through, what
>> is the point of the "return 0"?
>>
>> You replied "added a comment", but without sharing what the comment was,
>> so I still don't know ;-) Have you sent a follow-up patch that I missed?
>>
>
> Last patch I sent (maybe I sent later) has:
>
>     // handle first packet (HELLO) creating parser from capabilities
>     if (SPICE_UNLIKELY(ch->parser == NULL)) {
>         // Here we return 0 from this function to keep the packet in
>         // the queue. The packet will then be sent to the guest.
>         // We are initializing SpiceUsbBackendChannel, the
>         // SpiceUsbredirChannel is not ready to receive packets.
>
> that last "SpiceUsbredirChannel is not ready to receive packets" is
> the reason why not falling through.

Cool. Thanks for explaining. The comment really helps.

>
>> >
>> >>
>> >> > > Hmmm. That tends to confirm the impression above that some
>> >> > > boundary is subtly moving between the components. But I'm not
>> >> > > really familiar enough with usbredir to understand the intent just
>> >> > > from
>> >> > > the patches ;-)
>> >>
>> >> > Mainly before the flow was a single one (guest <-> usbredirhost), now
>> >> > data
>> >> > can flow
>> >> > to/from the "parser" to support emulated devices.
>> >>
>> >> I think that it's "to support emulated devices" that is really important
>> >> here. In terms of separation of concerns, that would mean that the
>> >> library does not care about that, and that only the GTK client provides
>> >> the
>> >> logic for emulating devices. Is that a good way to describe it?
>> >>
>> >
>> > The code is in spice-client-glib so not technically only GTK, all clients
>> > that will use spice-client-glib. This "emulation" is transparent, for the
>> > guest is like a remote physical device.
>>
>> Ah, I think I figured out what confused me. I assumed from the spice_
>> prefix that spice_usbredir_write_callback was a public API. So it did
>> not make sense to me to add some logic around it that would not be
>> done by the API itself. But spice_usbredir_write_callback is really just
>> an internal helper function that is neither public nor a callback, so it
>> does not really matter how you split the logic, it's invisible outside.
>>
>
> I think the "spice_" prefix came from the fact that the class is public.
> Why "_callback" suffix I have no idea, maybe was used directly. Maybe
> is related to the fact the SpiceUsbredirChannel pointer in
> SpiceUsbBackendChannel is "user_data".

Maybe an opportunity for a later cleanup?

>
>>
>> >
>> >> Would another theoretical client using the library never run into the
>> >> case because it would not expose the capabilities? Or is it just a case
>> >> that nobody cares about?
>> >>
>> >
>> > I don't understand. Do you mean the capabilities inside the HELLO packet?
>> > In this case it's just the confusion about the "handle" above, the HELLO
>> > is sent to the guest.
>>
>> Actually, I was simply incorrectly assuming some other client could
>> refer to spice_usbredir_write_callback directly. I find it's not
>> appropriately named, since it's not really used as a callback, but
>> called directly, and because it has a "spice_" prefix that (to me)
>> implies it his "closer to the public API" than usbredir_write_callback,
>> when in fact it's further away from the public interface.
>>
>>
>> >
>> >> (Also, to be clear, I don't think this invalidates the patch at all, I'm
>> >> really asking questions to make sure I understand the logic and that the
>> >> way the code is organized is fully intentional).
>> >>
>>
>> --
>> Cheers,
>> Christophe de Dinechin (IRC c3d)
>>


--
Cheers,
Christophe de Dinechin (IRC c3d)
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).

> Use a single state variable, data flows into/from a single parser.
> 
> Signed-off-by: Frediano Ziglio <fziglio@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);

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

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


Uri.

> +        g_assert(SPICE_ALIGNED_CAST(struct usb_redir_header *, data)->type == usb_redir_hello);
> +
> +        const uint32_t flags =
> +            usbredirparser_fl_write_cb_owns_buffer | usbredirparser_fl_usb_host |
> +            usbredirparser_fl_no_hello;
> +
> +        usbredirparser_init(ch->parser,
> +                            PACKAGE_STRING,
> +                            SPICE_ALIGNED_CAST(uint32_t *, data + hello_size),
> +                            (count - hello_size) / sizeof(uint32_t),
> +                            flags);
> +
> +        return 0;
>       }
>       res = spice_usbredir_write_callback(ch->user_data, data, count);
>       return res;
> @@ -707,31 +729,33 @@ int spice_usb_backend_read_guest_data(SpiceUsbBackendChannel *ch, uint8_t *data,
>   
>       ch->read_buf = data;
>       ch->read_buf_size = count;
> -    if (ch->usbredirhost) {
> -        res = usbredirhost_read_guest_data(ch->usbredirhost);
> -        if (!ch->hello_done_parser) {
> -            int res_parser;
> +    if (SPICE_UNLIKELY(ch->state == USB_CHANNEL_STATE_INITIALIZING)) {
> +        if (ch->usbredirhost != NULL) {
> +            res = usbredirhost_read_guest_data(ch->usbredirhost);
> +            if (res != 0) {
> +                return res;
> +            }
> +            ch->state = USB_CHANNEL_STATE_HOST;
> +
>               /* usbredirhost should consume hello response */
>               g_return_val_if_fail(ch->read_buf == NULL, USB_REDIR_ERROR_READ_PARSE);
> +        } else {
> +            ch->state = USB_CHANNEL_STATE_PARSER;
> +        }
>   
> -            ch->read_buf = data;
> -            ch->read_buf_size = count;
> -            ch->hello_done_parser = 1;
> -            if (ch->attached && ch->attached->edev) {
> -                /* case of CD sharing on connect */
> -                ch->usbredirhost = NULL;
> -                ch->parser = ch->hidden_parser;
> -                SPICE_DEBUG("%s: switch %p to parser", __FUNCTION__, ch);
> -            }
> -            res_parser = usbredirparser_do_read(ch->hidden_parser);
> -            if (res >= 0) {
> -                res = res_parser;
> -            }
> +        ch->read_buf = data;
> +        ch->read_buf_size = count;
> +        if (ch->attached && ch->attached->edev) {
> +            /* case of CD sharing on connect */
> +            ch->state = USB_CHANNEL_STATE_PARSER;
> +            SPICE_DEBUG("%s: switch %p to parser", __FUNCTION__, ch);
>           }
> -    } else if (ch->parser) {
> -        res = usbredirparser_do_read(ch->parser);
> +        return usbredirparser_do_read(ch->parser);
> +    }
> +    if (ch->state == USB_CHANNEL_STATE_HOST) {
> +        res = usbredirhost_read_guest_data(ch->usbredirhost);
>       } else {
> -        res = USB_REDIR_ERROR_IO;
> +        res = usbredirparser_do_read(ch->parser);
>       }
>       switch (res)
>       {
> @@ -783,14 +807,12 @@ GError *spice_usb_backend_get_error_details(int error_code, gchar *desc)
>   
>   void spice_usb_backend_return_write_data(SpiceUsbBackendChannel *ch, void *data)
>   {
> -    if (ch->usbredirhost) {
> +    if (ch->state == USB_CHANNEL_STATE_HOST) {
>           SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
>           usbredirhost_free_write_buffer(ch->usbredirhost, data);
> -    } else if (ch->parser) {
> +    } else {
>           SPICE_DEBUG("%s ch %p -> parser", __FUNCTION__, ch);
>           usbredirparser_free_write_buffer(ch->parser, data);
> -    } else {
> -        SPICE_DEBUG("%s ch %p - NOBODY TO CALL", __FUNCTION__, ch);
>       }
>   }
>   
> @@ -1005,10 +1027,10 @@ static void usbredir_device_disconnect_ack(void *priv)
>   {
>       SpiceUsbBackendChannel *ch = priv;
>       SPICE_DEBUG("%s ch %p", __FUNCTION__, ch);
> -    if (ch->parser && ch->wait_disconnect_ack) {
> -        ch->parser = NULL;
> +    if (ch->state == USB_CHANNEL_STATE_PARSER && ch->usbredirhost != NULL &&
> +        ch->wait_disconnect_ack) {
> +        ch->state = USB_CHANNEL_STATE_HOST;
>           SPICE_DEBUG("%s switch to usbredirhost", __FUNCTION__);
> -        ch->usbredirhost = ch->hidden_host;
>       }
>       ch->wait_disconnect_ack = 0;
>   }
> @@ -1027,9 +1049,6 @@ usbredir_hello(void *priv, struct usb_redir_hello_header *hello)
>       SPICE_DEBUG("%s %p %sattached %s", __FUNCTION__, ch,
>           edev ? "" : "not ",  hello ? "" : "(internal)");
>   
> -    if (hello) {
> -        ch->hello_done_parser = 1;
> -    }
>       if (!edev) {
>           return;
>       }
> @@ -1079,53 +1098,24 @@ usbredir_hello(void *priv, struct usb_redir_hello_header *hello)
>       usbredir_write_flush_callback(ch);
>   }
>   
> -static void initialize_parser(SpiceUsbBackendChannel *ch, gboolean do_hello)
> +static void initialize_parser(SpiceUsbBackendChannel *ch)
>   {
>       uint32_t flags, caps[USB_REDIR_CAPS_SIZE] = { 0 };
>   
> -    g_return_if_fail(ch->hidden_parser != NULL);
> -    if (ch->parser_init_done) {
> -        if (ch->parser_with_hello != do_hello) {
> -            g_return_if_reached();
> -        }
> -        return;
> -    }
> +    g_assert(ch->usbredirhost == NULL);
>   
> -    ch->parser_init_done = 1;
> -    ch->parser_with_hello = do_hello;
>       flags = usbredirparser_fl_write_cb_owns_buffer | usbredirparser_fl_usb_host;
>   
> -    if (do_hello) {
> -        ch->hello_sent = 1;
> -        ch->host_caps |= 1 << usb_redir_cap_connect_device_version;
> -        ch->host_caps |= 1 << usb_redir_cap_device_disconnect_ack;
> -        ch->host_caps |= 1 << usb_redir_cap_ep_info_max_packet_size;
> -        ch->host_caps |= 1 << usb_redir_cap_64bits_ids;
> -        ch->host_caps |= 1 << usb_redir_cap_32bits_bulk_length;
> -    } else {
> -        flags |= usbredirparser_fl_no_hello;
> -    }
> -
> -    if (ch->host_caps & (1 << usb_redir_cap_connect_device_version)) {
> -        usbredirparser_caps_set_cap(caps, usb_redir_cap_connect_device_version);
> -    }
> +    usbredirparser_caps_set_cap(caps, usb_redir_cap_connect_device_version);
>       usbredirparser_caps_set_cap(caps, usb_redir_cap_filter);
> -    if (ch->host_caps & (1 << usb_redir_cap_device_disconnect_ack)) {
> -        usbredirparser_caps_set_cap(caps, usb_redir_cap_device_disconnect_ack);
> -    }
> -    if (ch->host_caps & (1 << usb_redir_cap_ep_info_max_packet_size)) {
> -        usbredirparser_caps_set_cap(caps, usb_redir_cap_ep_info_max_packet_size);
> -    }
> -    if (ch->host_caps & (1 << usb_redir_cap_64bits_ids)) {
> -        usbredirparser_caps_set_cap(caps, usb_redir_cap_64bits_ids);
> -    }
> -    if (ch->host_caps & (1 << usb_redir_cap_32bits_bulk_length)) {
> -        usbredirparser_caps_set_cap(caps, usb_redir_cap_32bits_bulk_length);
> -    }
> -    if (ch->host_caps & (1 << usb_redir_cap_bulk_streams)) {
> -        usbredirparser_caps_set_cap(caps, usb_redir_cap_bulk_streams);
> -    }
> -    usbredirparser_init(ch->hidden_parser, PACKAGE_STRING, caps, USB_REDIR_CAPS_SIZE, flags);
> +    usbredirparser_caps_set_cap(caps, usb_redir_cap_device_disconnect_ack);
> +    usbredirparser_caps_set_cap(caps, usb_redir_cap_ep_info_max_packet_size);
> +    usbredirparser_caps_set_cap(caps, usb_redir_cap_64bits_ids);
> +    usbredirparser_caps_set_cap(caps, usb_redir_cap_32bits_bulk_length);
> +    usbredirparser_caps_set_cap(caps, usb_redir_cap_bulk_receiving);
> +    usbredirparser_caps_set_cap(caps, usb_redir_cap_bulk_streams);
> +
> +    usbredirparser_init(ch->parser, PACKAGE_STRING, caps, USB_REDIR_CAPS_SIZE, flags);
>   }
>   
>   /*
> @@ -1180,7 +1170,7 @@ static gboolean attach_edev(SpiceUsbBackendChannel *ch,
>              _("Failed to redirect device %d"), 1);
>           return FALSE;
>       }
> -    if (ch->usbredirhost && !ch->hello_done_parser) {
> +    if (ch->state == USB_CHANNEL_STATE_INITIALIZING) {
>           /*
>               we can't initialize parser until we see hello from usbredir
>               and the parser can't work until it sees the hello response.
> @@ -1190,15 +1180,13 @@ static gboolean attach_edev(SpiceUsbBackendChannel *ch,
>           SPICE_DEBUG("%s waiting until the channel is ready", __FUNCTION__);
>   
>       } else {
> -        initialize_parser(ch, ch->hidden_host == NULL);
> -        ch->usbredirhost = NULL;
> -        ch->parser = ch->hidden_parser;
> +        ch->state = USB_CHANNEL_STATE_PARSER;
>       }
>       ch->wait_disconnect_ack = 0;
>       ch->attached = dev;
>       dev->attached_to = ch;
> -    device_ops(dev->edev)->attach(dev->edev, ch->hidden_parser);
> -    if (ch->hello_done_parser) {
> +    device_ops(dev->edev)->attach(dev->edev, ch->parser);
> +    if (ch->state == USB_CHANNEL_STATE_PARSER) {
>           /* send device info */
>           usbredir_hello(ch, NULL);
>       }
> @@ -1218,9 +1206,15 @@ gboolean spice_usb_backend_channel_attach(SpiceUsbBackendChannel *ch,
>           return attach_edev(ch, dev, error);
>       }
>   
> +    // no physical device enabled
> +    if (ch->usbredirhost == NULL) {
> +        return FALSE;
> +    }
> +
>       libusb_device_handle *handle = NULL;
> -    ch->usbredirhost = ch->hidden_host;
> -    ch->parser = NULL;
> +    if (ch->state != USB_CHANNEL_STATE_INITIALIZING) {
> +        ch->state = USB_CHANNEL_STATE_HOST;
> +    }
>   
>       /*
>          Under Windows we need to avoid updating
> @@ -1261,10 +1255,10 @@ void spice_usb_backend_channel_detach(SpiceUsbBackendChannel *ch)
>           SPICE_DEBUG("%s: nothing to detach", __FUNCTION__);
>           return;
>       }
> -    if (ch->usbredirhost) {
> +    if (ch->state == USB_CHANNEL_STATE_HOST) {
>           /* it will call libusb_close internally */
>           usbredirhost_set_device(ch->usbredirhost, NULL);
> -    } else if (ch->parser) {
> +    } else {
>           if (edev) {
>               device_ops(edev)->detach(edev);
>           }
> @@ -1272,9 +1266,8 @@ void spice_usb_backend_channel_detach(SpiceUsbBackendChannel *ch)
>           usbredir_write_flush_callback(ch);
>           ch->wait_disconnect_ack =
>               usbredirparser_peer_has_cap(ch->parser, usb_redir_cap_device_disconnect_ack);
> -        if (!ch->wait_disconnect_ack) {
> -            ch->usbredirhost = ch->hidden_host;
> -            ch->parser = NULL;
> +        if (!ch->wait_disconnect_ack && ch->usbredirhost != NULL) {
> +            ch->state = USB_CHANNEL_STATE_HOST;
>           }
>       }
>       SPICE_DEBUG("%s ch %p, detach done", __FUNCTION__, ch);
> @@ -1311,16 +1304,22 @@ SpiceUsbBackendChannel *spice_usb_backend_channel_new(SpiceUsbBackend *be,
>                                           usbredirparser_warning,
>                                      usbredirhost_fl_write_cb_owns_buffer);
>           g_warn_if_fail(ch->usbredirhost != NULL);
> +        if (ch->usbredirhost != NULL) {
> +            usbredirhost_set_buffered_output_size_cb(ch->usbredirhost,
> +                                                     usbredir_buffered_output_size_callback);
> +            // force flush of HELLO packet and creation of parser
> +            usbredirhost_write_guest_data(ch->usbredirhost);
> +        }
> +    } else {
> +        // no physical device support, only emulated, create the
> +        // parser
> +        ch->parser = create_parser(ch);
> +        if (ch->parser != NULL) {
> +            initialize_parser(ch);
> +        }
>       }
>   
> -    if (ch->usbredirhost) {
> -        ch->hidden_parser = create_parser(ch);
> -        ch->hidden_host = ch->usbredirhost;
> -        usbredirhost_set_buffered_output_size_cb(ch->usbredirhost,
> -                                                 usbredir_buffered_output_size_callback);
> -    }
> -
> -    if (!ch->hidden_parser) {
> +    if (!ch->parser) {
>           spice_usb_backend_channel_delete(ch);
>           ch = NULL;
>       }
> @@ -1332,12 +1331,9 @@ SpiceUsbBackendChannel *spice_usb_backend_channel_new(SpiceUsbBackend *be,
>   void spice_usb_backend_channel_flush_writes(SpiceUsbBackendChannel *ch)
>   {
>       SPICE_DEBUG("%s %p is up", __FUNCTION__, ch);
> -    if (ch->usbredirhost) {
> +    if (ch->state != USB_CHANNEL_STATE_PARSER && ch->usbredirhost != NULL) {
>           usbredirhost_write_guest_data(ch->usbredirhost);
> -        initialize_parser(ch, FALSE);
>       } else {
> -        initialize_parser(ch, TRUE);
> -        ch->parser = ch->hidden_parser;
>           usbredirparser_do_write(ch->parser);
>       }
>   }
> @@ -1348,11 +1344,11 @@ void spice_usb_backend_channel_delete(SpiceUsbBackendChannel *ch)
>       if (!ch) {
>           return;
>       }
> -    if (ch->hidden_host) {
> -        usbredirhost_close(ch->hidden_host);
> +    if (ch->usbredirhost) {
> +        usbredirhost_close(ch->usbredirhost);
>       }
> -    if (ch->hidden_parser) {
> -        usbredirparser_destroy(ch->hidden_parser);
> +    if (ch->parser) {
> +        usbredirparser_destroy(ch->parser);
>       }
>   
>       if (ch->rules) {
> @@ -1372,7 +1368,7 @@ spice_usb_backend_channel_get_guest_filter(SpiceUsbBackendChannel *ch,
>       int i;
>       *r = NULL;
>       *count = 0;
> -    if (ch->usbredirhost) {
> +    if (ch->usbredirhost != NULL) {
>           usbredirhost_get_guest_filter(ch->usbredirhost, r, count);
>       }
>       if (*r == NULL) {
>
> 
> 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@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
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@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.
> 
> 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@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.
>