[Spice-devel] [PATCH spice-gtk v4 24/29] usb-backend: Rewrite USB emulation support
Frediano Ziglio
fziglio at redhat.com
Wed Aug 28 10:16:06 UTC 2019
>
> 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 at redhat.com>
> > ---
> > src/usb-backend.c | 236 +++++++++++++++++++++++-----------------------
> > 1 file changed, 116 insertions(+), 120 deletions(-)
> >
> > diff --git a/src/usb-backend.c b/src/usb-backend.c
> > index 36a73a89..d614e693 100644
> > --- a/src/usb-backend.c
> > +++ b/src/usb-backend.c
> > @@ -78,21 +78,21 @@ struct _SpiceUsbBackend
> > uint32_t own_devices_mask;
> > };
> >
> > +typedef enum {
> > + USB_CHANNEL_STATE_INITIALIZING,
>
> (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
More information about the Spice-devel
mailing list