[spice-gtk,v3,5/9] usb-redir: extend USB backend to support emulated devices

Submitted by Yuri Benditovich on Aug. 12, 2019, 9:57 a.m.

Details

Message ID 20190812095729.32692-6-yuri.benditovich@daynix.com
State Superseded
Headers show
Series "added feature of sharing CD image" ( rev: 5 ) in Spice

Not browsing as part of any series.

Commit Message

Yuri Benditovich Aug. 12, 2019, 9:57 a.m.
Redirection of emulated devices requires special approach,
as usbredirhost can't be used for that (it works only with
libusb devices). For emulated devices we create instance of
usbredirparser that implements USB redirection protocol.
In order to work with the same set of protocol capabilities
that usbredirhost sets up with remote side, the parser shall:
- not send its own 'hello' to the server
- initialize the same capabilities that usbredirhost
- receive the same 'hello' response
For that we analyze 'hello' sent by USB redir parser and
extract set of capabilities from it and upon receive of
'hello' response we provide it also to the parser.
When libusb device is redirected via a channel, instance of
usbredirhost serves it.
When emulated device is redirected via a channel, instance
of usbredirparser does the job.

Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
---
 src/usb-backend.c | 616 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 580 insertions(+), 36 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/usb-backend.c b/src/usb-backend.c
index aa11c79..9aee6c7 100644
--- a/src/usb-backend.c
+++ b/src/usb-backend.c
@@ -55,6 +55,7 @@  struct _SpiceUsbBackendDevice
     gint ref_count;
     SpiceUsbBackendChannel *attached_to;
     UsbDeviceInformation device_info;
+    gboolean edev_configured;
 };
 
 struct _SpiceUsbBackend
@@ -80,10 +81,20 @@  struct _SpiceUsbBackend
 struct _SpiceUsbBackendChannel
 {
     struct usbredirhost *usbredirhost;
+    struct usbredirhost *hidden_host;
+    struct usbredirparser *parser;
+    struct usbredirparser *hidden_parser;
     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_disc_ack     : 1;
     SpiceUsbBackendDevice *attached;
     SpiceUsbredirChannel  *user_data;
     SpiceUsbBackend *backend;
@@ -355,7 +366,11 @@  gboolean spice_usb_backend_device_isoch(SpiceUsbBackendDevice *dev)
     gint i, j, k;
     int rc;
 
-    g_return_val_if_fail(libdev != NULL, 0);
+    g_return_val_if_fail(libdev != NULL || dev->edev != NULL, 0);
+    if (dev->edev) {
+        /* currently we do not emulate isoch devices */
+        return FALSE;
+    }
 
     rc = libusb_get_active_config_descriptor(libdev, &conf_desc);
     if (rc) {
@@ -390,13 +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->usbredirhost) {
-        /* just to be on the safe side */
-        return;
-    }
     if (is_channel_ready(ch->user_data)) {
-        SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
-        usbredirhost_write_guest_data(ch->usbredirhost);
+        if (ch->usbredirhost) {
+            SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
+            usbredirhost_write_guest_data(ch->usbredirhost);
+        } else if (ch->parser) {
+            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);
     }
@@ -551,13 +569,52 @@  void spice_usb_backend_device_unref(SpiceUsbBackendDevice *dev)
     }
 }
 
-int spice_usb_backend_device_check_filter(
-    SpiceUsbBackendDevice *dev,
-    const struct usbredirfilter_rule *rules,
-    int count)
+static int check_edev_device_filter(SpiceUsbBackendDevice *dev,
+                                    const struct usbredirfilter_rule *rules,
+                                    int count)
 {
-    return usbredirhost_check_device_filter(
-        rules, count, dev->libusb_device, 0);
+    SpiceUsbEmulatedDevice *edev = dev->edev;
+    uint8_t cls[32], subcls[32], proto[32], *cfg, ifnum = 0;
+    uint16_t size, offset = 0;
+    if (!edev) {
+        return 1;
+    }
+    if (!device_ops(edev)->get_descriptor(edev, LIBUSB_DT_CONFIG, 0, (void **)&cfg, &size)) {
+        return 1;
+    }
+    while ((offset + 1) < size) {
+        uint8_t len  = cfg[offset];
+        uint8_t type = cfg[offset + 1];
+        if ((offset + len) > size) {
+            break;
+        }
+        if (type == LIBUSB_DT_INTERFACE) {
+            cls[ifnum] = cfg[offset + 5];
+            subcls[ifnum] = cfg[offset + 6];
+            proto[ifnum] = cfg[offset + 7];
+            ifnum++;
+        }
+        offset += len;
+    }
+
+    return usbredirfilter_check(rules, count,
+                                dev->device_info.class,
+                                dev->device_info.subclass,
+                                dev->device_info.protocol,
+                                cls, subcls, proto, ifnum,
+                                dev->device_info.vid,
+                                dev->device_info.pid,
+                                dev->device_info.bcdUSB, 0);
+}
+
+int spice_usb_backend_device_check_filter(SpiceUsbBackendDevice *dev,
+                                          const struct usbredirfilter_rule *rules, int count)
+{
+    if (dev->libusb_device) {
+        return usbredirhost_check_device_filter(rules, count, dev->libusb_device, 0);
+    } else {
+        return check_edev_device_filter(dev, rules, count);
+    }
 }
 
 static int usbredir_read_callback(void *user_data, uint8_t *data, int count)
@@ -621,6 +678,17 @@  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);
+        }
+    }
     res = spice_usbredir_write_callback(ch->user_data, data, count);
     return res;
 }
@@ -641,6 +709,27 @@  int spice_usb_backend_read_guest_data(SpiceUsbBackendChannel *ch, uint8_t *data,
     ch->read_buf_size = count;
     if (ch->usbredirhost) {
         res = usbredirhost_read_guest_data(ch->usbredirhost);
+        if (!ch->hello_done_parser) {
+            int res_parser;
+            /* usbredirhost should consume hello response */
+            g_return_val_if_fail(ch->read_buf == NULL, USB_REDIR_ERROR_READ_PARSE);
+
+            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;
+            }
+        }
+    } else if (ch->parser) {
+        res = usbredirparser_do_read(ch->parser);
     } else {
         res = USB_REDIR_ERROR_IO;
     }
@@ -661,6 +750,11 @@  int spice_usb_backend_read_guest_data(SpiceUsbBackendChannel *ch, uint8_t *data,
     }
     SPICE_DEBUG("%s ch %p, %d bytes, res %d", __FUNCTION__, ch, count, res);
 
+    if (ch->rejected) {
+        ch->rejected = 0;
+        res = USB_REDIR_ERROR_DEV_REJECTED;
+    }
+
     return res;
 }
 
@@ -690,13 +784,426 @@  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) {
-        SPICE_DEBUG("%s ch %p", __FUNCTION__, ch);
+        SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
         usbredirhost_free_write_buffer(ch->usbredirhost, data);
+    } else if (ch->parser) {
+        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);
     }
 }
 
+static void usbredir_control_packet(void *priv,
+    uint64_t id, struct usb_redir_control_packet_header *h,
+    uint8_t *data, int data_len)
+{
+    SpiceUsbBackendChannel *ch = priv;
+    SpiceUsbBackendDevice *d = ch->attached;
+    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
+    struct usb_redir_control_packet_header response = *h;
+    uint8_t reqtype = h->requesttype & 0x7f;
+    gboolean done = FALSE;
+    void *out_buffer = NULL;
+
+    response.status = usb_redir_stall;
+    SPICE_DEBUG("%s %p: TRVIL %02X %02X %04X %04X %04X",
+                __FUNCTION__,
+                ch, h->requesttype, h->request,
+                h->value, h->index, h->length);
+
+    if (!edev) {
+        SPICE_DEBUG("%s: device not attached", __FUNCTION__);
+        response.status = usb_redir_ioerror;
+        done = TRUE;
+    } else if (reqtype == (LIBUSB_REQUEST_TYPE_STANDARD | LIBUSB_RECIPIENT_DEVICE) &&
+               h->request == LIBUSB_REQUEST_GET_DESCRIPTOR) {
+        uint16_t size;
+        done = device_ops(edev)->get_descriptor(edev, h->value >> 8, h->value & 0xff,
+                                                &out_buffer, &size);
+        response.length = size;
+        if (done) {
+            response.status = 0;
+        }
+        done = TRUE;
+    }
+
+    if (!done) {
+        device_ops(edev)->control_request(edev, data, data_len, &response, &out_buffer);
+        done = TRUE;
+    }
+
+    if (response.status) {
+        response.length = 0;
+    } else if (response.length > h->length) {
+        response.length = h->length;
+    }
+
+    SPICE_DEBUG("%s responding with payload of %02X, status %X",
+                __FUNCTION__, response.length, response.status);
+    usbredirparser_send_control_packet(ch->parser, id, &response,
+                                       response.length ? out_buffer : NULL,
+                                       response.length);
+
+    usbredir_write_flush_callback(ch);
+    usbredirparser_free_packet_data(ch->parser, data);
+}
+
+static void usbredir_bulk_packet(void *priv,
+    uint64_t id, struct usb_redir_bulk_packet_header *h,
+    uint8_t *data, int data_len)
+{
+    SpiceUsbBackendChannel *ch = priv;
+    SpiceUsbBackendDevice *d = ch->attached;
+    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
+    struct usb_redir_bulk_packet_header hout = *h;
+    uint32_t len = (h->length_high << 16) | h->length;
+    SPICE_DEBUG("%s %p: ep %X, len %u, id %" G_GUINT64_FORMAT, __FUNCTION__,
+                ch, h->endpoint, len, id);
+
+    if (!edev) {
+        SPICE_DEBUG("%s: device not attached", __FUNCTION__);
+        hout.status = usb_redir_ioerror;
+        hout.length = hout.length_high = 0;
+        SPICE_DEBUG("%s: responding with ZLP status %d", __FUNCTION__, hout.status);
+    } else if (h->endpoint & LIBUSB_ENDPOINT_IN) {
+        if (device_ops(edev)->bulk_in_request(edev, id, &hout)) {
+            usbredirparser_free_packet_data(ch->parser, data);
+            /* completion is asynchronous */
+            return;
+        }
+    } else {
+        hout.status = usb_redir_stall;
+        device_ops(edev)->bulk_out_request(edev, h->endpoint, data, data_len, &hout.status);
+        SPICE_DEBUG("%s: responding status %d", __FUNCTION__, hout.status);
+    }
+
+    usbredirparser_send_bulk_packet(ch->parser, id, &hout, NULL, 0);
+    usbredirparser_free_packet_data(ch->parser, data);
+    usbredir_write_flush_callback(ch);
+}
+
+static void usbredir_device_reset(void *priv)
+{
+    SpiceUsbBackendChannel *ch = priv;
+    SpiceUsbBackendDevice *d = ch->attached;
+    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
+    SPICE_DEBUG("%s ch %p", __FUNCTION__, ch);
+    if (edev) {
+        device_ops(edev)->reset(edev);
+    }
+}
+
+static void usbredir_interface_info(void *priv,
+    struct usb_redir_interface_info_header *interface_info)
+{
+    SpiceUsbBackendChannel *ch = priv;
+    SPICE_DEBUG("%s not implemented %p", __FUNCTION__, ch);
+}
+
+static void usbredir_interface_ep_info(void *priv,
+    struct usb_redir_ep_info_header *ep_info)
+{
+    SpiceUsbBackendChannel *ch = priv;
+    SPICE_DEBUG("%s not implemented %p", __FUNCTION__, ch);
+}
+
+static void usbredir_set_configuration(void *priv,
+    uint64_t id, struct usb_redir_set_configuration_header *set_configuration)
+{
+    SpiceUsbBackendChannel *ch = priv;
+    struct usb_redir_configuration_status_header h;
+    h.status = 0;
+    h.configuration = set_configuration->configuration;
+    SPICE_DEBUG("%s ch %p, cfg %d", __FUNCTION__, ch, h.configuration);
+    if (ch->attached) {
+        ch->attached->edev_configured = h.configuration != 0;
+    }
+    usbredirparser_send_configuration_status(ch->parser, id, &h);
+    usbredir_write_flush_callback(ch);
+}
+
+static void usbredir_get_configuration(void *priv, uint64_t id)
+{
+    SpiceUsbBackendChannel *ch = priv;
+    struct usb_redir_configuration_status_header h;
+    h.status = 0;
+    h.configuration = ch->attached && ch->attached->edev_configured;
+    SPICE_DEBUG("%s ch %p, cfg %d", __FUNCTION__, ch, h.configuration);
+    usbredirparser_send_configuration_status(ch->parser, id, &h);
+    usbredir_write_flush_callback(ch);
+}
+
+static void usbredir_set_alt_setting(void *priv,
+    uint64_t id, struct usb_redir_set_alt_setting_header *s)
+{
+    SpiceUsbBackendChannel *ch = priv;
+    struct usb_redir_alt_setting_status_header sh;
+    sh.status = (!s->interface && !s->alt) ? 0 : usb_redir_stall;
+    sh.interface = s->interface;
+    sh.alt = s->alt;
+    SPICE_DEBUG("%s ch %p, %d:%d", __FUNCTION__, ch, s->interface, s->alt);
+    usbredirparser_send_alt_setting_status(ch->parser, id, &sh);
+    usbredir_write_flush_callback(ch);
+}
+
+static void usbredir_get_alt_setting(void *priv,
+    uint64_t id, struct usb_redir_get_alt_setting_header *s)
+{
+    SpiceUsbBackendChannel *ch = priv;
+    struct usb_redir_alt_setting_status_header sh;
+    sh.status = (s->interface == 0) ? 0 : usb_redir_stall;
+    sh.interface = s->interface;
+    sh.alt = 0;
+    SPICE_DEBUG("%s ch %p, if %d", __FUNCTION__, ch, s->interface);
+    usbredirparser_send_alt_setting_status(ch->parser, id, &sh);
+    usbredir_write_flush_callback(ch);
+}
+
+static void usbredir_cancel_data(void *priv, uint64_t id)
+{
+    SpiceUsbBackendChannel *ch = priv;
+    SpiceUsbBackendDevice *d = ch->attached;
+    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
+    if (!edev) {
+        SPICE_DEBUG("%s: device not attached", __FUNCTION__);
+        return;
+    }
+    device_ops(edev)->cancel_request(edev, id);
+}
+
+static void usbredir_filter_reject(void *priv)
+{
+    SpiceUsbBackendChannel *ch = priv;
+    SPICE_DEBUG("%s %p", __FUNCTION__, ch);
+    ch->rejected = 1;
+}
+
+/* Note that the ownership of the rules array is passed on to the callback. */
+static void usbredir_filter_filter(void *priv,
+    struct usbredirfilter_rule *r, int count)
+{
+    SpiceUsbBackendChannel *ch = priv;
+    SPICE_DEBUG("%s ch %p %d filters", __FUNCTION__, ch, count);
+
+    free(ch->rules);
+
+    ch->rules = r;
+    ch->rules_count = count;
+    if (count) {
+        int i;
+        for (i = 0; i < count; i++) {
+            SPICE_DEBUG("%s class %d, %X:%X",
+                r[i].allow ? "allowed" : "denied", r[i].device_class,
+                (uint32_t)r[i].vendor_id, (uint32_t)r[i].product_id);
+        }
+    }
+}
+
+static void usbredir_device_disconnect_ack(void *priv)
+{
+    SpiceUsbBackendChannel *ch = priv;
+    SPICE_DEBUG("%s ch %p", __FUNCTION__, ch);
+    if (ch->parser && ch->wait_disc_ack) {
+        ch->parser = NULL;
+        SPICE_DEBUG("%s switch to usbredirhost", __FUNCTION__);
+        ch->usbredirhost = ch->hidden_host;
+    }
+    ch->wait_disc_ack = 0;
+}
+
+static void usbredir_hello(void *priv,
+    struct usb_redir_hello_header *hello)
+{
+    SpiceUsbBackendChannel *ch = priv;
+    SpiceUsbBackendDevice *d = ch->attached;
+    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
+    struct usb_redir_device_connect_header device_connect;
+    struct usb_redir_ep_info_header ep_info = { 0 };
+    struct usb_redir_interface_info_header interface_info = { 0 };
+    uint8_t *cfg;
+    uint16_t size, offset = 0;
+    SPICE_DEBUG("%s %p %sattached %s", __FUNCTION__, ch,
+        edev ? "" : "not ",  hello ? "" : "(internal)");
+
+    if (hello) {
+        ch->hello_done_parser = 1;
+    }
+    if (!edev) {
+        return;
+    }
+    if (!device_ops(edev)->get_descriptor(edev, LIBUSB_DT_CONFIG, 0, (void **)&cfg, &size)) {
+        return;
+    }
+    while ((offset + 1) < size) {
+        uint8_t len  = cfg[offset];
+        uint8_t type = cfg[offset + 1];
+        if ((offset + len) > size) {
+            break;
+        }
+        if (type == LIBUSB_DT_INTERFACE) {
+            uint32_t i = interface_info.interface_count;
+            uint8_t class, subclass, protocol;
+            class = cfg[offset + 5];
+            subclass = cfg[offset + 6];
+            protocol = cfg[offset + 7];
+            interface_info.interface_class[i] = class;
+            interface_info.interface_subclass[i] = subclass;
+            interface_info.interface_protocol[i] = protocol;
+            interface_info.interface_count++;
+            SPICE_DEBUG("%s IF%d: %d/%d/%d", __FUNCTION__, i, class, subclass, protocol);
+        } else if (type == LIBUSB_DT_ENDPOINT) {
+            uint8_t address = cfg[offset + 2];
+            uint16_t max_packet_size = 0x100 * cfg[offset + 5] + cfg[offset + 4];
+            uint8_t index = address & 0xf;
+            if (address & 0x80) index += 0x10;
+            ep_info.type[index] = cfg[offset + 3] & 0x3;
+            ep_info.max_packet_size[index] = max_packet_size;
+            SPICE_DEBUG("%s EP[%02X]: %d/%d", __FUNCTION__, index, ep_info.type[index], max_packet_size);
+        }
+        offset += len;
+    }
+
+    usbredirparser_send_interface_info(ch->parser, &interface_info);
+    usbredirparser_send_ep_info(ch->parser, &ep_info);
+
+    device_connect.device_class = 0; //d->device_info.class;
+    device_connect.device_subclass = 0; //d->device_info.subclass;
+    device_connect.device_protocol = 0; //d->device_info.protocol;;
+    device_connect.vendor_id = d->device_info.vid;
+    device_connect.product_id = d->device_info.pid;
+    device_connect.device_version_bcd = d->device_info.bcdUSB;
+    device_connect.speed = usb_redir_speed_high;
+    usbredirparser_send_device_connect(ch->parser, &device_connect);
+    usbredir_write_flush_callback(ch);
+}
+
+static void initialize_parser(SpiceUsbBackendChannel *ch, gboolean do_hello)
+{
+    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;
+    }
+
+    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_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);
+}
+
+/*
+    We can initialize the usbredirparser with HELLO enabled only in case
+    the libusb is not active and the usbredirhost does not function.
+    Then the parser sends session HELLO and receives server's response.
+    Otherwise (usbredirparser initialized with HELLO disabled):
+    - the usbredirhost sends session HELLO
+    - we look into it to know set of capabilities we shall initialize
+      the parser with
+    - when we receive server's response to HELLO we provide it also to
+      parser to let it synchronize with server's capabilities
+*/
+static struct usbredirparser *create_parser(SpiceUsbBackendChannel *ch)
+{
+    struct usbredirparser *parser = usbredirparser_create();
+
+    g_return_val_if_fail(parser != NULL, NULL);
+
+    parser->priv = ch;
+    parser->log_func = usbredir_log;
+    parser->read_func = usbredir_read_callback;
+    parser->write_func = usbredir_write_callback;
+    parser->reset_func = usbredir_device_reset;
+    parser->set_configuration_func = usbredir_set_configuration;
+    parser->get_configuration_func = usbredir_get_configuration;
+    parser->set_alt_setting_func = usbredir_set_alt_setting;
+    parser->get_alt_setting_func = usbredir_get_alt_setting;
+    parser->cancel_data_packet_func = usbredir_cancel_data;
+    parser->control_packet_func = usbredir_control_packet;
+    parser->bulk_packet_func = usbredir_bulk_packet;
+    parser->alloc_lock_func = usbredir_alloc_lock;
+    parser->lock_func = usbredir_lock_lock;
+    parser->unlock_func = usbredir_unlock_lock;
+    parser->free_lock_func = usbredir_free_lock;
+    parser->hello_func = usbredir_hello;
+    parser->filter_reject_func = usbredir_filter_reject;
+    parser->device_disconnect_ack_func = usbredir_device_disconnect_ack;
+    parser->interface_info_func = usbredir_interface_info;
+    parser->ep_info_func = usbredir_interface_ep_info;
+    parser->filter_filter_func = usbredir_filter_filter;
+
+    return parser;
+}
+
+static gboolean attach_edev(SpiceUsbBackendChannel *ch,
+                            SpiceUsbBackendDevice *dev,
+                            GError **error)
+{
+    if (!dev->edev) {
+        g_set_error(error, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
+           _("Failed to redirect device %d"), 1);
+        return FALSE;
+    }
+    if (ch->usbredirhost && !ch->hello_done_parser) {
+        /*
+            we can't initialize parser until we see hello from usbredir
+            and the parser can't work until it sees the hello response.
+            this is case of autoconnect when emulated device is attached
+            before the channel is up
+        */
+        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->wait_disc_ack = 0;
+    ch->attached = dev;
+    dev->attached_to = ch;
+    device_ops(dev->edev)->attach(dev->edev, ch->hidden_parser);
+    if (ch->hello_done_parser) {
+        /* send device info */
+        usbredir_hello(ch, NULL);
+    }
+    return TRUE;
+}
+
 gboolean spice_usb_backend_channel_attach(SpiceUsbBackendChannel *ch,
                                           SpiceUsbBackendDevice *dev,
                                           GError **error)
@@ -706,7 +1213,13 @@  gboolean spice_usb_backend_channel_attach(SpiceUsbBackendChannel *ch,
 
     g_return_val_if_fail(dev != NULL, FALSE);
 
+    if (!dev->libusb_device) {
+        return attach_edev(ch, dev, error);
+    }
+
     libusb_device_handle *handle = NULL;
+    ch->usbredirhost = ch->hidden_host;
+    ch->parser = NULL;
 
     /*
        Under Windows we need to avoid updating
@@ -740,18 +1253,33 @@  gboolean spice_usb_backend_channel_attach(SpiceUsbBackendChannel *ch,
 
 void spice_usb_backend_channel_detach(SpiceUsbBackendChannel *ch)
 {
+    SpiceUsbBackendDevice *d = ch->attached;
+    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
     SPICE_DEBUG("%s >> ch %p, was attached %p", __FUNCTION__, ch, ch->attached);
-    if (!ch->attached) {
+    if (!d) {
         SPICE_DEBUG("%s: nothing to detach", __FUNCTION__);
         return;
     }
     if (ch->usbredirhost) {
         /* it will call libusb_close internally */
         usbredirhost_set_device(ch->usbredirhost, NULL);
+    } else if (ch->parser) {
+        if (edev) {
+            device_ops(edev)->detach(edev);
+        }
+        usbredirparser_send_device_disconnect(ch->parser);
+        usbredir_write_flush_callback(ch);
+        ch->wait_disc_ack = usbredirparser_peer_has_cap(ch->parser,
+                                                        usb_redir_cap_device_disconnect_ack);
+        if (!ch->wait_disc_ack) {
+            ch->usbredirhost = ch->hidden_host;
+            ch->parser = NULL;
+        }
     }
     SPICE_DEBUG("%s ch %p, detach done", __FUNCTION__, ch);
     ch->attached->attached_to = NULL;
     ch->attached = NULL;
+    ch->rejected = 0;
 }
 
 SpiceUsbBackendChannel *spice_usb_backend_channel_new(SpiceUsbBackend *be,
@@ -766,26 +1294,33 @@  SpiceUsbBackendChannel *spice_usb_backend_channel_new(SpiceUsbBackend *be,
     ch->user_data = SPICE_USBREDIR_CHANNEL(user_data);
     if (be->libusb_context) {
         ch->backend = be;
-        ch->usbredirhost = usbredirhost_open_full(
-            be->libusb_context,
-            NULL,
-            usbredir_log,
-            usbredir_read_callback,
-            usbredir_write_callback,
-            usbredir_write_flush_callback,
-            usbredir_alloc_lock,
-            usbredir_lock_lock,
-            usbredir_unlock_lock,
-            usbredir_free_lock,
-            ch, PACKAGE_STRING,
-            spice_util_get_debug() ? usbredirparser_debug : usbredirparser_warning,
-            usbredirhost_fl_write_cb_owns_buffer);
+        ch->usbredirhost =
+            usbredirhost_open_full(be->libusb_context,
+                                   NULL,
+                                   usbredir_log,
+                                   usbredir_read_callback,
+                                   usbredir_write_callback,
+                                   usbredir_write_flush_callback,
+                                   usbredir_alloc_lock,
+                                   usbredir_lock_lock,
+                                   usbredir_unlock_lock,
+                                   usbredir_free_lock,
+                                   ch, PACKAGE_STRING,
+                                   spice_util_get_debug() ? usbredirparser_debug :
+                                        usbredirparser_warning,
+                                   usbredirhost_fl_write_cb_owns_buffer);
         g_warn_if_fail(ch->usbredirhost != NULL);
     }
+
     if (ch->usbredirhost) {
-        usbredirhost_set_buffered_output_size_cb(ch->usbredirhost, usbredir_buffered_output_size_callback);
-    } else {
-        g_free(ch);
+        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) {
+        spice_usb_backend_channel_delete(ch);
         ch = NULL;
     }
 
@@ -795,9 +1330,14 @@  SpiceUsbBackendChannel *spice_usb_backend_channel_new(SpiceUsbBackend *be,
 
 void spice_usb_backend_channel_flush_writes(SpiceUsbBackendChannel *ch)
 {
-    SPICE_DEBUG("%s %p, host %p", __FUNCTION__, ch, ch->usbredirhost);
+    SPICE_DEBUG("%s %p is up", __FUNCTION__, ch);
     if (ch->usbredirhost) {
         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);
     }
 }
 
@@ -807,12 +1347,16 @@  void spice_usb_backend_channel_delete(SpiceUsbBackendChannel *ch)
     if (!ch) {
         return;
     }
-    if (ch->usbredirhost) {
-        usbredirhost_close(ch->usbredirhost);
+    if (ch->hidden_host) {
+        usbredirhost_close(ch->hidden_host);
+    }
+    if (ch->hidden_parser) {
+        usbredirparser_destroy(ch->hidden_parser);
     }
 
     if (ch->rules) {
-        g_free(ch->rules);
+        /* rules were allocated by usbredirparser */
+        free(ch->rules);
     }
 
     g_free(ch);

Comments

> 
> Redirection of emulated devices requires special approach,
> as usbredirhost can't be used for that (it works only with
> libusb devices). For emulated devices we create instance of
> usbredirparser that implements USB redirection protocol.

Make sense. Actually usbredirhost internally uses a parser to
parse protocol messages and redirect to the physical device.
But the parse is internal and is used by usbredirhost so
would be hard to reuse.

> In order to work with the same set of protocol capabilities
> that usbredirhost sets up with remote side, the parser shall:
> - not send its own 'hello' to the server
> - initialize the same capabilities that usbredirhost
> - receive the same 'hello' response
> For that we analyze 'hello' sent by USB redir parser and
> extract set of capabilities from it and upon receive of
> 'hello' response we provide it also to the parser.

This make sense but some part of the code does not exactly that.
In some path the code send the 'hello' is sent by the parser.
It would be more clear to implement just what you described
above, why the implementation is a bit different.

> When libusb device is redirected via a channel, instance of
> usbredirhost serves it.
> When emulated device is redirected via a channel, instance
> of usbredirparser does the job.
> 
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> ---
>  src/usb-backend.c | 616 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 580 insertions(+), 36 deletions(-)
> 
> diff --git a/src/usb-backend.c b/src/usb-backend.c
> index aa11c79..9aee6c7 100644
> --- a/src/usb-backend.c
> +++ b/src/usb-backend.c
> @@ -55,6 +55,7 @@ struct _SpiceUsbBackendDevice
>      gint ref_count;
>      SpiceUsbBackendChannel *attached_to;
>      UsbDeviceInformation device_info;
> +    gboolean edev_configured;

This variable is never reset, I would expect that at least
when device is detached (this should be equivalent to physical
removal).
Why not moving to SpiceUsbBackendChannel?

>  };
>  
>  struct _SpiceUsbBackend
> @@ -80,10 +81,20 @@ struct _SpiceUsbBackend
>  struct _SpiceUsbBackendChannel
>  {
>      struct usbredirhost *usbredirhost;
> +    struct usbredirhost *hidden_host;
> +    struct usbredirparser *parser;
> +    struct usbredirparser *hidden_parser;

I don't understand all these variable.
Won't be easier to have parser initialized for emulated
devices and being NULL (so freed) when emulated device
is detached?

>      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_disc_ack     : 1;

The "disc" naming is confusing, also taking into account the
"Compact Disc" addition. Reading the code this "disc" is a shorten
for "disconnect", maybe would be better to just use "wait_disconnect_ack"?

>      SpiceUsbBackendDevice *attached;
>      SpiceUsbredirChannel  *user_data;

OT: this definitively should be renamed!

>      SpiceUsbBackend *backend;
> @@ -355,7 +366,11 @@ gboolean
> spice_usb_backend_device_isoch(SpiceUsbBackendDevice *dev)
>      gint i, j, k;
>      int rc;
>  
> -    g_return_val_if_fail(libdev != NULL, 0);
> +    g_return_val_if_fail(libdev != NULL || dev->edev != NULL, 0);
> +    if (dev->edev) {
> +        /* currently we do not emulate isoch devices */
> +        return FALSE;
> +    }
>  
>      rc = libusb_get_active_config_descriptor(libdev, &conf_desc);
>      if (rc) {
> @@ -390,13 +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->usbredirhost) {
> -        /* just to be on the safe side */
> -        return;
> -    }

These check should be in a preparatory patch. Nothing wrong with this patch,
but the code (master) is confusing. usbredirhost is initialized as soon as
possible, can be NULL only before usbredirhost_open_full returns.
Actually can be NULL in this call for this reason (this callback is called
inside usbredirhost_open_full).

>      if (is_channel_ready(ch->user_data)) {
> -        SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
> -        usbredirhost_write_guest_data(ch->usbredirhost);
> +        if (ch->usbredirhost) {
> +            SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
> +            usbredirhost_write_guest_data(ch->usbredirhost);
> +        } else if (ch->parser) {
> +            SPICE_DEBUG("%s ch %p -> parser", __FUNCTION__, ch);
> +            usbredirparser_do_write(ch->parser);
> +        } else {
> +            SPICE_DEBUG("%s ch %p (NOT ACTIVE)", __FUNCTION__, ch);

This third case should never be possible. This is one aspect that master
code is confusing about.

> +        }
>      } else {
>          SPICE_DEBUG("%s ch %p (not ready)", __FUNCTION__, ch);
>      }
> @@ -551,13 +569,52 @@ void
> spice_usb_backend_device_unref(SpiceUsbBackendDevice *dev)
>      }
>  }
>  
> -int spice_usb_backend_device_check_filter(
> -    SpiceUsbBackendDevice *dev,
> -    const struct usbredirfilter_rule *rules,
> -    int count)
> +static int check_edev_device_filter(SpiceUsbBackendDevice *dev,
> +                                    const struct usbredirfilter_rule *rules,
> +                                    int count)
>  {
> -    return usbredirhost_check_device_filter(
> -        rules, count, dev->libusb_device, 0);
> +    SpiceUsbEmulatedDevice *edev = dev->edev;
> +    uint8_t cls[32], subcls[32], proto[32], *cfg, ifnum = 0;
> +    uint16_t size, offset = 0;
> +    if (!edev) {
> +        return 1;
> +    }
> +    if (!device_ops(edev)->get_descriptor(edev, LIBUSB_DT_CONFIG, 0, (void
> **)&cfg, &size)) {
> +        return 1;
> +    }
> +    while ((offset + 1) < size) {
> +        uint8_t len  = cfg[offset];
> +        uint8_t type = cfg[offset + 1];
> +        if ((offset + len) > size) {
> +            break;
> +        }
> +        if (type == LIBUSB_DT_INTERFACE) {
> +            cls[ifnum] = cfg[offset + 5];
> +            subcls[ifnum] = cfg[offset + 6];
> +            proto[ifnum] = cfg[offset + 7];
> +            ifnum++;
> +        }
> +        offset += len;
> +    }
> +
> +    return usbredirfilter_check(rules, count,
> +                                dev->device_info.class,
> +                                dev->device_info.subclass,
> +                                dev->device_info.protocol,
> +                                cls, subcls, proto, ifnum,
> +                                dev->device_info.vid,
> +                                dev->device_info.pid,
> +                                dev->device_info.bcdUSB, 0);
> +}
> +
> +int spice_usb_backend_device_check_filter(SpiceUsbBackendDevice *dev,
> +                                          const struct usbredirfilter_rule
> *rules, int count)
> +{
> +    if (dev->libusb_device) {
> +        return usbredirhost_check_device_filter(rules, count,
> dev->libusb_device, 0);
> +    } else {
> +        return check_edev_device_filter(dev, rules, count);
> +    }
>  }
>  
>  static int usbredir_read_callback(void *user_data, uint8_t *data, int count)
> @@ -621,6 +678,17 @@ 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) {

Is confusing that the same callback is used for both usbredir parser and
usbredir host but the hello should be parsed only for the host.
Maybe disabling hello for the parser and check the type (hello) here?

> +        /* hello is short header (12) + hello struct (64) + capabilities (4)
> */
> +        int hello_size = sizeof(struct usb_redir_header) + sizeof(struct
> usb_redir_hello_header);

This formulae is confusing with the description. sizeof(struct usb_redir_header)
is 16 but is not the short (32bit) header.

> +        ch->hello_sent = 1;
> +        if (count == hello_size) {

I would say "count >= hello_size" to support future extensions of the
capability array.

> +            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);
> +        }
> +    }
>      res = spice_usbredir_write_callback(ch->user_data, data, count);
>      return res;
>  }
> @@ -641,6 +709,27 @@ int
> spice_usb_backend_read_guest_data(SpiceUsbBackendChannel *ch, uint8_t *data,
>      ch->read_buf_size = count;
>      if (ch->usbredirhost) {

Maybe this could be "ch->usbredirhost || !ch->hello_done_parser" to
parse the 'hello' always with usbredir host ?

>          res = usbredirhost_read_guest_data(ch->usbredirhost);
> +        if (!ch->hello_done_parser) {
> +            int res_parser;
> +            /* usbredirhost should consume hello response */
> +            g_return_val_if_fail(ch->read_buf == NULL,
> USB_REDIR_ERROR_READ_PARSE);
> +
> +            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;
> +            }
> +        }
> +    } else if (ch->parser) {
> +        res = usbredirparser_do_read(ch->parser);
>      } else {
>          res = USB_REDIR_ERROR_IO;
>      }
> @@ -661,6 +750,11 @@ int
> spice_usb_backend_read_guest_data(SpiceUsbBackendChannel *ch, uint8_t *data,
>      }
>      SPICE_DEBUG("%s ch %p, %d bytes, res %d", __FUNCTION__, ch, count, res);
>  
> +    if (ch->rejected) {
> +        ch->rejected = 0;
> +        res = USB_REDIR_ERROR_DEV_REJECTED;
> +    }

