On 9/3/19 3:08 PM, Frediano Ziglio wrote:
Hi,
On 8/27/19 12:22 PM, Frediano Ziglio wrote:
Make initialisation easier.
Always initialise parser.
Initialise both parser and host during spice_usb_backend_channel_new.
Support not having libusb context (no physical devices).
Avoids too much state variables.
parser is always initialised after creation making sure the state
is consistent.
If usbredirhost is used, why is there a need for a parser too ?
Also below in initialize_parser there is a check that
ch->usbredirhost is NULL (possibly because the parser is always
being initialized before the host).
For a larger explanation see former patch from Yuri.
For a shorter to parse data directed to emulated devices.
So host devices (physical) through usbredirhost, emulated devices
through usbparser.
The commit log is saying the parser is always being initialized.
I'm asking if it is needed also for physical devices.
The check for NULL as this path is supposed to initialize the
parse only if usbredirhost is not initialize > Maybe would be worth a comment or another name. Or maybe just inline
it now, not very long. What do you suggest?
Just leave it as is. I'll look at previous patches to better understand.
Use a single state variable, data flows into/from a single parser.
Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
---
src/usb-backend.c | 236 +++++++++++++++++++++++-----------------------
1 file changed, 116 insertions(+), 120 deletions(-)
diff --git a/src/usb-backend.c b/src/usb-backend.c
index 36a73a89..d614e693 100644
--- a/src/usb-backend.c
+++ b/src/usb-backend.c
@@ -78,21 +78,21 @@ struct _SpiceUsbBackend
uint32_t own_devices_mask;
};
+typedef enum {
+ USB_CHANNEL_STATE_INITIALIZING,
+ USB_CHANNEL_STATE_HOST,
+ USB_CHANNEL_STATE_PARSER,
+} SpiceUsbBackendChannelState;
+
struct _SpiceUsbBackendChannel
{
struct usbredirhost *usbredirhost;
- struct usbredirhost *hidden_host;
struct usbredirparser *parser;
- struct usbredirparser *hidden_parser;
+ SpiceUsbBackendChannelState state;
uint8_t *read_buf;
int read_buf_size;
struct usbredirfilter_rule *rules;
int rules_count;
- uint32_t host_caps;
- uint32_t parser_init_done : 1;
- uint32_t parser_with_hello : 1;
- uint32_t hello_done_parser : 1;
- uint32_t hello_sent : 1;
uint32_t rejected : 1;
uint32_t wait_disconnect_ack : 1;
SpiceUsbBackendDevice *attached;
@@ -405,15 +405,16 @@ from both the main thread as well as from the usb
event handling thread */
static void usbredir_write_flush_callback(void *user_data)
{
SpiceUsbBackendChannel *ch = user_data;
+ if (ch->parser == NULL) {
+ return;
+ }
if (is_channel_ready(ch->user_data)) {
- if (ch->usbredirhost) {
Do you need the following g_assert, or is the ch->parser==NULL enough
g_assert(ch->state != USB_CHANNEL_STATE_INITIALIZING);
Which one are you referring to?
+ if (ch->state == USB_CHANNEL_STATE_HOST) {
SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
usbredirhost_write_guest_data(ch->usbredirhost);
- } else if (ch->parser) {
+ } else {
SPICE_DEBUG("%s ch %p -> parser", __FUNCTION__, ch);
usbredirparser_do_write(ch->parser);
- } else {
- SPICE_DEBUG("%s ch %p (NOT ACTIVE)", __FUNCTION__, ch);
}
} else {
SPICE_DEBUG("%s ch %p (not ready)", __FUNCTION__, ch);
@@ -673,21 +674,42 @@ static void usbredir_log(void *user_data, int level,
const char *msg)
}
}
+static struct usbredirparser *create_parser(SpiceUsbBackendChannel *ch);
+
static int usbredir_write_callback(void *user_data, uint8_t *data, int
count)
{
SpiceUsbBackendChannel *ch = user_data;
int res;
SPICE_DEBUG("%s ch %p, %d bytes", __FUNCTION__, ch, count);
- if (!ch->hello_sent) {
- /* hello is short header (12) + hello struct (64) + capabilities
(4) */
- int hello_size = sizeof(struct usb_redir_header) + sizeof(struct
usb_redir_hello_header);
- ch->hello_sent = 1;
- if (count == hello_size) {
- memcpy(&ch->host_caps, data + hello_size -
sizeof(ch->host_caps),
- sizeof(ch->host_caps));
- SPICE_DEBUG("%s ch %p, sending first hello, caps %08X",
- __FUNCTION__, ch, ch->host_caps);
+
+ // handle first packet (HELLO) creating parser from capabilities
+ if (SPICE_UNLIKELY(ch->parser == NULL)) {
+ // we are still initializing the host
+ if (ch->usbredirhost == NULL) {
+ return 0;
}
+
+ ch->parser = create_parser(ch);
+ if (!ch->parser) {
+ return 0;
+ }
+
+ /* hello is short header (12) + hello struct (64) */
+ const int hello_size = 12 + sizeof(struct usb_redir_hello_header);
sizeof(struct usb_redir_header) instead of 12 (and btw it's 16)
Yes, that's why 12 is used. It's not usb_redir_header that you want but the
32 bit version which is defined only internally in usbredir code (not public
headers).
Using internal-only compatibility header is not nice.
Why not use 64bit and public headers (check the peer supports
it too first) ?
(again if it was answered in previous emails I apologize)
+ g_assert(count >= hello_size + 4);
nit: Maybe replace 4 with
const size_t caps_size = sizeof(uint32_t) * USB_REDIR_CAPS_SIZE;
g_assert(count >= hello_size + caps_size);
This potentially can crash in the future when USB_REDIR_CAPS_SIZE will change.
But I suppose you can replace "4" with sizeof(uint32_t).
OK, so basically there must be at least 1 uint32_t for caps and
in the future maybe more (according to count). If it does change
in the future and we really want to, we can tell by peer version.
Thanks,
Uri.
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel