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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
> ---
>  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)
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]