Why this is after the read? Won't it discard the value?

> +
>      return res;
>  }
>  
> @@ -690,13 +784,426 @@ 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) {
> -        SPICE_DEBUG("%s ch %p", __FUNCTION__, ch);
> +        SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
>          usbredirhost_free_write_buffer(ch->usbredirhost, data);
> +    } else if (ch->parser) {
> +        SPICE_DEBUG("%s ch %p -> parser", __FUNCTION__, ch);
> +        usbredirparser_free_write_buffer(ch->parser, data);

A bit scary that during a swicth host <-> parser this can call the wrong
function. But currently the implementation for both is to free 'data'.

>      } else {
>          SPICE_DEBUG("%s ch %p - NOBODY TO CALL", __FUNCTION__, ch);
>      }
>  }
>  
> +static void usbredir_control_packet(void *priv,
> +    uint64_t id, struct usb_redir_control_packet_header *h,
> +    uint8_t *data, int data_len)

This style is new, I suppose copied from usbredir, but is not
spice-gtk.

> +{
> +    SpiceUsbBackendChannel *ch = priv;
> +    SpiceUsbBackendDevice *d = ch->attached;
> +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> +    struct usb_redir_control_packet_header response = *h;
> +    uint8_t reqtype = h->requesttype & 0x7f;
> +    gboolean done = FALSE;
> +    void *out_buffer = NULL;
> +
> +    response.status = usb_redir_stall;
> +    SPICE_DEBUG("%s %p: TRVIL %02X %02X %04X %04X %04X",
> +                __FUNCTION__,
> +                ch, h->requesttype, h->request,
> +                h->value, h->index, h->length);
> +
> +    if (!edev) {
> +        SPICE_DEBUG("%s: device not attached", __FUNCTION__);
> +        response.status = usb_redir_ioerror;
> +        done = TRUE;
> +    } else if (reqtype == (LIBUSB_REQUEST_TYPE_STANDARD |
> LIBUSB_RECIPIENT_DEVICE) &&
> +               h->request == LIBUSB_REQUEST_GET_DESCRIPTOR) {
> +        uint16_t size;
> +        done = device_ops(edev)->get_descriptor(edev, h->value >> 8,
> h->value & 0xff,
> +                                                &out_buffer, &size);
> +        response.length = size;
> +        if (done) {
> +            response.status = 0;
> +        }
> +        done = TRUE;
> +    }
> +
> +    if (!done) {
> +        device_ops(edev)->control_request(edev, data, data_len, &response,
> &out_buffer);
> +        done = TRUE;
> +    }
> +
> +    if (response.status) {
> +        response.length = 0;
> +    } else if (response.length > h->length) {
> +        response.length = h->length;
> +    }
> +
> +    SPICE_DEBUG("%s responding with payload of %02X, status %X",
> +                __FUNCTION__, response.length, response.status);
> +    usbredirparser_send_control_packet(ch->parser, id, &response,
> +                                       response.length ? out_buffer : NULL,
> +                                       response.length);
> +
> +    usbredir_write_flush_callback(ch);
> +    usbredirparser_free_packet_data(ch->parser, data);
> +}
> +
> +static void usbredir_bulk_packet(void *priv,
> +    uint64_t id, struct usb_redir_bulk_packet_header *h,
> +    uint8_t *data, int data_len)
> +{
> +    SpiceUsbBackendChannel *ch = priv;
> +    SpiceUsbBackendDevice *d = ch->attached;
> +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> +    struct usb_redir_bulk_packet_header hout = *h;
> +    uint32_t len = (h->length_high << 16) | h->length;
> +    SPICE_DEBUG("%s %p: ep %X, len %u, id %" G_GUINT64_FORMAT, __FUNCTION__,
> +                ch, h->endpoint, len, id);
> +
> +    if (!edev) {
> +        SPICE_DEBUG("%s: device not attached", __FUNCTION__);
> +        hout.status = usb_redir_ioerror;
> +        hout.length = hout.length_high = 0;
> +        SPICE_DEBUG("%s: responding with ZLP status %d", __FUNCTION__,
> hout.status);
> +    } else if (h->endpoint & LIBUSB_ENDPOINT_IN) {
> +        if (device_ops(edev)->bulk_in_request(edev, id, &hout)) {
> +            usbredirparser_free_packet_data(ch->parser, data);
> +            /* completion is asynchronous */
> +            return;
> +        }
> +    } else {
> +        hout.status = usb_redir_stall;
> +        device_ops(edev)->bulk_out_request(edev, h->endpoint, data,
> data_len, &hout.status);
> +        SPICE_DEBUG("%s: responding status %d", __FUNCTION__, hout.status);
> +    }
> +
> +    usbredirparser_send_bulk_packet(ch->parser, id, &hout, NULL, 0);
> +    usbredirparser_free_packet_data(ch->parser, data);
> +    usbredir_write_flush_callback(ch);
> +}
> +
> +static void usbredir_device_reset(void *priv)
> +{
> +    SpiceUsbBackendChannel *ch = priv;
> +    SpiceUsbBackendDevice *d = ch->attached;
> +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> +    SPICE_DEBUG("%s ch %p", __FUNCTION__, ch);
> +    if (edev) {
> +        device_ops(edev)->reset(edev);
> +    }
> +}
> +
> +static void usbredir_interface_info(void *priv,
> +    struct usb_redir_interface_info_header *interface_info)
> +{
> +    SpiceUsbBackendChannel *ch = priv;
> +    SPICE_DEBUG("%s not implemented %p", __FUNCTION__, ch);
> +}
> +
> +static void usbredir_interface_ep_info(void *priv,
> +    struct usb_redir_ep_info_header *ep_info)
> +{
> +    SpiceUsbBackendChannel *ch = priv;
> +    SPICE_DEBUG("%s not implemented %p", __FUNCTION__, ch);
> +}
> +
> +static void usbredir_set_configuration(void *priv,
> +    uint64_t id, struct usb_redir_set_configuration_header
> *set_configuration)
> +{
> +    SpiceUsbBackendChannel *ch = priv;
> +    struct usb_redir_configuration_status_header h;
> +    h.status = 0;
> +    h.configuration = set_configuration->configuration;
> +    SPICE_DEBUG("%s ch %p, cfg %d", __FUNCTION__, ch, h.configuration);
> +    if (ch->attached) {
> +        ch->attached->edev_configured = h.configuration != 0;
> +    }
> +    usbredirparser_send_configuration_status(ch->parser, id, &h);
> +    usbredir_write_flush_callback(ch);
> +}
> +
> +static void usbredir_get_configuration(void *priv, uint64_t id)
> +{
> +    SpiceUsbBackendChannel *ch = priv;
> +    struct usb_redir_configuration_status_header h;
> +    h.status = 0;
> +    h.configuration = ch->attached && ch->attached->edev_configured;
> +    SPICE_DEBUG("%s ch %p, cfg %d", __FUNCTION__, ch, h.configuration);
> +    usbredirparser_send_configuration_status(ch->parser, id, &h);
> +    usbredir_write_flush_callback(ch);
> +}
> +
> +static void usbredir_set_alt_setting(void *priv,
> +    uint64_t id, struct usb_redir_set_alt_setting_header *s)
> +{
> +    SpiceUsbBackendChannel *ch = priv;
> +    struct usb_redir_alt_setting_status_header sh;
> +    sh.status = (!s->interface && !s->alt) ? 0 : usb_redir_stall;
> +    sh.interface = s->interface;
> +    sh.alt = s->alt;
> +    SPICE_DEBUG("%s ch %p, %d:%d", __FUNCTION__, ch, s->interface, s->alt);
> +    usbredirparser_send_alt_setting_status(ch->parser, id, &sh);
> +    usbredir_write_flush_callback(ch);
> +}
> +
> +static void usbredir_get_alt_setting(void *priv,
> +    uint64_t id, struct usb_redir_get_alt_setting_header *s)
> +{
> +    SpiceUsbBackendChannel *ch = priv;
> +    struct usb_redir_alt_setting_status_header sh;
> +    sh.status = (s->interface == 0) ? 0 : usb_redir_stall;
> +    sh.interface = s->interface;
> +    sh.alt = 0;
> +    SPICE_DEBUG("%s ch %p, if %d", __FUNCTION__, ch, s->interface);
> +    usbredirparser_send_alt_setting_status(ch->parser, id, &sh);
> +    usbredir_write_flush_callback(ch);
> +}
> +
> +static void usbredir_cancel_data(void *priv, uint64_t id)
> +{
> +    SpiceUsbBackendChannel *ch = priv;
> +    SpiceUsbBackendDevice *d = ch->attached;
> +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> +    if (!edev) {
> +        SPICE_DEBUG("%s: device not attached", __FUNCTION__);
> +        return;
> +    }
> +    device_ops(edev)->cancel_request(edev, id);
> +}
> +
> +static void usbredir_filter_reject(void *priv)
> +{
> +    SpiceUsbBackendChannel *ch = priv;
> +    SPICE_DEBUG("%s %p", __FUNCTION__, ch);
> +    ch->rejected = 1;
> +}
> +
> +/* Note that the ownership of the rules array is passed on to the callback.
> */
> +static void usbredir_filter_filter(void *priv,
> +    struct usbredirfilter_rule *r, int count)
> +{
> +    SpiceUsbBackendChannel *ch = priv;
> +    SPICE_DEBUG("%s ch %p %d filters", __FUNCTION__, ch, count);
> +
> +    free(ch->rules);
> +
> +    ch->rules = r;
> +    ch->rules_count = count;
> +    if (count) {
> +        int i;
> +        for (i = 0; i < count; i++) {
> +            SPICE_DEBUG("%s class %d, %X:%X",
> +                r[i].allow ? "allowed" : "denied", r[i].device_class,
> +                (uint32_t)r[i].vendor_id, (uint32_t)r[i].product_id);
> +        }
> +    }
> +}
> +
> +static void usbredir_device_disconnect_ack(void *priv)
> +{
> +    SpiceUsbBackendChannel *ch = priv;
> +    SPICE_DEBUG("%s ch %p", __FUNCTION__, ch);
> +    if (ch->parser && ch->wait_disc_ack) {
> +        ch->parser = NULL;
> +        SPICE_DEBUG("%s switch to usbredirhost", __FUNCTION__);
> +        ch->usbredirhost = ch->hidden_host;
> +    }
> +    ch->wait_disc_ack = 0;
> +}
> +
> +static void usbredir_hello(void *priv,
> +    struct usb_redir_hello_header *hello)
> +{
> +    SpiceUsbBackendChannel *ch = priv;
> +    SpiceUsbBackendDevice *d = ch->attached;
> +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> +    struct usb_redir_device_connect_header device_connect;
> +    struct usb_redir_ep_info_header ep_info = { 0 };
> +    struct usb_redir_interface_info_header interface_info = { 0 };
> +    uint8_t *cfg;
> +    uint16_t size, offset = 0;
> +    SPICE_DEBUG("%s %p %sattached %s", __FUNCTION__, ch,
> +        edev ? "" : "not ",  hello ? "" : "(internal)");
> +
> +    if (hello) {
> +        ch->hello_done_parser = 1;
> +    }
> +    if (!edev) {
> +        return;
> +    }
> +    if (!device_ops(edev)->get_descriptor(edev, LIBUSB_DT_CONFIG, 0, (void
> **)&cfg, &size)) {
> +        return;
> +    }
> +    while ((offset + 1) < size) {
> +        uint8_t len  = cfg[offset];
> +        uint8_t type = cfg[offset + 1];
> +        if ((offset + len) > size) {
> +            break;
> +        }
> +        if (type == LIBUSB_DT_INTERFACE) {
> +            uint32_t i = interface_info.interface_count;
> +            uint8_t class, subclass, protocol;
> +            class = cfg[offset + 5];
> +            subclass = cfg[offset + 6];
> +            protocol = cfg[offset + 7];
> +            interface_info.interface_class[i] = class;
> +            interface_info.interface_subclass[i] = subclass;
> +            interface_info.interface_protocol[i] = protocol;
> +            interface_info.interface_count++;
> +            SPICE_DEBUG("%s IF%d: %d/%d/%d", __FUNCTION__, i, class,
> subclass, protocol);
> +        } else if (type == LIBUSB_DT_ENDPOINT) {
> +            uint8_t address = cfg[offset + 2];
> +            uint16_t max_packet_size = 0x100 * cfg[offset + 5] + cfg[offset
> + 4];
> +            uint8_t index = address & 0xf;
> +            if (address & 0x80) index += 0x10;
> +            ep_info.type[index] = cfg[offset + 3] & 0x3;
> +            ep_info.max_packet_size[index] = max_packet_size;
> +            SPICE_DEBUG("%s EP[%02X]: %d/%d", __FUNCTION__, index,
> ep_info.type[index], max_packet_size);
> +        }
> +        offset += len;
> +    }
> +
> +    usbredirparser_send_interface_info(ch->parser, &interface_info);
> +    usbredirparser_send_ep_info(ch->parser, &ep_info);
> +
> +    device_connect.device_class = 0; //d->device_info.class;
> +    device_connect.device_subclass = 0; //d->device_info.subclass;
> +    device_connect.device_protocol = 0; //d->device_info.protocol;;
> +    device_connect.vendor_id = d->device_info.vid;
> +    device_connect.product_id = d->device_info.pid;
> +    device_connect.device_version_bcd = d->device_info.bcdUSB;
> +    device_connect.speed = usb_redir_speed_high;
> +    usbredirparser_send_device_connect(ch->parser, &device_connect);
> +    usbredir_write_flush_callback(ch);
> +}
> +
> +static void initialize_parser(SpiceUsbBackendChannel *ch, gboolean do_hello)
> +{
> +    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;
> +    }
> +
> +    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;

In which case you need to do the hello in the parser instead of the host?
This could cause issues switching from parser back to host in case you
remove an emulated device and you add a real one.

> +        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_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);
> +}
> +
> +/*
> +    We can initialize the usbredirparser with HELLO enabled only in case
> +    the libusb is not active and the usbredirhost does not function.
> +    Then the parser sends session HELLO and receives server's response.
> +    Otherwise (usbredirparser initialized with HELLO disabled):
> +    - the usbredirhost sends session HELLO
> +    - we look into it to know set of capabilities we shall initialize
> +      the parser with
> +    - when we receive server's response to HELLO we provide it also to
> +      parser to let it synchronize with server's capabilities
> +*/
> +static struct usbredirparser *create_parser(SpiceUsbBackendChannel *ch)
> +{
> +    struct usbredirparser *parser = usbredirparser_create();
> +
> +    g_return_val_if_fail(parser != NULL, NULL);
> +
> +    parser->priv = ch;
> +    parser->log_func = usbredir_log;
> +    parser->read_func = usbredir_read_callback;
> +    parser->write_func = usbredir_write_callback;
> +    parser->reset_func = usbredir_device_reset;
> +    parser->set_configuration_func = usbredir_set_configuration;
> +    parser->get_configuration_func = usbredir_get_configuration;
> +    parser->set_alt_setting_func = usbredir_set_alt_setting;
> +    parser->get_alt_setting_func = usbredir_get_alt_setting;
> +    parser->cancel_data_packet_func = usbredir_cancel_data;
> +    parser->control_packet_func = usbredir_control_packet;
> +    parser->bulk_packet_func = usbredir_bulk_packet;
> +    parser->alloc_lock_func = usbredir_alloc_lock;
> +    parser->lock_func = usbredir_lock_lock;
> +    parser->unlock_func = usbredir_unlock_lock;
> +    parser->free_lock_func = usbredir_free_lock;
> +    parser->hello_func = usbredir_hello;
> +    parser->filter_reject_func = usbredir_filter_reject;
> +    parser->device_disconnect_ack_func = usbredir_device_disconnect_ack;
> +    parser->interface_info_func = usbredir_interface_info;
> +    parser->ep_info_func = usbredir_interface_ep_info;
> +    parser->filter_filter_func = usbredir_filter_filter;
> +
> +    return parser;
> +}
> +
> +static gboolean attach_edev(SpiceUsbBackendChannel *ch,
> +                            SpiceUsbBackendDevice *dev,
> +                            GError **error)
> +{
> +    if (!dev->edev) {
> +        g_set_error(error, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> +           _("Failed to redirect device %d"), 1);

Minor: I would say more an assert level error, the device should be real or emulated.

> +        return FALSE;
> +    }
> +    if (ch->usbredirhost && !ch->hello_done_parser) {
> +        /*
> +            we can't initialize parser until we see hello from usbredir
> +            and the parser can't work until it sees the hello response.
> +            this is case of autoconnect when emulated device is attached
> +            before the channel is up
> +        */
> +        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->wait_disc_ack = 0;
> +    ch->attached = dev;
> +    dev->attached_to = ch;
> +    device_ops(dev->edev)->attach(dev->edev, ch->hidden_parser);
> +    if (ch->hello_done_parser) {
> +        /* send device info */
> +        usbredir_hello(ch, NULL);
> +    }
> +    return TRUE;
> +}
> +
>  gboolean spice_usb_backend_channel_attach(SpiceUsbBackendChannel *ch,
>                                            SpiceUsbBackendDevice *dev,
>                                            GError **error)
> @@ -706,7 +1213,13 @@ gboolean
> spice_usb_backend_channel_attach(SpiceUsbBackendChannel *ch,
>  
>      g_return_val_if_fail(dev != NULL, FALSE);
>  
> +    if (!dev->libusb_device) {
> +        return attach_edev(ch, dev, error);
> +    }
> +
>      libusb_device_handle *handle = NULL;
> +    ch->usbredirhost = ch->hidden_host;
> +    ch->parser = NULL;
>  
>      /*
>         Under Windows we need to avoid updating
> @@ -740,18 +1253,33 @@ gboolean
> spice_usb_backend_channel_attach(SpiceUsbBackendChannel *ch,
>  
>  void spice_usb_backend_channel_detach(SpiceUsbBackendChannel *ch)
>  {
> +    SpiceUsbBackendDevice *d = ch->attached;
> +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
>      SPICE_DEBUG("%s >> ch %p, was attached %p", __FUNCTION__, ch,
>      ch->attached);
> -    if (!ch->attached) {
> +    if (!d) {
>          SPICE_DEBUG("%s: nothing to detach", __FUNCTION__);
>          return;
>      }
>      if (ch->usbredirhost) {
>          /* it will call libusb_close internally */
>          usbredirhost_set_device(ch->usbredirhost, NULL);
> +    } else if (ch->parser) {
> +        if (edev) {
> +            device_ops(edev)->detach(edev);
> +        }
> +        usbredirparser_send_device_disconnect(ch->parser);
> +        usbredir_write_flush_callback(ch);
> +        ch->wait_disc_ack = usbredirparser_peer_has_cap(ch->parser,
> +
> usb_redir_cap_device_disconnect_ack);
> +        if (!ch->wait_disc_ack) {
> +            ch->usbredirhost = ch->hidden_host;
> +            ch->parser = NULL;
> +        }
>      }
>      SPICE_DEBUG("%s ch %p, detach done", __FUNCTION__, ch);
>      ch->attached->attached_to = NULL;
>      ch->attached = NULL;
> +    ch->rejected = 0;
>  }
>  
>  SpiceUsbBackendChannel *spice_usb_backend_channel_new(SpiceUsbBackend *be,
> @@ -766,26 +1294,33 @@ SpiceUsbBackendChannel
> *spice_usb_backend_channel_new(SpiceUsbBackend *be,
>      ch->user_data = SPICE_USBREDIR_CHANNEL(user_data);
>      if (be->libusb_context) {

This can never be NULL, if we are not able to allocate it inside
spice_usb_backend_new the function returns NULL.
This should be in a preliminary patch to make this clear.

>          ch->backend = be;
> -        ch->usbredirhost = usbredirhost_open_full(
> -            be->libusb_context,
> -            NULL,
> -            usbredir_log,
> -            usbredir_read_callback,
> -            usbredir_write_callback,
> -            usbredir_write_flush_callback,
> -            usbredir_alloc_lock,
> -            usbredir_lock_lock,
> -            usbredir_unlock_lock,
> -            usbredir_free_lock,
> -            ch, PACKAGE_STRING,
> -            spice_util_get_debug() ? usbredirparser_debug :
> usbredirparser_warning,
> -            usbredirhost_fl_write_cb_owns_buffer);
> +        ch->usbredirhost =
> +            usbredirhost_open_full(be->libusb_context,
> +                                   NULL,
> +                                   usbredir_log,
> +                                   usbredir_read_callback,
> +                                   usbredir_write_callback,
> +                                   usbredir_write_flush_callback,
> +                                   usbredir_alloc_lock,
> +                                   usbredir_lock_lock,
> +                                   usbredir_unlock_lock,
> +                                   usbredir_free_lock,
> +                                   ch, PACKAGE_STRING,
> +                                   spice_util_get_debug() ?
> usbredirparser_debug :
> +                                        usbredirparser_warning,
> +                                   usbredirhost_fl_write_cb_owns_buffer);
>          g_warn_if_fail(ch->usbredirhost != NULL);
>      }
> +
>      if (ch->usbredirhost) {
> -        usbredirhost_set_buffered_output_size_cb(ch->usbredirhost,
> usbredir_buffered_output_size_callback);
> -    } else {
> -        g_free(ch);
> +        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) {
> +        spice_usb_backend_channel_delete(ch);
>          ch = NULL;
>      }
>  
> @@ -795,9 +1330,14 @@ SpiceUsbBackendChannel
> *spice_usb_backend_channel_new(SpiceUsbBackend *be,
>  
>  void spice_usb_backend_channel_flush_writes(SpiceUsbBackendChannel *ch)
>  {
> -    SPICE_DEBUG("%s %p, host %p", __FUNCTION__, ch, ch->usbredirhost);
> +    SPICE_DEBUG("%s %p is up", __FUNCTION__, ch);
>      if (ch->usbredirhost) {
>          usbredirhost_write_guest_data(ch->usbredirhost);
> +        initialize_parser(ch, FALSE);
> +    } else {
> +        initialize_parser(ch, TRUE);

Why initialize_parse is called here?

> +        ch->parser = ch->hidden_parser;
> +        usbredirparser_do_write(ch->parser);
>      }
>  }
>  
> @@ -807,12 +1347,16 @@ void
> spice_usb_backend_channel_delete(SpiceUsbBackendChannel *ch)
>      if (!ch) {
>          return;
>      }
> -    if (ch->usbredirhost) {
> -        usbredirhost_close(ch->usbredirhost);
> +    if (ch->hidden_host) {
> +        usbredirhost_close(ch->hidden_host);
> +    }
> +    if (ch->hidden_parser) {
> +        usbredirparser_destroy(ch->hidden_parser);
>      }
>  
>      if (ch->rules) {
> -        g_free(ch->rules);
> +        /* rules were allocated by usbredirparser */
> +        free(ch->rules);

Minor :I suppose this small change can go to a preliminary patch too

>      }
>  
>      g_free(ch);

Frediano
On Tue, Aug 13, 2019 at 2:59 PM Frediano Ziglio <fziglio@redhat.com> wrote:
>
> >
> > Redirection of emulated devices requires special approach,
> > as usbredirhost can't be used for that (it works only with
> > libusb devices). For emulated devices we create instance of
> > usbredirparser that implements USB redirection protocol.
>
> Make sense. Actually usbredirhost internally uses a parser to
> parse protocol messages and redirect to the physical device.
> But the parse is internal and is used by usbredirhost so
> would be hard to reuse.
>
> > In order to work with the same set of protocol capabilities
> > that usbredirhost sets up with remote side, the parser shall:
> > - not send its own 'hello' to the server
> > - initialize the same capabilities that usbredirhost
> > - receive the same 'hello' response
> > For that we analyze 'hello' sent by USB redir parser and
> > extract set of capabilities from it and upon receive of
> > 'hello' response we provide it also to the parser.
>
> This make sense but some part of the code does not exactly that.
> In some path the code send the 'hello' is sent by the parser.
> It would be more clear to implement just what you described
> above, why the implementation is a bit different.

Due to corner case described below.

>
> > When libusb device is redirected via a channel, instance of
> > usbredirhost serves it.
> > When emulated device is redirected via a channel, instance
> > of usbredirparser does the job.
> >
> > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> > ---
> >  src/usb-backend.c | 616 +++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 580 insertions(+), 36 deletions(-)
> >
> > diff --git a/src/usb-backend.c b/src/usb-backend.c
> > index aa11c79..9aee6c7 100644
> > --- a/src/usb-backend.c
> > +++ b/src/usb-backend.c
> > @@ -55,6 +55,7 @@ struct _SpiceUsbBackendDevice
> >      gint ref_count;
> >      SpiceUsbBackendChannel *attached_to;
> >      UsbDeviceInformation device_info;
> > +    gboolean edev_configured;
>
> This variable is never reset, I would expect that at least
> when device is detached (this should be equivalent to physical
> removal).

Probably, need to reset it on attach.
In fact, nobody uses it, the driver usually just sets the configuration.

> Why not moving to SpiceUsbBackendChannel?
>

Because it is hardly related to the channel.

> >  };
> >
> >  struct _SpiceUsbBackend
> > @@ -80,10 +81,20 @@ struct _SpiceUsbBackend
> >  struct _SpiceUsbBackendChannel
> >  {
> >      struct usbredirhost *usbredirhost;
> > +    struct usbredirhost *hidden_host;
> > +    struct usbredirparser *parser;
> > +    struct usbredirparser *hidden_parser;
>
> I don't understand all these variable.
> Won't be easier to have parser initialized for emulated
> devices and being NULL (so freed) when emulated device
> is detached?

When the channel is created it does not know what it will redirect.
The same channel is used for regular devices and emulated one. Both
parser and usbredirhost shall be internally synchronized with the
remote side.
If I'd create the parser each time the emulated device is attached I'd
need each time to initialize it with received hello from the second
side. We'd need to write additional code to process (even imaginable)
error cases (what if we can't create the parser etc).
Finally this appears to be more complicated than have everythign ready
from the beginning.

There is also corner case when the device is attached before the
channel is ready (redirect-on-connect setting). We decide on
usbredihost' or 'parser' depending on kind of attached device to
redirect.
If the channel is still not ready, the parser/usbredirhost can't send
anything till the channel is up. In this case switching back and forth
between usbredirhost becomes more complicated than current solution.

>
> >      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_disc_ack     : 1;
>
> The "disc" naming is confusing, also taking into account the
> "Compact Disc" addition. Reading the code this "disc" is a shorten
> for "disconnect", maybe would be better to just use "wait_disconnect_ack"?
>

ok

> >      SpiceUsbBackendDevice *attached;
> >      SpiceUsbredirChannel  *user_data;
>
> OT: this definitively should be renamed!

Probably in separate commit.

>
> >      SpiceUsbBackend *backend;
> > @@ -355,7 +366,11 @@ gboolean
> > spice_usb_backend_device_isoch(SpiceUsbBackendDevice *dev)
> >      gint i, j, k;
> >      int rc;
> >
> > -    g_return_val_if_fail(libdev != NULL, 0);
> > +    g_return_val_if_fail(libdev != NULL || dev->edev != NULL, 0);
> > +    if (dev->edev) {
> > +        /* currently we do not emulate isoch devices */
> > +        return FALSE;
> > +    }
> >
> >      rc = libusb_get_active_config_descriptor(libdev, &conf_desc);
> >      if (rc) {
> > @@ -390,13 +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->usbredirhost) {
> > -        /* just to be on the safe side */
> > -        return;
> > -    }
>
> These check should be in a preparatory patch. Nothing wrong with this patch,
> but the code (master) is confusing. usbredirhost is initialized as soon as
> possible, can be NULL only before usbredirhost_open_full returns.
> Actually can be NULL in this call for this reason (this callback is called
> inside usbredirhost_open_full).
>
> >      if (is_channel_ready(ch->user_data)) {
> > -        SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
> > -        usbredirhost_write_guest_data(ch->usbredirhost);
> > +        if (ch->usbredirhost) {
> > +            SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
> > +            usbredirhost_write_guest_data(ch->usbredirhost);
> > +        } else if (ch->parser) {
> > +            SPICE_DEBUG("%s ch %p -> parser", __FUNCTION__, ch);
> > +            usbredirparser_do_write(ch->parser);
> > +        } else {
> > +            SPICE_DEBUG("%s ch %p (NOT ACTIVE)", __FUNCTION__, ch);
>
> This third case should never be possible. This is one aspect that master
> code is confusing about.
>
> > +        }
> >      } else {
> >          SPICE_DEBUG("%s ch %p (not ready)", __FUNCTION__, ch);
> >      }
> > @@ -551,13 +569,52 @@ void
> > spice_usb_backend_device_unref(SpiceUsbBackendDevice *dev)
> >      }
> >  }
> >
> > -int spice_usb_backend_device_check_filter(
> > -    SpiceUsbBackendDevice *dev,
> > -    const struct usbredirfilter_rule *rules,
> > -    int count)
> > +static int check_edev_device_filter(SpiceUsbBackendDevice *dev,
> > +                                    const struct usbredirfilter_rule *rules,
> > +                                    int count)
> >  {
> > -    return usbredirhost_check_device_filter(
> > -        rules, count, dev->libusb_device, 0);
> > +    SpiceUsbEmulatedDevice *edev = dev->edev;
> > +    uint8_t cls[32], subcls[32], proto[32], *cfg, ifnum = 0;
> > +    uint16_t size, offset = 0;
> > +    if (!edev) {
> > +        return 1;
> > +    }
> > +    if (!device_ops(edev)->get_descriptor(edev, LIBUSB_DT_CONFIG, 0, (void
> > **)&cfg, &size)) {
> > +        return 1;
> > +    }
> > +    while ((offset + 1) < size) {
> > +        uint8_t len  = cfg[offset];
> > +        uint8_t type = cfg[offset + 1];
> > +        if ((offset + len) > size) {
> > +            break;
> > +        }
> > +        if (type == LIBUSB_DT_INTERFACE) {
> > +            cls[ifnum] = cfg[offset + 5];
> > +            subcls[ifnum] = cfg[offset + 6];
> > +            proto[ifnum] = cfg[offset + 7];
> > +            ifnum++;
> > +        }
> > +        offset += len;
> > +    }
> > +
> > +    return usbredirfilter_check(rules, count,
> > +                                dev->device_info.class,
> > +                                dev->device_info.subclass,
> > +                                dev->device_info.protocol,
> > +                                cls, subcls, proto, ifnum,
> > +                                dev->device_info.vid,
> > +                                dev->device_info.pid,
> > +                                dev->device_info.bcdUSB, 0);
> > +}
> > +
> > +int spice_usb_backend_device_check_filter(SpiceUsbBackendDevice *dev,
> > +                                          const struct usbredirfilter_rule
> > *rules, int count)
> > +{
> > +    if (dev->libusb_device) {
> > +        return usbredirhost_check_device_filter(rules, count,
> > dev->libusb_device, 0);
> > +    } else {
> > +        return check_edev_device_filter(dev, rules, count);
> > +    }
> >  }
> >
> >  static int usbredir_read_callback(void *user_data, uint8_t *data, int count)
> > @@ -621,6 +678,17 @@ 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) {
>
> Is confusing that the same callback is used for both usbredir parser and
> usbredir host but the hello should be parsed only for the host.
> Maybe disabling hello for the parser and check the type (hello) here?



>
> > +        /* hello is short header (12) + hello struct (64) + capabilities (4)
> > */
> > +        int hello_size = sizeof(struct usb_redir_header) + sizeof(struct
> > usb_redir_hello_header);
>
> This formulae is confusing with the description. sizeof(struct usb_redir_header)
> is 16 but is not the short (32bit) header.
>

At stage of sending hello the parser (also when is works under
usbredirhost) does not know what the second side support, so it
uses legacy-compatible structure. If size of hello is bigger than we
expect, something is completely not good.
We need to find capabilities in the packet, so we need to know for
sure where to look for them.

> > +        ch->hello_sent = 1;
> > +        if (count == hello_size) {
>
> I would say "count >= hello_size" to support future extensions of the
> capability array.

If there are capabilities that we're not aware of, we're not
synchronized with the usbredir library.

>
> > +            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);
> > +        }
> > +    }
> >      res = spice_usbredir_write_callback(ch->user_data, data, count);
> >      return res;
> >  }
> > @@ -641,6 +709,27 @@ int
> > spice_usb_backend_read_guest_data(SpiceUsbBackendChannel *ch, uint8_t *data,
> >      ch->read_buf_size = count;
> >      if (ch->usbredirhost) {
>
> Maybe this could be "ch->usbredirhost || !ch->hello_done_parser" to
> parse the 'hello' always with usbredir host ?

I expect this will make the code more complicated (from previous attempts).

>
> >          res = usbredirhost_read_guest_data(ch->usbredirhost);
> > +        if (!ch->hello_done_parser) {
> > +            int res_parser;
> > +            /* usbredirhost should consume hello response */
> > +            g_return_val_if_fail(ch->read_buf == NULL,
> > USB_REDIR_ERROR_READ_PARSE);
> > +
> > +            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;
> > +            }
> > +        }
> > +    } else if (ch->parser) {
> > +        res = usbredirparser_do_read(ch->parser);
> >      } else {
> >          res = USB_REDIR_ERROR_IO;
> >      }
> > @@ -661,6 +750,11 @@ int
> > spice_usb_backend_read_guest_data(SpiceUsbBackendChannel *ch, uint8_t *data,
> >      }
> >      SPICE_DEBUG("%s ch %p, %d bytes, res %d", __FUNCTION__, ch, count, res);
> >
> > +    if (ch->rejected) {
> > +        ch->rejected = 0;
> > +        res = USB_REDIR_ERROR_DEV_REJECTED;
> > +    }
>
> Why this is after the read? Won't it discard the value?

Because we need to provide the error to the usb-redir-channel.
We know about 'reject' from our callback that is not in context of
usb-redir-channel operation.

>
> > +
> >      return res;
> >  }
> >
> > @@ -690,13 +784,426 @@ 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) {
> > -        SPICE_DEBUG("%s ch %p", __FUNCTION__, ch);
> > +        SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
> >          usbredirhost_free_write_buffer(ch->usbredirhost, data);
> > +    } else if (ch->parser) {
> > +        SPICE_DEBUG("%s ch %p -> parser", __FUNCTION__, ch);
> > +        usbredirparser_free_write_buffer(ch->parser, data);
>
> A bit scary that during a swicth host <-> parser this can call the wrong
> function. But currently the implementation for both is to free 'data'.
>
> >      } else {
> >          SPICE_DEBUG("%s ch %p - NOBODY TO CALL", __FUNCTION__, ch);
> >      }
> >  }
> >
> > +static void usbredir_control_packet(void *priv,
> > +    uint64_t id, struct usb_redir_control_packet_header *h,
> > +    uint8_t *data, int data_len)
>
> This style is new, I suppose copied from usbredir, but is not
> spice-gtk.
>
> > +{
> > +    SpiceUsbBackendChannel *ch = priv;
> > +    SpiceUsbBackendDevice *d = ch->attached;
> > +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> > +    struct usb_redir_control_packet_header response = *h;
> > +    uint8_t reqtype = h->requesttype & 0x7f;
> > +    gboolean done = FALSE;
> > +    void *out_buffer = NULL;
> > +
> > +    response.status = usb_redir_stall;
> > +    SPICE_DEBUG("%s %p: TRVIL %02X %02X %04X %04X %04X",
> > +                __FUNCTION__,
> > +                ch, h->requesttype, h->request,
> > +                h->value, h->index, h->length);
> > +
> > +    if (!edev) {
> > +        SPICE_DEBUG("%s: device not attached", __FUNCTION__);
> > +        response.status = usb_redir_ioerror;
> > +        done = TRUE;
> > +    } else if (reqtype == (LIBUSB_REQUEST_TYPE_STANDARD |
> > LIBUSB_RECIPIENT_DEVICE) &&
> > +               h->request == LIBUSB_REQUEST_GET_DESCRIPTOR) {
> > +        uint16_t size;
> > +        done = device_ops(edev)->get_descriptor(edev, h->value >> 8,
> > h->value & 0xff,
> > +                                                &out_buffer, &size);
> > +        response.length = size;
> > +        if (done) {
> > +            response.status = 0;
> > +        }
> > +        done = TRUE;
> > +    }
> > +
> > +    if (!done) {
> > +        device_ops(edev)->control_request(edev, data, data_len, &response,
> > &out_buffer);
> > +        done = TRUE;
> > +    }
> > +
> > +    if (response.status) {
> > +        response.length = 0;
> > +    } else if (response.length > h->length) {
> > +        response.length = h->length;
> > +    }
> > +
> > +    SPICE_DEBUG("%s responding with payload of %02X, status %X",
> > +                __FUNCTION__, response.length, response.status);
> > +    usbredirparser_send_control_packet(ch->parser, id, &response,
> > +                                       response.length ? out_buffer : NULL,
> > +                                       response.length);
> > +
> > +    usbredir_write_flush_callback(ch);
> > +    usbredirparser_free_packet_data(ch->parser, data);
> > +}
> > +
> > +static void usbredir_bulk_packet(void *priv,
> > +    uint64_t id, struct usb_redir_bulk_packet_header *h,
> > +    uint8_t *data, int data_len)
> > +{
> > +    SpiceUsbBackendChannel *ch = priv;
> > +    SpiceUsbBackendDevice *d = ch->attached;
> > +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> > +    struct usb_redir_bulk_packet_header hout = *h;
> > +    uint32_t len = (h->length_high << 16) | h->length;
> > +    SPICE_DEBUG("%s %p: ep %X, len %u, id %" G_GUINT64_FORMAT, __FUNCTION__,
> > +                ch, h->endpoint, len, id);
> > +
> > +    if (!edev) {
> > +        SPICE_DEBUG("%s: device not attached", __FUNCTION__);
> > +        hout.status = usb_redir_ioerror;
> > +        hout.length = hout.length_high = 0;
> > +        SPICE_DEBUG("%s: responding with ZLP status %d", __FUNCTION__,
> > hout.status);
> > +    } else if (h->endpoint & LIBUSB_ENDPOINT_IN) {
> > +        if (device_ops(edev)->bulk_in_request(edev, id, &hout)) {
> > +            usbredirparser_free_packet_data(ch->parser, data);
> > +            /* completion is asynchronous */
> > +            return;
> > +        }
> > +    } else {
> > +        hout.status = usb_redir_stall;
> > +        device_ops(edev)->bulk_out_request(edev, h->endpoint, data,
> > data_len, &hout.status);
> > +        SPICE_DEBUG("%s: responding status %d", __FUNCTION__, hout.status);
> > +    }
> > +
> > +    usbredirparser_send_bulk_packet(ch->parser, id, &hout, NULL, 0);
> > +    usbredirparser_free_packet_data(ch->parser, data);
> > +    usbredir_write_flush_callback(ch);
> > +}
> > +
> > +static void usbredir_device_reset(void *priv)
> > +{
> > +    SpiceUsbBackendChannel *ch = priv;
> > +    SpiceUsbBackendDevice *d = ch->attached;
> > +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> > +    SPICE_DEBUG("%s ch %p", __FUNCTION__, ch);
> > +    if (edev) {
> > +        device_ops(edev)->reset(edev);
> > +    }
> > +}
> > +
> > +static void usbredir_interface_info(void *priv,
> > +    struct usb_redir_interface_info_header *interface_info)
> > +{
> > +    SpiceUsbBackendChannel *ch = priv;
> > +    SPICE_DEBUG("%s not implemented %p", __FUNCTION__, ch);
> > +}
> > +
> > +static void usbredir_interface_ep_info(void *priv,
> > +    struct usb_redir_ep_info_header *ep_info)
> > +{
> > +    SpiceUsbBackendChannel *ch = priv;
> > +    SPICE_DEBUG("%s not implemented %p", __FUNCTION__, ch);
> > +}
> > +
> > +static void usbredir_set_configuration(void *priv,
> > +    uint64_t id, struct usb_redir_set_configuration_header
> > *set_configuration)
> > +{
> > +    SpiceUsbBackendChannel *ch = priv;
> > +    struct usb_redir_configuration_status_header h;
> > +    h.status = 0;
> > +    h.configuration = set_configuration->configuration;
> > +    SPICE_DEBUG("%s ch %p, cfg %d", __FUNCTION__, ch, h.configuration);
> > +    if (ch->attached) {
> > +        ch->attached->edev_configured = h.configuration != 0;
> > +    }
> > +    usbredirparser_send_configuration_status(ch->parser, id, &h);
> > +    usbredir_write_flush_callback(ch);
> > +}
> > +
> > +static void usbredir_get_configuration(void *priv, uint64_t id)
> > +{
> > +    SpiceUsbBackendChannel *ch = priv;
> > +    struct usb_redir_configuration_status_header h;
> > +    h.status = 0;
> > +    h.configuration = ch->attached && ch->attached->edev_configured;
> > +    SPICE_DEBUG("%s ch %p, cfg %d", __FUNCTION__, ch, h.configuration);
> > +    usbredirparser_send_configuration_status(ch->parser, id, &h);
> > +    usbredir_write_flush_callback(ch);
> > +}
> > +
> > +static void usbredir_set_alt_setting(void *priv,
> > +    uint64_t id, struct usb_redir_set_alt_setting_header *s)
> > +{
> > +    SpiceUsbBackendChannel *ch = priv;
> > +    struct usb_redir_alt_setting_status_header sh;
> > +    sh.status = (!s->interface && !s->alt) ? 0 : usb_redir_stall;
> > +    sh.interface = s->interface;
> > +    sh.alt = s->alt;
> > +    SPICE_DEBUG("%s ch %p, %d:%d", __FUNCTION__, ch, s->interface, s->alt);
> > +    usbredirparser_send_alt_setting_status(ch->parser, id, &sh);
> > +    usbredir_write_flush_callback(ch);
> > +}
> > +
> > +static void usbredir_get_alt_setting(void *priv,
> > +    uint64_t id, struct usb_redir_get_alt_setting_header *s)
> > +{
> > +    SpiceUsbBackendChannel *ch = priv;
> > +    struct usb_redir_alt_setting_status_header sh;
> > +    sh.status = (s->interface == 0) ? 0 : usb_redir_stall;
> > +    sh.interface = s->interface;
> > +    sh.alt = 0;
> > +    SPICE_DEBUG("%s ch %p, if %d", __FUNCTION__, ch, s->interface);
> > +    usbredirparser_send_alt_setting_status(ch->parser, id, &sh);
> > +    usbredir_write_flush_callback(ch);
> > +}
> > +
> > +static void usbredir_cancel_data(void *priv, uint64_t id)
> > +{
> > +    SpiceUsbBackendChannel *ch = priv;
> > +    SpiceUsbBackendDevice *d = ch->attached;
> > +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> > +    if (!edev) {
> > +        SPICE_DEBUG("%s: device not attached", __FUNCTION__);
> > +        return;
> > +    }
> > +    device_ops(edev)->cancel_request(edev, id);
> > +}
> > +
> > +static void usbredir_filter_reject(void *priv)
> > +{
> > +    SpiceUsbBackendChannel *ch = priv;
> > +    SPICE_DEBUG("%s %p", __FUNCTION__, ch);
> > +    ch->rejected = 1;
> > +}
> > +
> > +/* Note that the ownership of the rules array is passed on to the callback.
> > */
> > +static void usbredir_filter_filter(void *priv,
> > +    struct usbredirfilter_rule *r, int count)
> > +{
> > +    SpiceUsbBackendChannel *ch = priv;
> > +    SPICE_DEBUG("%s ch %p %d filters", __FUNCTION__, ch, count);
> > +
> > +    free(ch->rules);
> > +
> > +    ch->rules = r;
> > +    ch->rules_count = count;
> > +    if (count) {
> > +        int i;
> > +        for (i = 0; i < count; i++) {
> > +            SPICE_DEBUG("%s class %d, %X:%X",
> > +                r[i].allow ? "allowed" : "denied", r[i].device_class,
> > +                (uint32_t)r[i].vendor_id, (uint32_t)r[i].product_id);
> > +        }
> > +    }
> > +}
> > +
> > +static void usbredir_device_disconnect_ack(void *priv)
> > +{
> > +    SpiceUsbBackendChannel *ch = priv;
> > +    SPICE_DEBUG("%s ch %p", __FUNCTION__, ch);
> > +    if (ch->parser && ch->wait_disc_ack) {
> > +        ch->parser = NULL;
> > +        SPICE_DEBUG("%s switch to usbredirhost", __FUNCTION__);
> > +        ch->usbredirhost = ch->hidden_host;
> > +    }
> > +    ch->wait_disc_ack = 0;
> > +}
> > +
> > +static void usbredir_hello(void *priv,
> > +    struct usb_redir_hello_header *hello)
> > +{
> > +    SpiceUsbBackendChannel *ch = priv;
> > +    SpiceUsbBackendDevice *d = ch->attached;
> > +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> > +    struct usb_redir_device_connect_header device_connect;
> > +    struct usb_redir_ep_info_header ep_info = { 0 };
> > +    struct usb_redir_interface_info_header interface_info = { 0 };
> > +    uint8_t *cfg;
> > +    uint16_t size, offset = 0;
> > +    SPICE_DEBUG("%s %p %sattached %s", __FUNCTION__, ch,
> > +        edev ? "" : "not ",  hello ? "" : "(internal)");
> > +
> > +    if (hello) {
> > +        ch->hello_done_parser = 1;
> > +    }
> > +    if (!edev) {
> > +        return;
> > +    }
> > +    if (!device_ops(edev)->get_descriptor(edev, LIBUSB_DT_CONFIG, 0, (void
> > **)&cfg, &size)) {
> > +        return;
> > +    }
> > +    while ((offset + 1) < size) {
> > +        uint8_t len  = cfg[offset];
> > +        uint8_t type = cfg[offset + 1];
> > +        if ((offset + len) > size) {
> > +            break;
> > +        }
> > +        if (type == LIBUSB_DT_INTERFACE) {
> > +            uint32_t i = interface_info.interface_count;
> > +            uint8_t class, subclass, protocol;
> > +            class = cfg[offset + 5];
> > +            subclass = cfg[offset + 6];
> > +            protocol = cfg[offset + 7];
> > +            interface_info.interface_class[i] = class;
> > +            interface_info.interface_subclass[i] = subclass;
> > +            interface_info.interface_protocol[i] = protocol;
> > +            interface_info.interface_count++;
> > +            SPICE_DEBUG("%s IF%d: %d/%d/%d", __FUNCTION__, i, class,
> > subclass, protocol);
> > +        } else if (type == LIBUSB_DT_ENDPOINT) {
> > +            uint8_t address = cfg[offset + 2];
> > +            uint16_t max_packet_size = 0x100 * cfg[offset + 5] + cfg[offset
> > + 4];
> > +            uint8_t index = address & 0xf;
> > +            if (address & 0x80) index += 0x10;
> > +            ep_info.type[index] = cfg[offset + 3] & 0x3;
> > +            ep_info.max_packet_size[index] = max_packet_size;
> > +            SPICE_DEBUG("%s EP[%02X]: %d/%d", __FUNCTION__, index,
> > ep_info.type[index], max_packet_size);
> > +        }
> > +        offset += len;
> > +    }
> > +
> > +    usbredirparser_send_interface_info(ch->parser, &interface_info);
> > +    usbredirparser_send_ep_info(ch->parser, &ep_info);
> > +
> > +    device_connect.device_class = 0; //d->device_info.class;
> > +    device_connect.device_subclass = 0; //d->device_info.subclass;
> > +    device_connect.device_protocol = 0; //d->device_info.protocol;;
> > +    device_connect.vendor_id = d->device_info.vid;
> > +    device_connect.product_id = d->device_info.pid;
> > +    device_connect.device_version_bcd = d->device_info.bcdUSB;
> > +    device_connect.speed = usb_redir_speed_high;
> > +    usbredirparser_send_device_connect(ch->parser, &device_connect);
> > +    usbredir_write_flush_callback(ch);
> > +}
> > +
> > +static void initialize_parser(SpiceUsbBackendChannel *ch, gboolean do_hello)
> > +{
> > +    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;
> > +    }
> > +
> > +    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;
>
> In which case you need to do the hello in the parser instead of the host?
> This could cause issues switching from parser back to host in case you
> remove an emulated device and you add a real one.

The case is described above. From my point of view, having both
(usbredirhost and parser) ready and switch
according to the current need is the simplest way to do the job.

>
> > +        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_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);
> > +}
> > +
> > +/*
> > +    We can initialize the usbredirparser with HELLO enabled only in case
> > +    the libusb is not active and the usbredirhost does not function.
> > +    Then the parser sends session HELLO and receives server's response.
> > +    Otherwise (usbredirparser initialized with HELLO disabled):
> > +    - the usbredirhost sends session HELLO
> > +    - we look into it to know set of capabilities we shall initialize
> > +      the parser with
> > +    - when we receive server's response to HELLO we provide it also to
> > +      parser to let it synchronize with server's capabilities
> > +*/
> > +static struct usbredirparser *create_parser(SpiceUsbBackendChannel *ch)
> > +{
> > +    struct usbredirparser *parser = usbredirparser_create();
> > +
> > +    g_return_val_if_fail(parser != NULL, NULL);
> > +
> > +    parser->priv = ch;
> > +    parser->log_func = usbredir_log;
> > +    parser->read_func = usbredir_read_callback;
> > +    parser->write_func = usbredir_write_callback;
> > +    parser->reset_func = usbredir_device_reset;
> > +    parser->set_configuration_func = usbredir_set_configuration;
> > +    parser->get_configuration_func = usbredir_get_configuration;
> > +    parser->set_alt_setting_func = usbredir_set_alt_setting;
> > +    parser->get_alt_setting_func = usbredir_get_alt_setting;
> > +    parser->cancel_data_packet_func = usbredir_cancel_data;
> > +    parser->control_packet_func = usbredir_control_packet;
> > +    parser->bulk_packet_func = usbredir_bulk_packet;
> > +    parser->alloc_lock_func = usbredir_alloc_lock;
> > +    parser->lock_func = usbredir_lock_lock;
> > +    parser->unlock_func = usbredir_unlock_lock;
> > +    parser->free_lock_func = usbredir_free_lock;
> > +    parser->hello_func = usbredir_hello;
> > +    parser->filter_reject_func = usbredir_filter_reject;
> > +    parser->device_disconnect_ack_func = usbredir_device_disconnect_ack;
> > +    parser->interface_info_func = usbredir_interface_info;
> > +    parser->ep_info_func = usbredir_interface_ep_info;
> > +    parser->filter_filter_func = usbredir_filter_filter;
> > +
> > +    return parser;
> > +}
> > +
> > +static gboolean attach_edev(SpiceUsbBackendChannel *ch,
> > +                            SpiceUsbBackendDevice *dev,
> > +                            GError **error)
> > +{
> > +    if (!dev->edev) {
> > +        g_set_error(error, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> > +           _("Failed to redirect device %d"), 1);
>
> Minor: I would say more an assert level error, the device should be real or emulated.
>
> > +        return FALSE;
> > +    }
> > +    if (ch->usbredirhost && !ch->hello_done_parser) {
> > +        /*
> > +            we can't initialize parser until we see hello from usbredir
> > +            and the parser can't work until it sees the hello response.
> > +            this is case of autoconnect when emulated device is attached
> > +            before the channel is up
> > +        */
> > +        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->wait_disc_ack = 0;
> > +    ch->attached = dev;
> > +    dev->attached_to = ch;
> > +    device_ops(dev->edev)->attach(dev->edev, ch->hidden_parser);
> > +    if (ch->hello_done_parser) {
> > +        /* send device info */
> > +        usbredir_hello(ch, NULL);
> > +    }
> > +    return TRUE;
> > +}
> > +
> >  gboolean spice_usb_backend_channel_attach(SpiceUsbBackendChannel *ch,
> >                                            SpiceUsbBackendDevice *dev,
> >                                            GError **error)
> > @@ -706,7 +1213,13 @@ gboolean
> > spice_usb_backend_channel_attach(SpiceUsbBackendChannel *ch,
> >
> >      g_return_val_if_fail(dev != NULL, FALSE);
> >
> > +    if (!dev->libusb_device) {
> > +        return attach_edev(ch, dev, error);
> > +    }
> > +
> >      libusb_device_handle *handle = NULL;
> > +    ch->usbredirhost = ch->hidden_host;
> > +    ch->parser = NULL;
> >
> >      /*
> >         Under Windows we need to avoid updating
> > @@ -740,18 +1253,33 @@ gboolean
> > spice_usb_backend_channel_attach(SpiceUsbBackendChannel *ch,
> >
> >  void spice_usb_backend_channel_detach(SpiceUsbBackendChannel *ch)
> >  {
> > +    SpiceUsbBackendDevice *d = ch->attached;
> > +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> >      SPICE_DEBUG("%s >> ch %p, was attached %p", __FUNCTION__, ch,
> >      ch->attached);
> > -    if (!ch->attached) {
> > +    if (!d) {
> >          SPICE_DEBUG("%s: nothing to detach", __FUNCTION__);
> >          return;
> >      }
> >      if (ch->usbredirhost) {
> >          /* it will call libusb_close internally */
> >          usbredirhost_set_device(ch->usbredirhost, NULL);
> > +    } else if (ch->parser) {
> > +        if (edev) {
> > +            device_ops(edev)->detach(edev);
> > +        }
> > +        usbredirparser_send_device_disconnect(ch->parser);
> > +        usbredir_write_flush_callback(ch);
> > +        ch->wait_disc_ack = usbredirparser_peer_has_cap(ch->parser,
> > +
> > usb_redir_cap_device_disconnect_ack);
> > +        if (!ch->wait_disc_ack) {
> > +            ch->usbredirhost = ch->hidden_host;
> > +            ch->parser = NULL;
> > +        }
> >      }
> >      SPICE_DEBUG("%s ch %p, detach done", __FUNCTION__, ch);
> >      ch->attached->attached_to = NULL;
> >      ch->attached = NULL;
> > +    ch->rejected = 0;
> >  }
> >
> >  SpiceUsbBackendChannel *spice_usb_backend_channel_new(SpiceUsbBackend *be,
> > @@ -766,26 +1294,33 @@ SpiceUsbBackendChannel
> > *spice_usb_backend_channel_new(SpiceUsbBackend *be,
> >      ch->user_data = SPICE_USBREDIR_CHANNEL(user_data);
> >      if (be->libusb_context) {
>
> This can never be NULL, if we are not able to allocate it inside
> spice_usb_backend_new the function returns NULL.
> This should be in a preliminary patch to make this clear.

Looking a little forward, after cd-sharing is in, this could be
possible to not disable the redirection is there is no usbdk.
Even in case the libusb does not work, the client can work with
redirection enabled and redirect only emulated devices.
I would not remove this check.

>
> >          ch->backend = be;
> > -        ch->usbredirhost = usbredirhost_open_full(
> > -            be->libusb_context,
> > -            NULL,
> > -            usbredir_log,
> > -            usbredir_read_callback,
> > -            usbredir_write_callback,
> > -            usbredir_write_flush_callback,
> > -            usbredir_alloc_lock,
> > -            usbredir_lock_lock,
> > -            usbredir_unlock_lock,
> > -            usbredir_free_lock,
> > -            ch, PACKAGE_STRING,
> > -            spice_util_get_debug() ? usbredirparser_debug :
> > usbredirparser_warning,
> > -            usbredirhost_fl_write_cb_owns_buffer);
> > +        ch->usbredirhost =
> > +            usbredirhost_open_full(be->libusb_context,
> > +                                   NULL,
> > +                                   usbredir_log,
> > +                                   usbredir_read_callback,
> > +                                   usbredir_write_callback,
> > +                                   usbredir_write_flush_callback,
> > +                                   usbredir_alloc_lock,
> > +                                   usbredir_lock_lock,
> > +                                   usbredir_unlock_lock,
> > +                                   usbredir_free_lock,
> > +                                   ch, PACKAGE_STRING,
> > +                                   spice_util_get_debug() ?
> > usbredirparser_debug :
> > +                                        usbredirparser_warning,
> > +                                   usbredirhost_fl_write_cb_owns_buffer);
> >          g_warn_if_fail(ch->usbredirhost != NULL);
> >      }
> > +
> >      if (ch->usbredirhost) {
> > -        usbredirhost_set_buffered_output_size_cb(ch->usbredirhost,
> > usbredir_buffered_output_size_callback);
> > -    } else {
> > -        g_free(ch);
> > +        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) {
> > +        spice_usb_backend_channel_delete(ch);
> >          ch = NULL;
> >      }
> >
> > @@ -795,9 +1330,14 @@ SpiceUsbBackendChannel
> > *spice_usb_backend_channel_new(SpiceUsbBackend *be,
> >
> >  void spice_usb_backend_channel_flush_writes(SpiceUsbBackendChannel *ch)
> >  {
> > -    SPICE_DEBUG("%s %p, host %p", __FUNCTION__, ch, ch->usbredirhost);
> > +    SPICE_DEBUG("%s %p is up", __FUNCTION__, ch);
> >      if (ch->usbredirhost) {
> >          usbredirhost_write_guest_data(ch->usbredirhost);
> > +        initialize_parser(ch, FALSE);
> > +    } else {
> > +        initialize_parser(ch, TRUE);
>
> Why initialize_parse is called here?

This is the decision point whether parser works with hello=on or not

>
> > +        ch->parser = ch->hidden_parser;
> > +        usbredirparser_do_write(ch->parser);
> >      }
> >  }
> >
> > @@ -807,12 +1347,16 @@ void
> > spice_usb_backend_channel_delete(SpiceUsbBackendChannel *ch)
> >      if (!ch) {
> >          return;
> >      }
> > -    if (ch->usbredirhost) {
> > -        usbredirhost_close(ch->usbredirhost);
> > +    if (ch->hidden_host) {
> > +        usbredirhost_close(ch->hidden_host);
> > +    }
> > +    if (ch->hidden_parser) {
> > +        usbredirparser_destroy(ch->hidden_parser);
> >      }
> >
> >      if (ch->rules) {
> > -        g_free(ch->rules);
> > +        /* rules were allocated by usbredirparser */
> > +        free(ch->rules);
>
> Minor :I suppose this small change can go to a preliminary patch too

This code was here before, but it was never used as ch->rules was
never assigned.

Honestly I do not understand what this "preliminary patch" should
contain and what it is good for.

>
> >      }
> >
> >      g_free(ch);
>
> Frediano
> 
> On Tue, Aug 13, 2019 at 2:59 PM Frediano Ziglio <fziglio@redhat.com> wrote:
> >
> > >
> > > Redirection of emulated devices requires special approach,
> > > as usbredirhost can't be used for that (it works only with
> > > libusb devices). For emulated devices we create instance of
> > > usbredirparser that implements USB redirection protocol.
> >
> > Make sense. Actually usbredirhost internally uses a parser to
> > parse protocol messages and redirect to the physical device.
> > But the parse is internal and is used by usbredirhost so
> > would be hard to reuse.
> >
> > > In order to work with the same set of protocol capabilities
> > > that usbredirhost sets up with remote side, the parser shall:
> > > - not send its own 'hello' to the server
> > > - initialize the same capabilities that usbredirhost
> > > - receive the same 'hello' response
> > > For that we analyze 'hello' sent by USB redir parser and
> > > extract set of capabilities from it and upon receive of
> > > 'hello' response we provide it also to the parser.
> >
> > This make sense but some part of the code does not exactly that.
> > In some path the code send the 'hello' is sent by the parser.
> > It would be more clear to implement just what you described
> > above, why the implementation is a bit different.
> 
> Due to corner case described below.
> 
> >
> > > When libusb device is redirected via a channel, instance of
> > > usbredirhost serves it.
> > > When emulated device is redirected via a channel, instance
> > > of usbredirparser does the job.
> > >
> > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> > > ---
> > >  src/usb-backend.c | 616 +++++++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 580 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/src/usb-backend.c b/src/usb-backend.c
> > > index aa11c79..9aee6c7 100644
> > > --- a/src/usb-backend.c
> > > +++ b/src/usb-backend.c
> > > @@ -55,6 +55,7 @@ struct _SpiceUsbBackendDevice
> > >      gint ref_count;
> > >      SpiceUsbBackendChannel *attached_to;
> > >      UsbDeviceInformation device_info;
> > > +    gboolean edev_configured;
> >
> > This variable is never reset, I would expect that at least
> > when device is detached (this should be equivalent to physical
> > removal).
> 
> Probably, need to reset it on attach.
> In fact, nobody uses it, the driver usually just sets the configuration.
> 
> > Why not moving to SpiceUsbBackendChannel?
> >
> 
> Because it is hardly related to the channel.
> 
> > >  };
> > >
> > >  struct _SpiceUsbBackend
> > > @@ -80,10 +81,20 @@ struct _SpiceUsbBackend
> > >  struct _SpiceUsbBackendChannel
> > >  {
> > >      struct usbredirhost *usbredirhost;
> > > +    struct usbredirhost *hidden_host;
> > > +    struct usbredirparser *parser;
> > > +    struct usbredirparser *hidden_parser;
> >
> > I don't understand all these variable.
> > Won't be easier to have parser initialized for emulated
> > devices and being NULL (so freed) when emulated device
> > is detached?
> 
> When the channel is created it does not know what it will redirect.
> The same channel is used for regular devices and emulated one. Both
> parser and usbredirhost shall be internally synchronized with the
> remote side.

But this is partially true. Only the capabilities are synchronized,
the data are passed either to usbredirhost or usbredirparser, not both.

> If I'd create the parser each time the emulated device is attached I'd
> need each time to initialize it with received hello from the second
> side. We'd need to write additional code to process (even imaginable)
> error cases (what if we can't create the parser etc).

If we can't create the parser we do what we do in all other cases,
we just call abort(), not that hard.

> Finally this appears to be more complicated than have everythign ready
> from the beginning.
> 

But this does not need to be implemented with 4 pointers and some "hidden_"
prefix. Could be a simple enum stating which one to use.

> There is also corner case when the device is attached before the
> channel is ready (redirect-on-connect setting). We decide on
> usbredihost' or 'parser' depending on kind of attached device to
> redirect.

If the parser is initialized before then the usbredir host and parse
are not in sync so this means that the channel will be only supporting
emulated devices and not real ones. This seems to me a bug to be fixed.

> If the channel is still not ready, the parser/usbredirhost can't send
> anything till the channel is up. In this case switching back and forth
> between usbredirhost becomes more complicated than current solution.
> 

I don't understand this sentence. The switch is needed in any case because
you can attach an emulated device, detach and then attach a real one.
If this is not implemented we have a bug.

> >
> > >      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_disc_ack     : 1;
> >
> > The "disc" naming is confusing, also taking into account the
> > "Compact Disc" addition. Reading the code this "disc" is a shorten
> > for "disconnect", maybe would be better to just use "wait_disconnect_ack"?
> >
> 
> ok
> 
> > >      SpiceUsbBackendDevice *attached;
> > >      SpiceUsbredirChannel  *user_data;
> >
> > OT: this definitively should be renamed!
> 
> Probably in separate commit.
> 

Yes, that's what the OT means.

> >
> > >      SpiceUsbBackend *backend;
> > > @@ -355,7 +366,11 @@ gboolean
> > > spice_usb_backend_device_isoch(SpiceUsbBackendDevice *dev)
> > >      gint i, j, k;
> > >      int rc;
> > >
> > > -    g_return_val_if_fail(libdev != NULL, 0);
> > > +    g_return_val_if_fail(libdev != NULL || dev->edev != NULL, 0);
> > > +    if (dev->edev) {
> > > +        /* currently we do not emulate isoch devices */
> > > +        return FALSE;
> > > +    }
> > >
> > >      rc = libusb_get_active_config_descriptor(libdev, &conf_desc);
> > >      if (rc) {
> > > @@ -390,13 +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->usbredirhost) {
> > > -        /* just to be on the safe side */
> > > -        return;
> > > -    }
> >
> > These check should be in a preparatory patch. Nothing wrong with this
> > patch,
> > but the code (master) is confusing. usbredirhost is initialized as soon as
> > possible, can be NULL only before usbredirhost_open_full returns.
> > Actually can be NULL in this call for this reason (this callback is called
> > inside usbredirhost_open_full).
> >
> > >      if (is_channel_ready(ch->user_data)) {
> > > -        SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
> > > -        usbredirhost_write_guest_data(ch->usbredirhost);
> > > +        if (ch->usbredirhost) {
> > > +            SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
> > > +            usbredirhost_write_guest_data(ch->usbredirhost);
> > > +        } else if (ch->parser) {
> > > +            SPICE_DEBUG("%s ch %p -> parser", __FUNCTION__, ch);
> > > +            usbredirparser_do_write(ch->parser);
> > > +        } else {
> > > +            SPICE_DEBUG("%s ch %p (NOT ACTIVE)", __FUNCTION__, ch);
> >
> > This third case should never be possible. This is one aspect that master
> > code is confusing about.
> >
> > > +        }
> > >      } else {
> > >          SPICE_DEBUG("%s ch %p (not ready)", __FUNCTION__, ch);
> > >      }
> > > @@ -551,13 +569,52 @@ void
> > > spice_usb_backend_device_unref(SpiceUsbBackendDevice *dev)
> > >      }
> > >  }
> > >
> > > -int spice_usb_backend_device_check_filter(
> > > -    SpiceUsbBackendDevice *dev,
> > > -    const struct usbredirfilter_rule *rules,
> > > -    int count)
> > > +static int check_edev_device_filter(SpiceUsbBackendDevice *dev,
> > > +                                    const struct usbredirfilter_rule
> > > *rules,
> > > +                                    int count)
> > >  {
> > > -    return usbredirhost_check_device_filter(
> > > -        rules, count, dev->libusb_device, 0);
> > > +    SpiceUsbEmulatedDevice *edev = dev->edev;
> > > +    uint8_t cls[32], subcls[32], proto[32], *cfg, ifnum = 0;
> > > +    uint16_t size, offset = 0;
> > > +    if (!edev) {
> > > +        return 1;
> > > +    }
> > > +    if (!device_ops(edev)->get_descriptor(edev, LIBUSB_DT_CONFIG, 0,
> > > (void
> > > **)&cfg, &size)) {
> > > +        return 1;
> > > +    }
> > > +    while ((offset + 1) < size) {
> > > +        uint8_t len  = cfg[offset];
> > > +        uint8_t type = cfg[offset + 1];
> > > +        if ((offset + len) > size) {
> > > +            break;
> > > +        }
> > > +        if (type == LIBUSB_DT_INTERFACE) {
> > > +            cls[ifnum] = cfg[offset + 5];
> > > +            subcls[ifnum] = cfg[offset + 6];
> > > +            proto[ifnum] = cfg[offset + 7];
> > > +            ifnum++;
> > > +        }
> > > +        offset += len;
> > > +    }
> > > +
> > > +    return usbredirfilter_check(rules, count,
> > > +                                dev->device_info.class,
> > > +                                dev->device_info.subclass,
> > > +                                dev->device_info.protocol,
> > > +                                cls, subcls, proto, ifnum,
> > > +                                dev->device_info.vid,
> > > +                                dev->device_info.pid,
> > > +                                dev->device_info.bcdUSB, 0);
> > > +}
> > > +
> > > +int spice_usb_backend_device_check_filter(SpiceUsbBackendDevice *dev,
> > > +                                          const struct
> > > usbredirfilter_rule
> > > *rules, int count)
> > > +{
> > > +    if (dev->libusb_device) {
> > > +        return usbredirhost_check_device_filter(rules, count,
> > > dev->libusb_device, 0);
> > > +    } else {
> > > +        return check_edev_device_filter(dev, rules, count);
> > > +    }
> > >  }
> > >
> > >  static int usbredir_read_callback(void *user_data, uint8_t *data, int
> > >  count)
> > > @@ -621,6 +678,17 @@ 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) {
> >
> > Is confusing that the same callback is used for both usbredir parser and
> > usbredir host but the hello should be parsed only for the host.
> > Maybe disabling hello for the parser and check the type (hello) here?
> 
> 
> 
> >
> > > +        /* hello is short header (12) + hello struct (64) + capabilities
> > > (4)
> > > */
> > > +        int hello_size = sizeof(struct usb_redir_header) + sizeof(struct
> > > usb_redir_hello_header);
> >
> > This formulae is confusing with the description. sizeof(struct
> > usb_redir_header)
> > is 16 but is not the short (32bit) header.
> >
> 
> At stage of sending hello the parser (also when is works under
> usbredirhost) does not know what the second side support, so it
> uses legacy-compatible structure. If size of hello is bigger than we
> expect, something is completely not good.
> We need to find capabilities in the packet, so we need to know for
> sure where to look for them.
> 

Yes, but the comment does not match the code formulae, that is confusing,
something like

   int hello_size = 12 + sizeof(struct usb_redir_hello_header) + 4;

is surely less confusing. This is however assuming that usbredir library
won't bump USB_REDIR_CAPS_SIZE in the future. I would at least add a
compile check for this (won't fix if usbredir is updated but at least
detected at next compile).

> > > +        ch->hello_sent = 1;
> > > +        if (count == hello_size) {
> >
> > I would say "count >= hello_size" to support future extensions of the
> > capability array.
> 
> If there are capabilities that we're not aware of, we're not
> synchronized with the usbredir library.
> 

Yes, a 

   SPICE_VERIFY(USB_REDIR_CAPS_SIZE == 1);

would help.

> >
> > > +            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);
> > > +        }
> > > +    }
> > >      res = spice_usbredir_write_callback(ch->user_data, data, count);
> > >      return res;
> > >  }
> > > @@ -641,6 +709,27 @@ int
> > > spice_usb_backend_read_guest_data(SpiceUsbBackendChannel *ch, uint8_t
> > > *data,
> > >      ch->read_buf_size = count;
> > >      if (ch->usbredirhost) {
> >
> > Maybe this could be "ch->usbredirhost || !ch->hello_done_parser" to
> > parse the 'hello' always with usbredir host ?
> 
> I expect this will make the code more complicated (from previous attempts).
> 
> >
> > >          res = usbredirhost_read_guest_data(ch->usbredirhost);
> > > +        if (!ch->hello_done_parser) {
> > > +            int res_parser;
> > > +            /* usbredirhost should consume hello response */
> > > +            g_return_val_if_fail(ch->read_buf == NULL,
> > > USB_REDIR_ERROR_READ_PARSE);
> > > +
> > > +            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;
> > > +            }
> > > +        }
> > > +    } else if (ch->parser) {
> > > +        res = usbredirparser_do_read(ch->parser);
> > >      } else {
> > >          res = USB_REDIR_ERROR_IO;
> > >      }
> > > @@ -661,6 +750,11 @@ int
> > > spice_usb_backend_read_guest_data(SpiceUsbBackendChannel *ch, uint8_t
> > > *data,
> > >      }
> > >      SPICE_DEBUG("%s ch %p, %d bytes, res %d", __FUNCTION__, ch, count,
> > >      res);
> > >
> > > +    if (ch->rejected) {
> > > +        ch->rejected = 0;
> > > +        res = USB_REDIR_ERROR_DEV_REJECTED;
> > > +    }
> >
> > Why this is after the read? Won't it discard the value?
> 
> Because we need to provide the error to the usb-redir-channel.
> We know about 'reject' from our callback that is not in context of
> usb-redir-channel operation.
> 

This does not answer my question, if we return error before you are still
providing the error, just you won't discard the read.
Or is this detecting an internal saved error detected during the process
of the data we sent?

Why not using true/false for booleans?

> >
> > > +
> > >      return res;
> > >  }
> > >
> > > @@ -690,13 +784,426 @@ 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) {
> > > -        SPICE_DEBUG("%s ch %p", __FUNCTION__, ch);
> > > +        SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
> > >          usbredirhost_free_write_buffer(ch->usbredirhost, data);
> > > +    } else if (ch->parser) {
> > > +        SPICE_DEBUG("%s ch %p -> parser", __FUNCTION__, ch);
> > > +        usbredirparser_free_write_buffer(ch->parser, data);
> >
> > A bit scary that during a swicth host <-> parser this can call the wrong
> > function. But currently the implementation for both is to free 'data'.
> >
> > >      } else {
> > >          SPICE_DEBUG("%s ch %p - NOBODY TO CALL", __FUNCTION__, ch);
> > >      }
> > >  }
> > >
> > > +static void usbredir_control_packet(void *priv,
> > > +    uint64_t id, struct usb_redir_control_packet_header *h,
> > > +    uint8_t *data, int data_len)
> >
> > This style is new, I suppose copied from usbredir, but is not
> > spice-gtk.
> >
> > > +{
> > > +    SpiceUsbBackendChannel *ch = priv;
> > > +    SpiceUsbBackendDevice *d = ch->attached;
> > > +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> > > +    struct usb_redir_control_packet_header response = *h;
> > > +    uint8_t reqtype = h->requesttype & 0x7f;
> > > +    gboolean done = FALSE;
> > > +    void *out_buffer = NULL;
> > > +
> > > +    response.status = usb_redir_stall;
> > > +    SPICE_DEBUG("%s %p: TRVIL %02X %02X %04X %04X %04X",
> > > +                __FUNCTION__,
> > > +                ch, h->requesttype, h->request,
> > > +                h->value, h->index, h->length);
> > > +
> > > +    if (!edev) {
> > > +        SPICE_DEBUG("%s: device not attached", __FUNCTION__);
> > > +        response.status = usb_redir_ioerror;
> > > +        done = TRUE;
> > > +    } else if (reqtype == (LIBUSB_REQUEST_TYPE_STANDARD |
> > > LIBUSB_RECIPIENT_DEVICE) &&
> > > +               h->request == LIBUSB_REQUEST_GET_DESCRIPTOR) {
> > > +        uint16_t size;
> > > +        done = device_ops(edev)->get_descriptor(edev, h->value >> 8,
> > > h->value & 0xff,
> > > +                                                &out_buffer, &size);
> > > +        response.length = size;
> > > +        if (done) {
> > > +            response.status = 0;
> > > +        }
> > > +        done = TRUE;
> > > +    }
> > > +
> > > +    if (!done) {
> > > +        device_ops(edev)->control_request(edev, data, data_len,
> > > &response,
> > > &out_buffer);
> > > +        done = TRUE;
> > > +    }
> > > +
> > > +    if (response.status) {
> > > +        response.length = 0;
> > > +    } else if (response.length > h->length) {
> > > +        response.length = h->length;
> > > +    }
> > > +
> > > +    SPICE_DEBUG("%s responding with payload of %02X, status %X",
> > > +                __FUNCTION__, response.length, response.status);
> > > +    usbredirparser_send_control_packet(ch->parser, id, &response,
> > > +                                       response.length ? out_buffer :
> > > NULL,
> > > +                                       response.length);
> > > +
> > > +    usbredir_write_flush_callback(ch);
> > > +    usbredirparser_free_packet_data(ch->parser, data);
> > > +}
> > > +
> > > +static void usbredir_bulk_packet(void *priv,
> > > +    uint64_t id, struct usb_redir_bulk_packet_header *h,
> > > +    uint8_t *data, int data_len)
> > > +{
> > > +    SpiceUsbBackendChannel *ch = priv;
> > > +    SpiceUsbBackendDevice *d = ch->attached;
> > > +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> > > +    struct usb_redir_bulk_packet_header hout = *h;
> > > +    uint32_t len = (h->length_high << 16) | h->length;
> > > +    SPICE_DEBUG("%s %p: ep %X, len %u, id %" G_GUINT64_FORMAT,
> > > __FUNCTION__,
> > > +                ch, h->endpoint, len, id);
> > > +
> > > +    if (!edev) {
> > > +        SPICE_DEBUG("%s: device not attached", __FUNCTION__);
> > > +        hout.status = usb_redir_ioerror;
> > > +        hout.length = hout.length_high = 0;
> > > +        SPICE_DEBUG("%s: responding with ZLP status %d", __FUNCTION__,
> > > hout.status);
> > > +    } else if (h->endpoint & LIBUSB_ENDPOINT_IN) {
> > > +        if (device_ops(edev)->bulk_in_request(edev, id, &hout)) {
> > > +            usbredirparser_free_packet_data(ch->parser, data);
> > > +            /* completion is asynchronous */
> > > +            return;
> > > +        }
> > > +    } else {
> > > +        hout.status = usb_redir_stall;
> > > +        device_ops(edev)->bulk_out_request(edev, h->endpoint, data,
> > > data_len, &hout.status);
> > > +        SPICE_DEBUG("%s: responding status %d", __FUNCTION__,
> > > hout.status);
> > > +    }
> > > +
> > > +    usbredirparser_send_bulk_packet(ch->parser, id, &hout, NULL, 0);
> > > +    usbredirparser_free_packet_data(ch->parser, data);
> > > +    usbredir_write_flush_callback(ch);
> > > +}
> > > +
> > > +static void usbredir_device_reset(void *priv)
> > > +{
> > > +    SpiceUsbBackendChannel *ch = priv;
> > > +    SpiceUsbBackendDevice *d = ch->attached;
> > > +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> > > +    SPICE_DEBUG("%s ch %p", __FUNCTION__, ch);
> > > +    if (edev) {
> > > +        device_ops(edev)->reset(edev);
> > > +    }
> > > +}
> > > +
> > > +static void usbredir_interface_info(void *priv,
> > > +    struct usb_redir_interface_info_header *interface_info)
> > > +{
> > > +    SpiceUsbBackendChannel *ch = priv;
> > > +    SPICE_DEBUG("%s not implemented %p", __FUNCTION__, ch);
> > > +}
> > > +
> > > +static void usbredir_interface_ep_info(void *priv,
> > > +    struct usb_redir_ep_info_header *ep_info)
> > > +{
> > > +    SpiceUsbBackendChannel *ch = priv;
> > > +    SPICE_DEBUG("%s not implemented %p", __FUNCTION__, ch);
> > > +}
> > > +
> > > +static void usbredir_set_configuration(void *priv,
> > > +    uint64_t id, struct usb_redir_set_configuration_header
> > > *set_configuration)
> > > +{
> > > +    SpiceUsbBackendChannel *ch = priv;
> > > +    struct usb_redir_configuration_status_header h;
> > > +    h.status = 0;
> > > +    h.configuration = set_configuration->configuration;
> > > +    SPICE_DEBUG("%s ch %p, cfg %d", __FUNCTION__, ch, h.configuration);
> > > +    if (ch->attached) {
> > > +        ch->attached->edev_configured = h.configuration != 0;
> > > +    }
> > > +    usbredirparser_send_configuration_status(ch->parser, id, &h);
> > > +    usbredir_write_flush_callback(ch);
> > > +}
> > > +
> > > +static void usbredir_get_configuration(void *priv, uint64_t id)
> > > +{
> > > +    SpiceUsbBackendChannel *ch = priv;
> > > +    struct usb_redir_configuration_status_header h;
> > > +    h.status = 0;
> > > +    h.configuration = ch->attached && ch->attached->edev_configured;
> > > +    SPICE_DEBUG("%s ch %p, cfg %d", __FUNCTION__, ch, h.configuration);
> > > +    usbredirparser_send_configuration_status(ch->parser, id, &h);
> > > +    usbredir_write_flush_callback(ch);
> > > +}
> > > +
> > > +static void usbredir_set_alt_setting(void *priv,
> > > +    uint64_t id, struct usb_redir_set_alt_setting_header *s)
> > > +{
> > > +    SpiceUsbBackendChannel *ch = priv;
> > > +    struct usb_redir_alt_setting_status_header sh;
> > > +    sh.status = (!s->interface && !s->alt) ? 0 : usb_redir_stall;
> > > +    sh.interface = s->interface;
> > > +    sh.alt = s->alt;
> > > +    SPICE_DEBUG("%s ch %p, %d:%d", __FUNCTION__, ch, s->interface,
> > > s->alt);
> > > +    usbredirparser_send_alt_setting_status(ch->parser, id, &sh);
> > > +    usbredir_write_flush_callback(ch);
> > > +}
> > > +
> > > +static void usbredir_get_alt_setting(void *priv,
> > > +    uint64_t id, struct usb_redir_get_alt_setting_header *s)
> > > +{
> > > +    SpiceUsbBackendChannel *ch = priv;
> > > +    struct usb_redir_alt_setting_status_header sh;
> > > +    sh.status = (s->interface == 0) ? 0 : usb_redir_stall;
> > > +    sh.interface = s->interface;
> > > +    sh.alt = 0;
> > > +    SPICE_DEBUG("%s ch %p, if %d", __FUNCTION__, ch, s->interface);
> > > +    usbredirparser_send_alt_setting_status(ch->parser, id, &sh);
> > > +    usbredir_write_flush_callback(ch);
> > > +}
> > > +
> > > +static void usbredir_cancel_data(void *priv, uint64_t id)
> > > +{
> > > +    SpiceUsbBackendChannel *ch = priv;
> > > +    SpiceUsbBackendDevice *d = ch->attached;
> > > +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> > > +    if (!edev) {
> > > +        SPICE_DEBUG("%s: device not attached", __FUNCTION__);
> > > +        return;
> > > +    }
> > > +    device_ops(edev)->cancel_request(edev, id);
> > > +}
> > > +
> > > +static void usbredir_filter_reject(void *priv)
> > > +{
> > > +    SpiceUsbBackendChannel *ch = priv;
> > > +    SPICE_DEBUG("%s %p", __FUNCTION__, ch);
> > > +    ch->rejected = 1;
> > > +}
> > > +
> > > +/* Note that the ownership of the rules array is passed on to the
> > > callback.
> > > */
> > > +static void usbredir_filter_filter(void *priv,
> > > +    struct usbredirfilter_rule *r, int count)
> > > +{
> > > +    SpiceUsbBackendChannel *ch = priv;
> > > +    SPICE_DEBUG("%s ch %p %d filters", __FUNCTION__, ch, count);
> > > +
> > > +    free(ch->rules);
> > > +
> > > +    ch->rules = r;
> > > +    ch->rules_count = count;
> > > +    if (count) {
> > > +        int i;
> > > +        for (i = 0; i < count; i++) {
> > > +            SPICE_DEBUG("%s class %d, %X:%X",
> > > +                r[i].allow ? "allowed" : "denied", r[i].device_class,
> > > +                (uint32_t)r[i].vendor_id, (uint32_t)r[i].product_id);
> > > +        }
> > > +    }
> > > +}
> > > +
> > > +static void usbredir_device_disconnect_ack(void *priv)
> > > +{
> > > +    SpiceUsbBackendChannel *ch = priv;
> > > +    SPICE_DEBUG("%s ch %p", __FUNCTION__, ch);
> > > +    if (ch->parser && ch->wait_disc_ack) {
> > > +        ch->parser = NULL;
> > > +        SPICE_DEBUG("%s switch to usbredirhost", __FUNCTION__);
> > > +        ch->usbredirhost = ch->hidden_host;
> > > +    }
> > > +    ch->wait_disc_ack = 0;
> > > +}
> > > +
> > > +static void usbredir_hello(void *priv,
> > > +    struct usb_redir_hello_header *hello)
> > > +{
> > > +    SpiceUsbBackendChannel *ch = priv;
> > > +    SpiceUsbBackendDevice *d = ch->attached;
> > > +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> > > +    struct usb_redir_device_connect_header device_connect;
> > > +    struct usb_redir_ep_info_header ep_info = { 0 };
> > > +    struct usb_redir_interface_info_header interface_info = { 0 };
> > > +    uint8_t *cfg;
> > > +    uint16_t size, offset = 0;
> > > +    SPICE_DEBUG("%s %p %sattached %s", __FUNCTION__, ch,
> > > +        edev ? "" : "not ",  hello ? "" : "(internal)");
> > > +
> > > +    if (hello) {
> > > +        ch->hello_done_parser = 1;
> > > +    }
> > > +    if (!edev) {
> > > +        return;
> > > +    }
> > > +    if (!device_ops(edev)->get_descriptor(edev, LIBUSB_DT_CONFIG, 0,
> > > (void
> > > **)&cfg, &size)) {
> > > +        return;
> > > +    }
> > > +    while ((offset + 1) < size) {
> > > +        uint8_t len  = cfg[offset];
> > > +        uint8_t type = cfg[offset + 1];
> > > +        if ((offset + len) > size) {
> > > +            break;
> > > +        }
> > > +        if (type == LIBUSB_DT_INTERFACE) {
> > > +            uint32_t i = interface_info.interface_count;
> > > +            uint8_t class, subclass, protocol;
> > > +            class = cfg[offset + 5];
> > > +            subclass = cfg[offset + 6];
> > > +            protocol = cfg[offset + 7];
> > > +            interface_info.interface_class[i] = class;
> > > +            interface_info.interface_subclass[i] = subclass;
> > > +            interface_info.interface_protocol[i] = protocol;
> > > +            interface_info.interface_count++;
> > > +            SPICE_DEBUG("%s IF%d: %d/%d/%d", __FUNCTION__, i, class,
> > > subclass, protocol);
> > > +        } else if (type == LIBUSB_DT_ENDPOINT) {
> > > +            uint8_t address = cfg[offset + 2];
> > > +            uint16_t max_packet_size = 0x100 * cfg[offset + 5] +
> > > cfg[offset
> > > + 4];
> > > +            uint8_t index = address & 0xf;
> > > +            if (address & 0x80) index += 0x10;
> > > +            ep_info.type[index] = cfg[offset + 3] & 0x3;
> > > +            ep_info.max_packet_size[index] = max_packet_size;
> > > +            SPICE_DEBUG("%s EP[%02X]: %d/%d", __FUNCTION__, index,
> > > ep_info.type[index], max_packet_size);
> > > +        }
> > > +        offset += len;
> > > +    }
> > > +
> > > +    usbredirparser_send_interface_info(ch->parser, &interface_info);
> > > +    usbredirparser_send_ep_info(ch->parser, &ep_info);
> > > +
> > > +    device_connect.device_class = 0; //d->device_info.class;
> > > +    device_connect.device_subclass = 0; //d->device_info.subclass;
> > > +    device_connect.device_protocol = 0; //d->device_info.protocol;;
> > > +    device_connect.vendor_id = d->device_info.vid;
> > > +    device_connect.product_id = d->device_info.pid;
> > > +    device_connect.device_version_bcd = d->device_info.bcdUSB;
> > > +    device_connect.speed = usb_redir_speed_high;
> > > +    usbredirparser_send_device_connect(ch->parser, &device_connect);
> > > +    usbredir_write_flush_callback(ch);
> > > +}
> > > +
> > > +static void initialize_parser(SpiceUsbBackendChannel *ch, gboolean
> > > do_hello)
> > > +{
> > > +    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;
> > > +    }
> > > +
> > > +    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;
> >
> > In which case you need to do the hello in the parser instead of the host?
> > This could cause issues switching from parser back to host in case you
> > remove an emulated device and you add a real one.
> 
> The case is described above. From my point of view, having both
> (usbredirhost and parser) ready and switch
> according to the current need is the simplest way to do the job.
> 
> >
> > > +        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_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);
> > > +}
> > > +
> > > +/*
> > > +    We can initialize the usbredirparser with HELLO enabled only in case
> > > +    the libusb is not active and the usbredirhost does not function.
> > > +    Then the parser sends session HELLO and receives server's response.
> > > +    Otherwise (usbredirparser initialized with HELLO disabled):
> > > +    - the usbredirhost sends session HELLO
> > > +    - we look into it to know set of capabilities we shall initialize
> > > +      the parser with
> > > +    - when we receive server's response to HELLO we provide it also to
> > > +      parser to let it synchronize with server's capabilities
> > > +*/
> > > +static struct usbredirparser *create_parser(SpiceUsbBackendChannel *ch)
> > > +{
> > > +    struct usbredirparser *parser = usbredirparser_create();
> > > +
> > > +    g_return_val_if_fail(parser != NULL, NULL);
> > > +
> > > +    parser->priv = ch;
> > > +    parser->log_func = usbredir_log;
> > > +    parser->read_func = usbredir_read_callback;
> > > +    parser->write_func = usbredir_write_callback;
> > > +    parser->reset_func = usbredir_device_reset;
> > > +    parser->set_configuration_func = usbredir_set_configuration;
> > > +    parser->get_configuration_func = usbredir_get_configuration;
> > > +    parser->set_alt_setting_func = usbredir_set_alt_setting;
> > > +    parser->get_alt_setting_func = usbredir_get_alt_setting;
> > > +    parser->cancel_data_packet_func = usbredir_cancel_data;
> > > +    parser->control_packet_func = usbredir_control_packet;
> > > +    parser->bulk_packet_func = usbredir_bulk_packet;
> > > +    parser->alloc_lock_func = usbredir_alloc_lock;
> > > +    parser->lock_func = usbredir_lock_lock;
> > > +    parser->unlock_func = usbredir_unlock_lock;
> > > +    parser->free_lock_func = usbredir_free_lock;
> > > +    parser->hello_func = usbredir_hello;
> > > +    parser->filter_reject_func = usbredir_filter_reject;
> > > +    parser->device_disconnect_ack_func = usbredir_device_disconnect_ack;
> > > +    parser->interface_info_func = usbredir_interface_info;
> > > +    parser->ep_info_func = usbredir_interface_ep_info;
> > > +    parser->filter_filter_func = usbredir_filter_filter;
> > > +
> > > +    return parser;
> > > +}
> > > +
> > > +static gboolean attach_edev(SpiceUsbBackendChannel *ch,
> > > +                            SpiceUsbBackendDevice *dev,
> > > +                            GError **error)
> > > +{
> > > +    if (!dev->edev) {
> > > +        g_set_error(error, SPICE_CLIENT_ERROR,
> > > SPICE_CLIENT_ERROR_FAILED,
> > > +           _("Failed to redirect device %d"), 1);
> >
> > Minor: I would say more an assert level error, the device should be real or
> > emulated.
> >
> > > +        return FALSE;
> > > +    }
> > > +    if (ch->usbredirhost && !ch->hello_done_parser) {
> > > +        /*
> > > +            we can't initialize parser until we see hello from usbredir
> > > +            and the parser can't work until it sees the hello response.
> > > +            this is case of autoconnect when emulated device is attached
> > > +            before the channel is up
> > > +        */
> > > +        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->wait_disc_ack = 0;
> > > +    ch->attached = dev;
> > > +    dev->attached_to = ch;
> > > +    device_ops(dev->edev)->attach(dev->edev, ch->hidden_parser);
> > > +    if (ch->hello_done_parser) {
> > > +        /* send device info */
> > > +        usbredir_hello(ch, NULL);
> > > +    }
> > > +    return TRUE;
> > > +}
> > > +
> > >  gboolean spice_usb_backend_channel_attach(SpiceUsbBackendChannel *ch,
> > >                                            SpiceUsbBackendDevice *dev,
> > >                                            GError **error)
> > > @@ -706,7 +1213,13 @@ gboolean
> > > spice_usb_backend_channel_attach(SpiceUsbBackendChannel *ch,
> > >
> > >      g_return_val_if_fail(dev != NULL, FALSE);
> > >
> > > +    if (!dev->libusb_device) {
> > > +        return attach_edev(ch, dev, error);
> > > +    }
> > > +
> > >      libusb_device_handle *handle = NULL;
> > > +    ch->usbredirhost = ch->hidden_host;
> > > +    ch->parser = NULL;
> > >
> > >      /*
> > >         Under Windows we need to avoid updating
> > > @@ -740,18 +1253,33 @@ gboolean
> > > spice_usb_backend_channel_attach(SpiceUsbBackendChannel *ch,
> > >
> > >  void spice_usb_backend_channel_detach(SpiceUsbBackendChannel *ch)
> > >  {
> > > +    SpiceUsbBackendDevice *d = ch->attached;
> > > +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> > >      SPICE_DEBUG("%s >> ch %p, was attached %p", __FUNCTION__, ch,
> > >      ch->attached);
> > > -    if (!ch->attached) {
> > > +    if (!d) {
> > >          SPICE_DEBUG("%s: nothing to detach", __FUNCTION__);
> > >          return;
> > >      }
> > >      if (ch->usbredirhost) {
> > >          /* it will call libusb_close internally */
> > >          usbredirhost_set_device(ch->usbredirhost, NULL);
> > > +    } else if (ch->parser) {
> > > +        if (edev) {
> > > +            device_ops(edev)->detach(edev);
> > > +        }
> > > +        usbredirparser_send_device_disconnect(ch->parser);
> > > +        usbredir_write_flush_callback(ch);
> > > +        ch->wait_disc_ack = usbredirparser_peer_has_cap(ch->parser,
> > > +
> > > usb_redir_cap_device_disconnect_ack);
> > > +        if (!ch->wait_disc_ack) {
> > > +            ch->usbredirhost = ch->hidden_host;
> > > +            ch->parser = NULL;
> > > +        }
> > >      }
> > >      SPICE_DEBUG("%s ch %p, detach done", __FUNCTION__, ch);
> > >      ch->attached->attached_to = NULL;
> > >      ch->attached = NULL;
> > > +    ch->rejected = 0;
> > >  }
> > >
> > >  SpiceUsbBackendChannel *spice_usb_backend_channel_new(SpiceUsbBackend
> > >  *be,
> > > @@ -766,26 +1294,33 @@ SpiceUsbBackendChannel
> > > *spice_usb_backend_channel_new(SpiceUsbBackend *be,
> > >      ch->user_data = SPICE_USBREDIR_CHANNEL(user_data);
> > >      if (be->libusb_context) {
> >
> > This can never be NULL, if we are not able to allocate it inside
> > spice_usb_backend_new the function returns NULL.
> > This should be in a preliminary patch to make this clear.
> 
> Looking a little forward, after cd-sharing is in, this could be
> possible to not disable the redirection is there is no usbdk.
> Even in case the libusb does not work, the client can work with
> redirection enabled and redirect only emulated devices.
> I would not remove this check.
> 

The problem is that current code has some part with one logic
(usb context is always available) and some other part of code
with the opposite logic (usb context is not required).

libusb works also without usbdk installed and provides a
valid (not NULL) usb context.

> >
> > >          ch->backend = be;
> > > -        ch->usbredirhost = usbredirhost_open_full(
> > > -            be->libusb_context,
> > > -            NULL,
> > > -            usbredir_log,
> > > -            usbredir_read_callback,
> > > -            usbredir_write_callback,
> > > -            usbredir_write_flush_callback,
> > > -            usbredir_alloc_lock,
> > > -            usbredir_lock_lock,
> > > -            usbredir_unlock_lock,
> > > -            usbredir_free_lock,
> > > -            ch, PACKAGE_STRING,
> > > -            spice_util_get_debug() ? usbredirparser_debug :
> > > usbredirparser_warning,
> > > -            usbredirhost_fl_write_cb_owns_buffer);
> > > +        ch->usbredirhost =
> > > +            usbredirhost_open_full(be->libusb_context,
> > > +                                   NULL,
> > > +                                   usbredir_log,
> > > +                                   usbredir_read_callback,
> > > +                                   usbredir_write_callback,
> > > +                                   usbredir_write_flush_callback,
> > > +                                   usbredir_alloc_lock,
> > > +                                   usbredir_lock_lock,
> > > +                                   usbredir_unlock_lock,
> > > +                                   usbredir_free_lock,
> > > +                                   ch, PACKAGE_STRING,
> > > +                                   spice_util_get_debug() ?
> > > usbredirparser_debug :
> > > +                                        usbredirparser_warning,
> > > +
> > > usbredirhost_fl_write_cb_owns_buffer);
> > >          g_warn_if_fail(ch->usbredirhost != NULL);
> > >      }
> > > +
> > >      if (ch->usbredirhost) {
> > > -        usbredirhost_set_buffered_output_size_cb(ch->usbredirhost,
> > > usbredir_buffered_output_size_callback);
> > > -    } else {
> > > -        g_free(ch);
> > > +        ch->hidden_parser = create_parser(ch);
> > > +        ch->hidden_host = ch->usbredirhost;
> > > +        usbredirhost_set_buffered_output_size_cb(ch->usbredirhost,
> > > +
> > > usbredir_buffered_output_size_callback);

Here you are assuming that usb context is not NULL which does not
match with your statement above.

> > > +    }
> > > +
> > > +    if (!ch->hidden_parser) {
> > > +        spice_usb_backend_channel_delete(ch);
> > >          ch = NULL;
> > >      }
> > >
> > > @@ -795,9 +1330,14 @@ SpiceUsbBackendChannel
> > > *spice_usb_backend_channel_new(SpiceUsbBackend *be,
> > >
> > >  void spice_usb_backend_channel_flush_writes(SpiceUsbBackendChannel *ch)
> > >  {
> > > -    SPICE_DEBUG("%s %p, host %p", __FUNCTION__, ch, ch->usbredirhost);
> > > +    SPICE_DEBUG("%s %p is up", __FUNCTION__, ch);
> > >      if (ch->usbredirhost) {
> > >          usbredirhost_write_guest_data(ch->usbredirhost);
> > > +        initialize_parser(ch, FALSE);
> > > +    } else {
> > > +        initialize_parser(ch, TRUE);
> >
> > Why initialize_parse is called here?
> 
> This is the decision point whether parser works with hello=on or not
> 

And so if this channel support only emulated devices which is a
regression.

> >
> > > +        ch->parser = ch->hidden_parser;
> > > +        usbredirparser_do_write(ch->parser);
> > >      }
> > >  }
> > >
> > > @@ -807,12 +1347,16 @@ void
> > > spice_usb_backend_channel_delete(SpiceUsbBackendChannel *ch)
> > >      if (!ch) {
> > >          return;
> > >      }
> > > -    if (ch->usbredirhost) {
> > > -        usbredirhost_close(ch->usbredirhost);
> > > +    if (ch->hidden_host) {
> > > +        usbredirhost_close(ch->hidden_host);
> > > +    }
> > > +    if (ch->hidden_parser) {
> > > +        usbredirparser_destroy(ch->hidden_parser);
> > >      }
> > >
> > >      if (ch->rules) {
> > > -        g_free(ch->rules);
> > > +        /* rules were allocated by usbredirparser */
> > > +        free(ch->rules);
> >
> > Minor :I suppose this small change can go to a preliminary patch too
> 
> This code was here before, but it was never used as ch->rules was
> never assigned.
> 

yes

> Honestly I do not understand what this "preliminary patch" should
> contain and what it is good for.
> 

I don't think is a good idea to add code with multiple logic for
the same condition. Currently most of the code assume usb context
is valid, I would go in a single direction, not adding additional
code with 2 different login about it.

> >
> > >      }
> > >
> > >      g_free(ch);
> >
> > Frediano
>
On Thu, Aug 22, 2019 at 9:56 AM Frediano Ziglio <fziglio@redhat.com> wrote:
>
> >
> > On Tue, Aug 13, 2019 at 2:59 PM Frediano Ziglio <fziglio@redhat.com> wrote:
> > >
> > > >
> > > > Redirection of emulated devices requires special approach,
> > > > as usbredirhost can't be used for that (it works only with
> > > > libusb devices). For emulated devices we create instance of
> > > > usbredirparser that implements USB redirection protocol.
> > >
> > > Make sense. Actually usbredirhost internally uses a parser to
> > > parse protocol messages and redirect to the physical device.
> > > But the parse is internal and is used by usbredirhost so
> > > would be hard to reuse.
> > >
> > > > In order to work with the same set of protocol capabilities
> > > > that usbredirhost sets up with remote side, the parser shall:
> > > > - not send its own 'hello' to the server
> > > > - initialize the same capabilities that usbredirhost
> > > > - receive the same 'hello' response
> > > > For that we analyze 'hello' sent by USB redir parser and
> > > > extract set of capabilities from it and upon receive of
> > > > 'hello' response we provide it also to the parser.
> > >
> > > This make sense but some part of the code does not exactly that.
> > > In some path the code send the 'hello' is sent by the parser.
> > > It would be more clear to implement just what you described
> > > above, why the implementation is a bit different.
> >
> > Due to corner case described below.
> >
> > >
> > > > When libusb device is redirected via a channel, instance of
> > > > usbredirhost serves it.
> > > > When emulated device is redirected via a channel, instance
> > > > of usbredirparser does the job.
> > > >
> > > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> > > > ---
> > > >  src/usb-backend.c | 616 +++++++++++++++++++++++++++++++++++++++++++---
> > > >  1 file changed, 580 insertions(+), 36 deletions(-)
> > > >
> > > > diff --git a/src/usb-backend.c b/src/usb-backend.c
> > > > index aa11c79..9aee6c7 100644
> > > > --- a/src/usb-backend.c
> > > > +++ b/src/usb-backend.c
> > > > @@ -55,6 +55,7 @@ struct _SpiceUsbBackendDevice
> > > >      gint ref_count;
> > > >      SpiceUsbBackendChannel *attached_to;
> > > >      UsbDeviceInformation device_info;
> > > > +    gboolean edev_configured;
> > >
> > > This variable is never reset, I would expect that at least
> > > when device is detached (this should be equivalent to physical
> > > removal).
> >
> > Probably, need to reset it on attach.
> > In fact, nobody uses it, the driver usually just sets the configuration.
> >
> > > Why not moving to SpiceUsbBackendChannel?
> > >
> >
> > Because it is hardly related to the channel.
> >
> > > >  };
> > > >
> > > >  struct _SpiceUsbBackend
> > > > @@ -80,10 +81,20 @@ struct _SpiceUsbBackend
> > > >  struct _SpiceUsbBackendChannel
> > > >  {
> > > >      struct usbredirhost *usbredirhost;
> > > > +    struct usbredirhost *hidden_host;
> > > > +    struct usbredirparser *parser;
> > > > +    struct usbredirparser *hidden_parser;
> > >
> > > I don't understand all these variable.
> > > Won't be easier to have parser initialized for emulated
> > > devices and being NULL (so freed) when emulated device
> > > is detached?
> >
> > When the channel is created it does not know what it will redirect.
> > The same channel is used for regular devices and emulated one. Both
> > parser and usbredirhost shall be internally synchronized with the
> > remote side.
>
> But this is partially true. Only the capabilities are synchronized,
> the data are passed either to usbredirhost or usbredirparser, not both.

Final initialization of actual capabilities happens when
parser/usbredirhost receives hello response from the remote side.
Both receive the response at proper time.

>
> > If I'd create the parser each time the emulated device is attached I'd
> > need each time to initialize it with received hello from the second
> > side. We'd need to write additional code to process (even imaginable)
> > error cases (what if we can't create the parser etc).
>
> If we can't create the parser we do what we do in all other cases,
> we just call abort(), not that hard.
>
> > Finally this appears to be more complicated than have everythign ready
> > from the beginning.
> >
>
> But this does not need to be implemented with 4 pointers and some "hidden_"
> prefix. Could be a simple enum stating which one to use.

There are may ways to implement it, I've selected simple one.
I still think it is better to have everything ready from the
beginning, this makes the code more consistent.
In the past I've tried different approaches to address all possible
flows and this one looks optimal for me.

>
> > There is also corner case when the device is attached before the
> > channel is ready (redirect-on-connect setting). We decide on
> > usbredihost' or 'parser' depending on kind of attached device to
> > redirect.
>
> If the parser is initialized before then the usbredir host and parse
> are not in sync so this means that the channel will be only supporting
> emulated devices and not real ones. This seems to me a bug to be fixed.

There is no bug, and the channel is able to support both types of
devices at proper time.

>
> > If the channel is still not ready, the parser/usbredirhost can't send
> > anything till the channel is up. In this case switching back and forth
> > between usbredirhost becomes more complicated than current solution.
> >
>
> I don't understand this sentence. The switch is needed in any case because
> you can attach an emulated device, detach and then attach a real one.
> If this is not implemented we have a bug.

Of course this is implemented.
The question is when exactly we do the switch. Currently the switch is
done at time of attach, this is simpler.

>
> > >
> > > >      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_disc_ack     : 1;
> > >
> > > The "disc" naming is confusing, also taking into account the
> > > "Compact Disc" addition. Reading the code this "disc" is a shorten
> > > for "disconnect", maybe would be better to just use "wait_disconnect_ack"?
> > >
> >
> > ok
> >
> > > >      SpiceUsbBackendDevice *attached;
> > > >      SpiceUsbredirChannel  *user_data;
> > >
> > > OT: this definitively should be renamed!
> >
> > Probably in separate commit.
> >
>
> Yes, that's what the OT means.
>
> > >
> > > >      SpiceUsbBackend *backend;
> > > > @@ -355,7 +366,11 @@ gboolean
> > > > spice_usb_backend_device_isoch(SpiceUsbBackendDevice *dev)
> > > >      gint i, j, k;
> > > >      int rc;
> > > >
> > > > -    g_return_val_if_fail(libdev != NULL, 0);
> > > > +    g_return_val_if_fail(libdev != NULL || dev->edev != NULL, 0);
> > > > +    if (dev->edev) {
> > > > +        /* currently we do not emulate isoch devices */
> > > > +        return FALSE;
> > > > +    }
> > > >
> > > >      rc = libusb_get_active_config_descriptor(libdev, &conf_desc);
> > > >      if (rc) {
> > > > @@ -390,13 +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->usbredirhost) {
> > > > -        /* just to be on the safe side */
> > > > -        return;
> > > > -    }
> > >
> > > These check should be in a preparatory patch. Nothing wrong with this
> > > patch,
> > > but the code (master) is confusing. usbredirhost is initialized as soon as
> > > possible, can be NULL only before usbredirhost_open_full returns.
> > > Actually can be NULL in this call for this reason (this callback is called
> > > inside usbredirhost_open_full).
> > >
> > > >      if (is_channel_ready(ch->user_data)) {
> > > > -        SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
> > > > -        usbredirhost_write_guest_data(ch->usbredirhost);
> > > > +        if (ch->usbredirhost) {
> > > > +            SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
> > > > +            usbredirhost_write_guest_data(ch->usbredirhost);
> > > > +        } else if (ch->parser) {
> > > > +            SPICE_DEBUG("%s ch %p -> parser", __FUNCTION__, ch);
> > > > +            usbredirparser_do_write(ch->parser);
> > > > +        } else {
> > > > +            SPICE_DEBUG("%s ch %p (NOT ACTIVE)", __FUNCTION__, ch);
> > >
> > > This third case should never be possible. This is one aspect that master
> > > code is confusing about.
> > >
> > > > +        }
> > > >      } else {
> > > >          SPICE_DEBUG("%s ch %p (not ready)", __FUNCTION__, ch);
> > > >      }
> > > > @@ -551,13 +569,52 @@ void
> > > > spice_usb_backend_device_unref(SpiceUsbBackendDevice *dev)
> > > >      }
> > > >  }
> > > >
> > > > -int spice_usb_backend_device_check_filter(
> > > > -    SpiceUsbBackendDevice *dev,
> > > > -    const struct usbredirfilter_rule *rules,
> > > > -    int count)
> > > > +static int check_edev_device_filter(SpiceUsbBackendDevice *dev,
> > > > +                                    const struct usbredirfilter_rule
> > > > *rules,
> > > > +                                    int count)
> > > >  {
> > > > -    return usbredirhost_check_device_filter(
> > > > -        rules, count, dev->libusb_device, 0);
> > > > +    SpiceUsbEmulatedDevice *edev = dev->edev;
> > > > +    uint8_t cls[32], subcls[32], proto[32], *cfg, ifnum = 0;
> > > > +    uint16_t size, offset = 0;
> > > > +    if (!edev) {
> > > > +        return 1;
> > > > +    }
> > > > +    if (!device_ops(edev)->get_descriptor(edev, LIBUSB_DT_CONFIG, 0,
> > > > (void
> > > > **)&cfg, &size)) {
> > > > +        return 1;
> > > > +    }
> > > > +    while ((offset + 1) < size) {
> > > > +        uint8_t len  = cfg[offset];
> > > > +        uint8_t type = cfg[offset + 1];
> > > > +        if ((offset + len) > size) {
> > > > +            break;
> > > > +        }
> > > > +        if (type == LIBUSB_DT_INTERFACE) {
> > > > +            cls[ifnum] = cfg[offset + 5];
> > > > +            subcls[ifnum] = cfg[offset + 6];
> > > > +            proto[ifnum] = cfg[offset + 7];
> > > > +            ifnum++;
> > > > +        }
> > > > +        offset += len;
> > > > +    }
> > > > +
> > > > +    return usbredirfilter_check(rules, count,
> > > > +                                dev->device_info.class,
> > > > +                                dev->device_info.subclass,
> > > > +                                dev->device_info.protocol,
> > > > +                                cls, subcls, proto, ifnum,
> > > > +                                dev->device_info.vid,
> > > > +                                dev->device_info.pid,
> > > > +                                dev->device_info.bcdUSB, 0);
> > > > +}
> > > > +
> > > > +int spice_usb_backend_device_check_filter(SpiceUsbBackendDevice *dev,
> > > > +                                          const struct
> > > > usbredirfilter_rule
> > > > *rules, int count)
> > > > +{
> > > > +    if (dev->libusb_device) {
> > > > +        return usbredirhost_check_device_filter(rules, count,
> > > > dev->libusb_device, 0);
> > > > +    } else {
> > > > +        return check_edev_device_filter(dev, rules, count);
> > > > +    }
> > > >  }
> > > >
> > > >  static int usbredir_read_callback(void *user_data, uint8_t *data, int
> > > >  count)
> > > > @@ -621,6 +678,17 @@ 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) {
> > >
> > > Is confusing that the same callback is used for both usbredir parser and
> > > usbredir host but the hello should be parsed only for the host.
> > > Maybe disabling hello for the parser and check the type (hello) here?
> >
> >
> >
> > >
> > > > +        /* hello is short header (12) + hello struct (64) + capabilities
> > > > (4)
> > > > */
> > > > +        int hello_size = sizeof(struct usb_redir_header) + sizeof(struct
> > > > usb_redir_hello_header);
> > >
> > > This formulae is confusing with the description. sizeof(struct
> > > usb_redir_header)
> > > is 16 but is not the short (32bit) header.
> > >
> >
> > At stage of sending hello the parser (also when is works under
> > usbredirhost) does not know what the second side support, so it
> > uses legacy-compatible structure. If size of hello is bigger than we
> > expect, something is completely not good.
> > We need to find capabilities in the packet, so we need to know for
> > sure where to look for them.
> >
>
> Yes, but the comment does not match the code formulae, that is confusing,
> something like
>
>    int hello_size = 12 + sizeof(struct usb_redir_hello_header) + 4;
>
> is surely less confusing. This is however assuming that usbredir library
> won't bump USB_REDIR_CAPS_SIZE in the future. I would at least add a
> compile check for this (won't fix if usbredir is updated but at least
> detected at next compile).
>
> > > > +        ch->hello_sent = 1;
> > > > +        if (count == hello_size) {
> > >
> > > I would say "count >= hello_size" to support future extensions of the
> > > capability array.
> >
> > If there are capabilities that we're not aware of, we're not
> > synchronized with the usbredir library.
> >
>
> Yes, a
>
>    SPICE_VERIFY(USB_REDIR_CAPS_SIZE == 1);
>
> would help.
>
> > >
> > > > +            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);
> > > > +        }
> > > > +    }
> > > >      res = spice_usbredir_write_callback(ch->user_data, data, count);
> > > >      return res;
> > > >  }
> > > > @@ -641,6 +709,27 @@ int
> > > > spice_usb_backend_read_guest_data(SpiceUsbBackendChannel *ch, uint8_t
> > > > *data,
> > > >      ch->read_buf_size = count;
> > > >      if (ch->usbredirhost) {
> > >
> > > Maybe this could be "ch->usbredirhost || !ch->hello_done_parser" to
> > > parse the 'hello' always with usbredir host ?
> >
> > I expect this will make the code more complicated (from previous attempts).
> >
> > >
> > > >          res = usbredirhost_read_guest_data(ch->usbredirhost);
> > > > +        if (!ch->hello_done_parser) {
> > > > +            int res_parser;
> > > > +            /* usbredirhost should consume hello response */
> > > > +            g_return_val_if_fail(ch->read_buf == NULL,
> > > > USB_REDIR_ERROR_READ_PARSE);
> > > > +
> > > > +            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;
> > > > +            }
> > > > +        }
> > > > +    } else if (ch->parser) {
> > > > +        res = usbredirparser_do_read(ch->parser);
> > > >      } else {
> > > >          res = USB_REDIR_ERROR_IO;
> > > >      }
> > > > @@ -661,6 +750,11 @@ int
> > > > spice_usb_backend_read_guest_data(SpiceUsbBackendChannel *ch, uint8_t
> > > > *data,
> > > >      }
> > > >      SPICE_DEBUG("%s ch %p, %d bytes, res %d", __FUNCTION__, ch, count,
> > > >      res);
> > > >
> > > > +    if (ch->rejected) {
> > > > +        ch->rejected = 0;
> > > > +        res = USB_REDIR_ERROR_DEV_REJECTED;
> > > > +    }
> > >
> > > Why this is after the read? Won't it discard the value?
> >
> > Because we need to provide the error to the usb-redir-channel.
> > We know about 'reject' from our callback that is not in context of
> > usb-redir-channel operation.
> >
>
> This does not answer my question, if we return error before you are still
> providing the error, just you won't discard the read.

We do not discard here anything. This procedure saves data buffer for
further read operation and issues request to parser/usbredirhost to
read the data.
It returns error or OK to usb-redir-channel and in case of error the
usbredir-channel will spawn error handler that will initiate device
disconnection and indicate the error to the application.

> Or is this detecting an internal saved error detected during the process
> of the data we sent?

We do not have other path to return error to the usbredir-channel.
This procedure returns error to it In both cases (the error is
returned by parser/ubredir or the error is saved internally).

> Why not using true/false for booleans?

I do not see here any boolean except of 'rejected' flag.
Why boolean is better than flag??

>
> > >
> > > > +
> > > >      return res;
> > > >  }
> > > >
> > > > @@ -690,13 +784,426 @@ 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) {
> > > > -        SPICE_DEBUG("%s ch %p", __FUNCTION__, ch);
> > > > +        SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
> > > >          usbredirhost_free_write_buffer(ch->usbredirhost, data);
> > > > +    } else if (ch->parser) {
> > > > +        SPICE_DEBUG("%s ch %p -> parser", __FUNCTION__, ch);
> > > > +        usbredirparser_free_write_buffer(ch->parser, data);
> > >
> > > A bit scary that during a swicth host <-> parser this can call the wrong
> > > function. But currently the implementation for both is to free 'data'.
> > >
> > > >      } else {
> > > >          SPICE_DEBUG("%s ch %p - NOBODY TO CALL", __FUNCTION__, ch);
> > > >      }
> > > >  }
> > > >
> > > > +static void usbredir_control_packet(void *priv,
> > > > +    uint64_t id, struct usb_redir_control_packet_header *h,
> > > > +    uint8_t *data, int data_len)
> > >
> > > This style is new, I suppose copied from usbredir, but is not
> > > spice-gtk.
> > >
> > > > +{
> > > > +    SpiceUsbBackendChannel *ch = priv;
> > > > +    SpiceUsbBackendDevice *d = ch->attached;
> > > > +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> > > > +    struct usb_redir_control_packet_header response = *h;
> > > > +    uint8_t reqtype = h->requesttype & 0x7f;
> > > > +    gboolean done = FALSE;
> > > > +    void *out_buffer = NULL;
> > > > +
> > > > +    response.status = usb_redir_stall;
> > > > +    SPICE_DEBUG("%s %p: TRVIL %02X %02X %04X %04X %04X",
> > > > +                __FUNCTION__,
> > > > +                ch, h->requesttype, h->request,
> > > > +                h->value, h->index, h->length);
> > > > +
> > > > +    if (!edev) {
> > > > +        SPICE_DEBUG("%s: device not attached", __FUNCTION__);
> > > > +        response.status = usb_redir_ioerror;
> > > > +        done = TRUE;
> > > > +    } else if (reqtype == (LIBUSB_REQUEST_TYPE_STANDARD |
> > > > LIBUSB_RECIPIENT_DEVICE) &&
> > > > +               h->request == LIBUSB_REQUEST_GET_DESCRIPTOR) {
> > > > +        uint16_t size;
> > > > +        done = device_ops(edev)->get_descriptor(edev, h->value >> 8,
> > > > h->value & 0xff,
> > > > +                                                &out_buffer, &size);
> > > > +        response.length = size;
> > > > +        if (done) {
> > > > +            response.status = 0;
> > > > +        }
> > > > +        done = TRUE;
> > > > +    }
> > > > +
> > > > +    if (!done) {
> > > > +        device_ops(edev)->control_request(edev, data, data_len,
> > > > &response,
> > > > &out_buffer);
> > > > +        done = TRUE;
> > > > +    }
> > > > +
> > > > +    if (response.status) {
> > > > +        response.length = 0;
> > > > +    } else if (response.length > h->length) {
> > > > +        response.length = h->length;
> > > > +    }
> > > > +
> > > > +    SPICE_DEBUG("%s responding with payload of %02X, status %X",
> > > > +                __FUNCTION__, response.length, response.status);
> > > > +    usbredirparser_send_control_packet(ch->parser, id, &response,
> > > > +                                       response.length ? out_buffer :
> > > > NULL,
> > > > +                                       response.length);
> > > > +
> > > > +    usbredir_write_flush_callback(ch);
> > > > +    usbredirparser_free_packet_data(ch->parser, data);
> > > > +}
> > > > +
> > > > +static void usbredir_bulk_packet(void *priv,
> > > > +    uint64_t id, struct usb_redir_bulk_packet_header *h,
> > > > +    uint8_t *data, int data_len)
> > > > +{
> > > > +    SpiceUsbBackendChannel *ch = priv;
> > > > +    SpiceUsbBackendDevice *d = ch->attached;
> > > > +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> > > > +    struct usb_redir_bulk_packet_header hout = *h;
> > > > +    uint32_t len = (h->length_high << 16) | h->length;
> > > > +    SPICE_DEBUG("%s %p: ep %X, len %u, id %" G_GUINT64_FORMAT,
> > > > __FUNCTION__,
> > > > +                ch, h->endpoint, len, id);
> > > > +
> > > > +    if (!edev) {
> > > > +        SPICE_DEBUG("%s: device not attached", __FUNCTION__);
> > > > +        hout.status = usb_redir_ioerror;
> > > > +        hout.length = hout.length_high = 0;
> > > > +        SPICE_DEBUG("%s: responding with ZLP status %d", __FUNCTION__,
> > > > hout.status);
> > > > +    } else if (h->endpoint & LIBUSB_ENDPOINT_IN) {
> > > > +        if (device_ops(edev)->bulk_in_request(edev, id, &hout)) {
> > > > +            usbredirparser_free_packet_data(ch->parser, data);
> > > > +            /* completion is asynchronous */
> > > > +            return;
> > > > +        }
> > > > +    } else {
> > > > +        hout.status = usb_redir_stall;
> > > > +        device_ops(edev)->bulk_out_request(edev, h->endpoint, data,
> > > > data_len, &hout.status);
> > > > +        SPICE_DEBUG("%s: responding status %d", __FUNCTION__,
> > > > hout.status);
> > > > +    }
> > > > +
> > > > +    usbredirparser_send_bulk_packet(ch->parser, id, &hout, NULL, 0);
> > > > +    usbredirparser_free_packet_data(ch->parser, data);
> > > > +    usbredir_write_flush_callback(ch);
> > > > +}
> > > > +
> > > > +static void usbredir_device_reset(void *priv)
> > > > +{
> > > > +    SpiceUsbBackendChannel *ch = priv;
> > > > +    SpiceUsbBackendDevice *d = ch->attached;
> > > > +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> > > > +    SPICE_DEBUG("%s ch %p", __FUNCTION__, ch);
> > > > +    if (edev) {
> > > > +        device_ops(edev)->reset(edev);
> > > > +    }
> > > > +}
> > > > +
> > > > +static void usbredir_interface_info(void *priv,
> > > > +    struct usb_redir_interface_info_header *interface_info)
> > > > +{
> > > > +    SpiceUsbBackendChannel *ch = priv;
> > > > +    SPICE_DEBUG("%s not implemented %p", __FUNCTION__, ch);
> > > > +}
> > > > +
> > > > +static void usbredir_interface_ep_info(void *priv,
> > > > +    struct usb_redir_ep_info_header *ep_info)
> > > > +{
> > > > +    SpiceUsbBackendChannel *ch = priv;
> > > > +    SPICE_DEBUG("%s not implemented %p", __FUNCTION__, ch);
> > > > +}
> > > > +
> > > > +static void usbredir_set_configuration(void *priv,
> > > > +    uint64_t id, struct usb_redir_set_configuration_header
> > > > *set_configuration)
> > > > +{
> > > > +    SpiceUsbBackendChannel *ch = priv;
> > > > +    struct usb_redir_configuration_status_header h;
> > > > +    h.status = 0;
> > > > +    h.configuration = set_configuration->configuration;
> > > > +    SPICE_DEBUG("%s ch %p, cfg %d", __FUNCTION__, ch, h.configuration);
> > > > +    if (ch->attached) {
> > > > +        ch->attached->edev_configured = h.configuration != 0;
> > > > +    }
> > > > +    usbredirparser_send_configuration_status(ch->parser, id, &h);
> > > > +    usbredir_write_flush_callback(ch);
> > > > +}
> > > > +
> > > > +static void usbredir_get_configuration(void *priv, uint64_t id)
> > > > +{
> > > > +    SpiceUsbBackendChannel *ch = priv;
> > > > +    struct usb_redir_configuration_status_header h;
> > > > +    h.status = 0;
> > > > +    h.configuration = ch->attached && ch->attached->edev_configured;
> > > > +    SPICE_DEBUG("%s ch %p, cfg %d", __FUNCTION__, ch, h.configuration);
> > > > +    usbredirparser_send_configuration_status(ch->parser, id, &h);
> > > > +    usbredir_write_flush_callback(ch);
> > > > +}
> > > > +
> > > > +static void usbredir_set_alt_setting(void *priv,
> > > > +    uint64_t id, struct usb_redir_set_alt_setting_header *s)
> > > > +{
> > > > +    SpiceUsbBackendChannel *ch = priv;
> > > > +    struct usb_redir_alt_setting_status_header sh;
> > > > +    sh.status = (!s->interface && !s->alt) ? 0 : usb_redir_stall;
> > > > +    sh.interface = s->interface;
> > > > +    sh.alt = s->alt;
> > > > +    SPICE_DEBUG("%s ch %p, %d:%d", __FUNCTION__, ch, s->interface,
> > > > s->alt);
> > > > +    usbredirparser_send_alt_setting_status(ch->parser, id, &sh);
> > > > +    usbredir_write_flush_callback(ch);
> > > > +}
> > > > +
> > > > +static void usbredir_get_alt_setting(void *priv,
> > > > +    uint64_t id, struct usb_redir_get_alt_setting_header *s)
> > > > +{
> > > > +    SpiceUsbBackendChannel *ch = priv;
> > > > +    struct usb_redir_alt_setting_status_header sh;
> > > > +    sh.status = (s->interface == 0) ? 0 : usb_redir_stall;
> > > > +    sh.interface = s->interface;
> > > > +    sh.alt = 0;
> > > > +    SPICE_DEBUG("%s ch %p, if %d", __FUNCTION__, ch, s->interface);
> > > > +    usbredirparser_send_alt_setting_status(ch->parser, id, &sh);
> > > > +    usbredir_write_flush_callback(ch);
> > > > +}
> > > > +
> > > > +static void usbredir_cancel_data(void *priv, uint64_t id)
> > > > +{
> > > > +    SpiceUsbBackendChannel *ch = priv;
> > > > +    SpiceUsbBackendDevice *d = ch->attached;
> > > > +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> > > > +    if (!edev) {
> > > > +        SPICE_DEBUG("%s: device not attached", __FUNCTION__);
> > > > +        return;
> > > > +    }
> > > > +    device_ops(edev)->cancel_request(edev, id);
> > > > +}
> > > > +
> > > > +static void usbredir_filter_reject(void *priv)
> > > > +{
> > > > +    SpiceUsbBackendChannel *ch = priv;
> > > > +    SPICE_DEBUG("%s %p", __FUNCTION__, ch);
> > > > +    ch->rejected = 1;
> > > > +}
> > > > +
> > > > +/* Note that the ownership of the rules array is passed on to the
> > > > callback.
> > > > */
> > > > +static void usbredir_filter_filter(void *priv,
> > > > +    struct usbredirfilter_rule *r, int count)
> > > > +{
> > > > +    SpiceUsbBackendChannel *ch = priv;
> > > > +    SPICE_DEBUG("%s ch %p %d filters", __FUNCTION__, ch, count);
> > > > +
> > > > +    free(ch->rules);
> > > > +
> > > > +    ch->rules = r;
> > > > +    ch->rules_count = count;
> > > > +    if (count) {
> > > > +        int i;
> > > > +        for (i = 0; i < count; i++) {
> > > > +            SPICE_DEBUG("%s class %d, %X:%X",
> > > > +                r[i].allow ? "allowed" : "denied", r[i].device_class,
> > > > +                (uint32_t)r[i].vendor_id, (uint32_t)r[i].product_id);
> > > > +        }
> > > > +    }
> > > > +}
> > > > +
> > > > +static void usbredir_device_disconnect_ack(void *priv)
> > > > +{
> > > > +    SpiceUsbBackendChannel *ch = priv;
> > > > +    SPICE_DEBUG("%s ch %p", __FUNCTION__, ch);
> > > > +    if (ch->parser && ch->wait_disc_ack) {
> > > > +        ch->parser = NULL;
> > > > +        SPICE_DEBUG("%s switch to usbredirhost", __FUNCTION__);
> > > > +        ch->usbredirhost = ch->hidden_host;
> > > > +    }
> > > > +    ch->wait_disc_ack = 0;
> > > > +}
> > > > +
> > > > +static void usbredir_hello(void *priv,
> > > > +    struct usb_redir_hello_header *hello)
> > > > +{
> > > > +    SpiceUsbBackendChannel *ch = priv;
> > > > +    SpiceUsbBackendDevice *d = ch->attached;
> > > > +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> > > > +    struct usb_redir_device_connect_header device_connect;
> > > > +    struct usb_redir_ep_info_header ep_info = { 0 };
> > > > +    struct usb_redir_interface_info_header interface_info = { 0 };
> > > > +    uint8_t *cfg;
> > > > +    uint16_t size, offset = 0;
> > > > +    SPICE_DEBUG("%s %p %sattached %s", __FUNCTION__, ch,
> > > > +        edev ? "" : "not ",  hello ? "" : "(internal)");
> > > > +
> > > > +    if (hello) {
> > > > +        ch->hello_done_parser = 1;
> > > > +    }
> > > > +    if (!edev) {
> > > > +        return;
> > > > +    }
> > > > +    if (!device_ops(edev)->get_descriptor(edev, LIBUSB_DT_CONFIG, 0,
> > > > (void
> > > > **)&cfg, &size)) {
> > > > +        return;
> > > > +    }
> > > > +    while ((offset + 1) < size) {
> > > > +        uint8_t len  = cfg[offset];
> > > > +        uint8_t type = cfg[offset + 1];
> > > > +        if ((offset + len) > size) {
> > > > +            break;
> > > > +        }
> > > > +        if (type == LIBUSB_DT_INTERFACE) {
> > > > +            uint32_t i = interface_info.interface_count;
> > > > +            uint8_t class, subclass, protocol;
> > > > +            class = cfg[offset + 5];
> > > > +            subclass = cfg[offset + 6];
> > > > +            protocol = cfg[offset + 7];
> > > > +            interface_info.interface_class[i] = class;
> > > > +            interface_info.interface_subclass[i] = subclass;
> > > > +            interface_info.interface_protocol[i] = protocol;
> > > > +            interface_info.interface_count++;
> > > > +            SPICE_DEBUG("%s IF%d: %d/%d/%d", __FUNCTION__, i, class,
> > > > subclass, protocol);
> > > > +        } else if (type == LIBUSB_DT_ENDPOINT) {
> > > > +            uint8_t address = cfg[offset + 2];
> > > > +            uint16_t max_packet_size = 0x100 * cfg[offset + 5] +
> > > > cfg[offset
> > > > + 4];
> > > > +            uint8_t index = address & 0xf;
> > > > +            if (address & 0x80) index += 0x10;
> > > > +            ep_info.type[index] = cfg[offset + 3] & 0x3;
> > > > +            ep_info.max_packet_size[index] = max_packet_size;
> > > > +            SPICE_DEBUG("%s EP[%02X]: %d/%d", __FUNCTION__, index,
> > > > ep_info.type[index], max_packet_size);
> > > > +        }
> > > > +        offset += len;
> > > > +    }
> > > > +
> > > > +    usbredirparser_send_interface_info(ch->parser, &interface_info);
> > > > +    usbredirparser_send_ep_info(ch->parser, &ep_info);
> > > > +
> > > > +    device_connect.device_class = 0; //d->device_info.class;
> > > > +    device_connect.device_subclass = 0; //d->device_info.subclass;
> > > > +    device_connect.device_protocol = 0; //d->device_info.protocol;;
> > > > +    device_connect.vendor_id = d->device_info.vid;
> > > > +    device_connect.product_id = d->device_info.pid;
> > > > +    device_connect.device_version_bcd = d->device_info.bcdUSB;
> > > > +    device_connect.speed = usb_redir_speed_high;
> > > > +    usbredirparser_send_device_connect(ch->parser, &device_connect);
> > > > +    usbredir_write_flush_callback(ch);
> > > > +}
> > > > +
> > > > +static void initialize_parser(SpiceUsbBackendChannel *ch, gboolean
> > > > do_hello)
> > > > +{
> > > > +    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;
> > > > +    }
> > > > +
> > > > +    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;
> > >
> > > In which case you need to do the hello in the parser instead of the host?
> > > This could cause issues switching from parser back to host in case you
> > > remove an emulated device and you add a real one.
> >
> > The case is described above. From my point of view, having both
> > (usbredirhost and parser) ready and switch
> > according to the current need is the simplest way to do the job.
> >
> > >
> > > > +        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_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);
> > > > +}
> > > > +
> > > > +/*
> > > > +    We can initialize the usbredirparser with HELLO enabled only in case
> > > > +    the libusb is not active and the usbredirhost does not function.
> > > > +    Then the parser sends session HELLO and receives server's response.
> > > > +    Otherwise (usbredirparser initialized with HELLO disabled):
> > > > +    - the usbredirhost sends session HELLO
> > > > +    - we look into it to know set of capabilities we shall initialize
> > > > +      the parser with
> > > > +    - when we receive server's response to HELLO we provide it also to
> > > > +      parser to let it synchronize with server's capabilities
> > > > +*/
> > > > +static struct usbredirparser *create_parser(SpiceUsbBackendChannel *ch)
> > > > +{
> > > > +    struct usbredirparser *parser = usbredirparser_create();
> > > > +
> > > > +    g_return_val_if_fail(parser != NULL, NULL);
> > > > +
> > > > +    parser->priv = ch;
> > > > +    parser->log_func = usbredir_log;
> > > > +    parser->read_func = usbredir_read_callback;
> > > > +    parser->write_func = usbredir_write_callback;
> > > > +    parser->reset_func = usbredir_device_reset;
> > > > +    parser->set_configuration_func = usbredir_set_configuration;
> > > > +    parser->get_configuration_func = usbredir_get_configuration;
> > > > +    parser->set_alt_setting_func = usbredir_set_alt_setting;
> > > > +    parser->get_alt_setting_func = usbredir_get_alt_setting;
> > > > +    parser->cancel_data_packet_func = usbredir_cancel_data;
> > > > +    parser->control_packet_func = usbredir_control_packet;
> > > > +    parser->bulk_packet_func = usbredir_bulk_packet;
> > > > +    parser->alloc_lock_func = usbredir_alloc_lock;
> > > > +    parser->lock_func = usbredir_lock_lock;
> > > > +    parser->unlock_func = usbredir_unlock_lock;
> > > > +    parser->free_lock_func = usbredir_free_lock;
> > > > +    parser->hello_func = usbredir_hello;
> > > > +    parser->filter_reject_func = usbredir_filter_reject;
> > > > +    parser->device_disconnect_ack_func = usbredir_device_disconnect_ack;
> > > > +    parser->interface_info_func = usbredir_interface_info;
> > > > +    parser->ep_info_func = usbredir_interface_ep_info;
> > > > +    parser->filter_filter_func = usbredir_filter_filter;
> > > > +
> > > > +    return parser;
> > > > +}
> > > > +
> > > > +static gboolean attach_edev(SpiceUsbBackendChannel *ch,
> > > > +                            SpiceUsbBackendDevice *dev,
> > > > +                            GError **error)
> > > > +{
> > > > +    if (!dev->edev) {
> > > > +        g_set_error(error, SPICE_CLIENT_ERROR,
> > > > SPICE_CLIENT_ERROR_FAILED,
> > > > +           _("Failed to redirect device %d"), 1);
> > >
> > > Minor: I would say more an assert level error, the device should be real or
> > > emulated.
> > >
> > > > +        return FALSE;
> > > > +    }
> > > > +    if (ch->usbredirhost && !ch->hello_done_parser) {
> > > > +        /*
> > > > +            we can't initialize parser until we see hello from usbredir
> > > > +            and the parser can't work until it sees the hello response.
> > > > +            this is case of autoconnect when emulated device is attached
> > > > +            before the channel is up
> > > > +        */
> > > > +        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->wait_disc_ack = 0;
> > > > +    ch->attached = dev;
> > > > +    dev->attached_to = ch;
> > > > +    device_ops(dev->edev)->attach(dev->edev, ch->hidden_parser);
> > > > +    if (ch->hello_done_parser) {
> > > > +        /* send device info */
> > > > +        usbredir_hello(ch, NULL);
> > > > +    }
> > > > +    return TRUE;
> > > > +}
> > > > +
> > > >  gboolean spice_usb_backend_channel_attach(SpiceUsbBackendChannel *ch,
> > > >                                            SpiceUsbBackendDevice *dev,
> > > >                                            GError **error)
> > > > @@ -706,7 +1213,13 @@ gboolean
> > > > spice_usb_backend_channel_attach(SpiceUsbBackendChannel *ch,
> > > >
> > > >      g_return_val_if_fail(dev != NULL, FALSE);
> > > >
> > > > +    if (!dev->libusb_device) {
> > > > +        return attach_edev(ch, dev, error);
> > > > +    }
> > > > +
> > > >      libusb_device_handle *handle = NULL;
> > > > +    ch->usbredirhost = ch->hidden_host;
> > > > +    ch->parser = NULL;
> > > >
> > > >      /*
> > > >         Under Windows we need to avoid updating
> > > > @@ -740,18 +1253,33 @@ gboolean
> > > > spice_usb_backend_channel_attach(SpiceUsbBackendChannel *ch,
> > > >
> > > >  void spice_usb_backend_channel_detach(SpiceUsbBackendChannel *ch)
> > > >  {
> > > > +    SpiceUsbBackendDevice *d = ch->attached;
> > > > +    SpiceUsbEmulatedDevice *edev = d ? d->edev : NULL;
> > > >      SPICE_DEBUG("%s >> ch %p, was attached %p", __FUNCTION__, ch,
> > > >      ch->attached);
> > > > -    if (!ch->attached) {
> > > > +    if (!d) {
> > > >          SPICE_DEBUG("%s: nothing to detach", __FUNCTION__);
> > > >          return;
> > > >      }
> > > >      if (ch->usbredirhost) {
> > > >          /* it will call libusb_close internally */
> > > >          usbredirhost_set_device(ch->usbredirhost, NULL);
> > > > +    } else if (ch->parser) {
> > > > +        if (edev) {
> > > > +            device_ops(edev)->detach(edev);
> > > > +        }
> > > > +        usbredirparser_send_device_disconnect(ch->parser);
> > > > +        usbredir_write_flush_callback(ch);
> > > > +        ch->wait_disc_ack = usbredirparser_peer_has_cap(ch->parser,
> > > > +
> > > > usb_redir_cap_device_disconnect_ack);
> > > > +        if (!ch->wait_disc_ack) {
> > > > +            ch->usbredirhost = ch->hidden_host;
> > > > +            ch->parser = NULL;
> > > > +        }
> > > >      }
> > > >      SPICE_DEBUG("%s ch %p, detach done", __FUNCTION__, ch);
> > > >      ch->attached->attached_to = NULL;
> > > >      ch->attached = NULL;
> > > > +    ch->rejected = 0;
> > > >  }
> > > >
> > > >  SpiceUsbBackendChannel *spice_usb_backend_channel_new(SpiceUsbBackend
> > > >  *be,
> > > > @@ -766,26 +1294,33 @@ SpiceUsbBackendChannel
> > > > *spice_usb_backend_channel_new(SpiceUsbBackend *be,
> > > >      ch->user_data = SPICE_USBREDIR_CHANNEL(user_data);
> > > >      if (be->libusb_context) {
> > >
> > > This can never be NULL, if we are not able to allocate it inside
> > > spice_usb_backend_new the function returns NULL.
> > > This should be in a preliminary patch to make this clear.
> >
> > Looking a little forward, after cd-sharing is in, this could be
> > possible to not disable the redirection is there is no usbdk.
> > Even in case the libusb does not work, the client can work with
> > redirection enabled and redirect only emulated devices.
> > I would not remove this check.
> >
>
> The problem is that current code has some part with one logic
> (usb context is always available) and some other part of code
> with the opposite logic (usb context is not required).
>
> libusb works also without usbdk installed and provides a
> valid (not NULL) usb context.

If I'm not mistaken, this is true only with latest libusb (which
supports WinUSB and UsbDk).
With previous libusb which is built for UsbDk, the context can't be
initialized without UsbDk.

>
> > >
> > > >          ch->backend = be;
> > > > -        ch->usbredirhost = usbredirhost_open_full(
> > > > -            be->libusb_context,
> > > > -            NULL,
> > > > -            usbredir_log,
> > > > -            usbredir_read_callback,
> > > > -            usbredir_write_callback,
> > > > -            usbredir_write_flush_callback,
> > > > -            usbredir_alloc_lock,
> > > > -            usbredir_lock_lock,
> > > > -            usbredir_unlock_lock,
> > > > -            usbredir_free_lock,
> > > > -            ch, PACKAGE_STRING,
> > > > -            spice_util_get_debug() ? usbredirparser_debug :
> > > > usbredirparser_warning,
> > > > -            usbredirhost_fl_write_cb_owns_buffer);
> > > > +        ch->usbredirhost =
> > > > +            usbredirhost_open_full(be->libusb_context,
> > > > +                                   NULL,
> > > > +                                   usbredir_log,
> > > > +                                   usbredir_read_callback,
> > > > +                                   usbredir_write_callback,
> > > > +                                   usbredir_write_flush_callback,
> > > > +                                   usbredir_alloc_lock,
> > > > +                                   usbredir_lock_lock,
> > > > +                                   usbredir_unlock_lock,
> > > > +                                   usbredir_free_lock,
> > > > +                                   ch, PACKAGE_STRING,
> > > > +                                   spice_util_get_debug() ?
> > > > usbredirparser_debug :
> > > > +                                        usbredirparser_warning,
> > > > +
> > > > usbredirhost_fl_write_cb_owns_buffer);
> > > >          g_warn_if_fail(ch->usbredirhost != NULL);
> > > >      }
> > > > +
> > > >      if (ch->usbredirhost) {
> > > > -        usbredirhost_set_buffered_output_size_cb(ch->usbredirhost,
> > > > usbredir_buffered_output_size_callback);
> > > > -    } else {
> > > > -        g_free(ch);
> > > > +        ch->hidden_parser = create_parser(ch);
> > > > +        ch->hidden_host = ch->usbredirhost;
> > > > +        usbredirhost_set_buffered_output_size_cb(ch->usbredirhost,
> > > > +
> > > > usbredir_buffered_output_size_callback);
>
> Here you are assuming that usb context is not NULL which does not
> match with your statement above.
>
> > > > +    }
> > > > +
> > > > +    if (!ch->hidden_parser) {
> > > > +        spice_usb_backend_channel_delete(ch);
> > > >          ch = NULL;
> > > >      }
> > > >
> > > > @@ -795,9 +1330,14 @@ SpiceUsbBackendChannel
> > > > *spice_usb_backend_channel_new(SpiceUsbBackend *be,
> > > >
> > > >  void spice_usb_backend_channel_flush_writes(SpiceUsbBackendChannel *ch)
> > > >  {
> > > > -    SPICE_DEBUG("%s %p, host %p", __FUNCTION__, ch, ch->usbredirhost);
> > > > +    SPICE_DEBUG("%s %p is up", __FUNCTION__, ch);
> > > >      if (ch->usbredirhost) {
> > > >          usbredirhost_write_guest_data(ch->usbredirhost);
> > > > +        initialize_parser(ch, FALSE);
> > > > +    } else {
> > > > +        initialize_parser(ch, TRUE);
> > >
> > > Why initialize_parse is called here?
> >
> > This is the decision point whether parser works with hello=on or not
> >
>
> And so if this channel support only emulated devices which is a
> regression.

There is no regression.
At this specific point (in this corner case flow, because we just send
our capabilities) this channel supports only emulated device because
the emulated device is attached.
Strictly speaking, it does not support even emulated devices as we did
not receive the hello response yet.
It does not need to support local device _before_ the local device is attached.
Toward the moment when local device is attached the hello response
will be provided to the usbredirhost and it will support properly the
local device also.

>
> > >
> > > > +        ch->parser = ch->hidden_parser;
> > > > +        usbredirparser_do_write(ch->parser);
> > > >      }
> > > >  }
> > > >
> > > > @@ -807,12 +1347,16 @@ void
> > > > spice_usb_backend_channel_delete(SpiceUsbBackendChannel *ch)
> > > >      if (!ch) {
> > > >          return;
> > > >      }
> > > > -    if (ch->usbredirhost) {
> > > > -        usbredirhost_close(ch->usbredirhost);
> > > > +    if (ch->hidden_host) {
> > > > +        usbredirhost_close(ch->hidden_host);
> > > > +    }
> > > > +    if (ch->hidden_parser) {
> > > > +        usbredirparser_destroy(ch->hidden_parser);
> > > >      }
> > > >
> > > >      if (ch->rules) {
> > > > -        g_free(ch->rules);
> > > > +        /* rules were allocated by usbredirparser */
> > > > +        free(ch->rules);
> > >
> > > Minor :I suppose this small change can go to a preliminary patch too
> >
> > This code was here before, but it was never used as ch->rules was
> > never assigned.
> >
>
> yes
>
> > Honestly I do not understand what this "preliminary patch" should
> > contain and what it is good for.
> >
>
> I don't think is a good idea to add code with multiple logic for
> the same condition. Currently most of the code assume usb context
> is valid, I would go in a single direction, not adding additional
> code with 2 different login about it.

I still do not understand what is 'preliminary patch', what it should
contain and why it is good to have it.

>
> > >
> > > >      }
> > > >
> > > >      g_free(ch);
> > >
> > > Frediano
> >

On Fri, Aug 23, 2019 at 10:59 AM Victor Toso <victortoso@redhat.com> wrote:
>
> On Thu, Aug 22, 2019 at 11:27:19AM +0300, Yuri Benditovich wrote:
> > On Thu, Aug 22, 2019 at 9:56 AM Frediano Ziglio <fziglio@redhat.com> wrote:
> > > > On Tue, Aug 13, 2019 at 2:59 PM Frediano Ziglio <fziglio@redhat.com> wrote:
> > > > > > +        ch->parser = ch->hidden_parser;
> > > > > > +        usbredirparser_do_write(ch->parser);
> > > > > >      }
> > > > > >  }
> > > > > >
> > > > > > @@ -807,12 +1347,16 @@ void
> > > > > > spice_usb_backend_channel_delete(SpiceUsbBackendChannel *ch)
> > > > > >      if (!ch) {
> > > > > >          return;
> > > > > >      }
> > > > > > -    if (ch->usbredirhost) {
> > > > > > -        usbredirhost_close(ch->usbredirhost);
> > > > > > +    if (ch->hidden_host) {
> > > > > > +        usbredirhost_close(ch->hidden_host);
> > > > > > +    }
>
> It was usbredirhost, now it is hidden_host /* Preparatory comment :) */
>
> > > > > > +    if (ch->hidden_parser) {
> > > > > > +        usbredirparser_destroy(ch->hidden_parser);
>
> A new thing, hidden_parser /* Preparatory comment :) */
>
> > > > > >      }
> > > > > >
> > > > > >      if (ch->rules) {
> > > > > > -        g_free(ch->rules);
> > > > > > +        /* rules were allocated by usbredirparser */
> > > > > > +        free(ch->rules);
>
> Rules no more g_free, now is free /* Preparatory comment :) */
>
> > > > >
> > > > > Minor :I suppose this small change can go to a preliminary patch too
> > > >
> > > > This code was here before, but it was never used as ch->rules was
> > > > never assigned.
> > >
> > > yes
> > >
> > > > Honestly I do not understand what this "preliminary patch"
> > > > should contain and what it is good for.
> > >
> > > I don't think is a good idea to add code with multiple logic
> > > for the same condition. Currently most of the code assume usb
> > > context is valid, I would go in a single direction, not
> > > adding additional code with 2 different login about it.
> >
> > I still do not understand what is 'preliminary patch', what it
> > should contain and why it is good to have it.
>
> If we say "preparatory patch" would make it clear for you? The
> goal is to review faster, that's what you want, I'm sure of it.
> Break it down as much as you can and we can check it and learn
> from it easier.

No, this does not make thing more clear for me.
It looks like in order to create some 'preparatory patch' I need to
introduce new fields that are not actually used and create some code
for them that will be changed in next patch.
Is the goal to make the review faster? The code already has been
reviewed several times in details.
I expect this 'preparatory patch' will make things more complicated
and will spawn additional rounds of discussions.

>
> To proper reply your questions so this will not extend:
>
> > what it should contain
>
> If code existed and you can change it before adding more code,
> that's what it should be the content.
>
> > why it is good to have it
>
> To have fewer discussions on possible regressions, too big
> patches and if a change is really needed or not - this should be
> valid enough reason.
>
> Amazes me a little that it takes 4 iterations, possible more yet,
> for a single line of review with the keywords "Minor" + "can go"
> + "preliminary patch".
>
> Could you please split or do you have an actual reason not to?

It is hard to do as I do not understand the goal.
I'd suggest you to make any 'preparatory patch' that you find useful.


>
> Btw, really happy with Frediano's review, many thanks!