<div dir="ltr"><div dir="ltr"> </div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jan 15, 2019 at 4:52 PM Christophe Fergeau <<a href="mailto:cfergeau@redhat.com">cfergeau@redhat.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hey, On Mon, Jan 07, 2019 at 03:50:12PM +0200, Yuri Benditovich wrote: > > > > > if (!priv->host) > > > - g_error("Out of memory allocating usbredirhost"); > > > + g_error("Out of memory initializing redirection support"); > > > > > > -#if USBREDIR_VERSION >= 0x000701 > > > - usbredirhost_set_buffered_output_size_cb(priv->host, > > usbredir_buffered_output_size_callback); > > > -#endif > > > #ifdef USE_LZ4 > > > spice_channel_set_capability(channel, > > SPICE_SPICEVMC_CAP_DATA_COMPRESS_LZ4); > > > #endif > > > @@ -299,8 +279,6 @@ static gboolean spice_usbredir_channel_open_device( > > > { > > > SpiceUsbredirChannelPrivate *priv = channel->priv; > > > SpiceSession *session; > > > - libusb_device_handle *handle = NULL; > > > - int rc, status; > > > SpiceUsbDeviceManager *manager; > > > > > > g_return_val_if_fail(priv->state == STATE_DISCONNECTED > > > @@ -309,21 +287,16 @@ static gboolean spice_usbredir_channel_open_device( > > > #endif > > > , FALSE); > > > > > > - rc = libusb_open(priv->device, &handle); > > > - if (rc != 0) { > > > - g_set_error(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED, > > > - "Could not open usb device: %s [%i]", > > > - spice_usbutil_libusb_strerror(rc), rc); > > > - return FALSE; > > > - } > > > - > > > priv->catch_error = err; > > > - status = usbredirhost_set_device(priv->host, handle); > > > - priv->catch_error = NULL; > > > - if (status != usb_redir_success) { > > > - g_return_val_if_fail(err == NULL || *err != NULL, FALSE); > > > + if (!spice_usb_backend_channel_attach(priv->host, priv->device, > > err)) { > > > + priv->catch_error = NULL; > > > + if (*err == NULL) { > > > + g_set_error(err, SPICE_CLIENT_ERROR, > > SPICE_CLIENT_ERROR_FAILED, > > > + "Error attaching device: (no error information)"); > > > > You could line up the "Error .." with the opening parenthesis. > > > > > This is not described exactly in Spice coding style document and existing > code of spice-gtk > uses different solution for indentation, so this is point of personal > preference. >From a quick look, aligning multiple line arguments lists seem much more common that then opposite. See the g_set_error call that this hunk is removing for example. > > > @@ -362,8 +335,7 @@ static void spice_usbredir_channel_open_acl_cb( > > > spice_usbredir_channel_open_device(channel, &err); > > > } > > > if (err) { > > > - libusb_unref_device(priv->device); > > > - priv->device = NULL; > > > + g_clear_pointer(&priv->device, > > spice_usb_backend_device_release); > > > > _unref rather than _release? > > > > This can be changed in V3, although I would prefer not to change it. > Semantics of ref/unref and acquire/release are not different. > Please let me know whether this change in mandatory. I assume you mean that the semantics of ref/unref and acquire/release *are* different? If this is what you mean, how are they different? If the semantics are the same, then let's use the same name. </blockquote><div> </div><div>No, I mean that this is the same and there is no difference between ref/unref</div><div>and acquire/release; ref/unref is used in glib, so I prefer to use acquire/release.</div><div>I do not have a goal to mimic libusb, I rather prefer to remove any knowledge</div><div>about libusb from all the parts of spice-gtk except of backend code.</div><div>I've asked whether this change is mandatory, i.e. if this leads the rejection of the patch.</div><div>Please respond, preferrably yes or no.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > > > @@ -446,7 +420,8 @@ void spice_usbredir_channel_connect_device_async( > > > goto done; > > > } > > > > > > - priv->device = libusb_ref_device(device); > > > + spice_usb_backend_device_acquire(device); > > > + priv->device = device; > > > > You could mimic libusb API actually, > > priv->device = spice_usb_backend_device_ref(device); > > > > I do not think I need to mimic libusb or something other. > Please let me know whether this change is mandatory. libusb_ref_device, g_object_ref all return a pointer to the new ref, and this makes that code slightly simpler, so just a suggestion in case you think that's a good change. > > > > > + void *msc; > > > + } d; > > > + gboolean is_libusb; > > > + gint ref_count; > > > + SpiceUsbBackendChannel *attached_to; > > > > You don't need 'msc' just yet, nor 'is_libusb', and I'm not sure about > > 'attached_to' > > > > msc can be changed to reserved if you do not like it so much. > removal of is_libusb makes further merges harder (and current commit is > only prerequisite for further merge). > attached_to is useful for debugging/support > Please let me know whether this is a requirement. This patch is refactoring the existing code so that we can add more stuff on top of it. It should not be adding new things, especially if they are not used in the current commit. attached_to can be added in a separate commit with the debugging checks that it lets us do, msc can be added when it's needed. </blockquote><div> </div><div>Changing the code as you suggest makes further patches more complicated.</div><div>Please let me know whether this is a requirement. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > > > > > > + void *hotplug_user_data; > > > + libusb_hotplug_callback_handle hotplug_handle; > > > +}; > > > + > > > +struct _SpiceUsbBackendChannel > > > +{ > > > + struct usbredirhost *usbredirhost; > > > + uint8_t *read_buf; > > > + int read_buf_size; > > > + struct usbredirfilter_rule *rules; > > > + int rules_count; > > > + SpiceUsbBackendDevice *attached; > > > > I don't think this is used. > > > > The same as is_libusb. > In general even now the reference to attached device and attached channel > are useful for debugging and parsing logs and dumps. If it's not used in this commit, then it does not belong in it. Once it starts being used, then we can add it. Or if it's a debugging feature, just add it in its own commit with a description of why we want it. > > > > > + SpiceUsbBackendChannelInitData channel_data; > > > > I don't think 'channel_data' is a much more descriptive than the > > previously used 'data'. SpiceUsbBackendChannelInitData is a bunch of > > vfuncs/callbacks, so maybe use one of these words in the naming? > > > > It can be renamed in V3, please provide the name you agree with. backend_vfuncs maybe? Overall, this just feels like something odd we put in each SpiceUsbBackendChannel instances for lack of a good place in the design. > > > > > > > +}; > > > + > > > +// it's unclear why we use this procedure instead of libusb_error_name, > > > +// which by definition supports any error that libusb returns > > > > Why not add a commit before this one switching libusb_error_name() then? > > > > Because this looks strange: first change it in 10 places then remove all of > them in next commit. > Also because this make the merge very difficult (for me). On your cd redirection branch, do a git rebase -i origin/master^ and say that you want to edit the commit for the current git master. Do the libusb_error_name() change there, commit it. git revert HEAD git rebase --continue Then you rebase once more to do squash the revert with this commit, and you need to edit this commit to make it use libusb_error_name() too. > I can remove this note. I can use libusb_error_name instead of this > procedure. If what I described above is complicated, then remove the comment from this commit, and you can have a followup commit switching to libusb_error_name(). > > > > > +#endif > > > +#endif > > > + } > > > + SPICE_DEBUG("%s <<", __FUNCTION__); > > > + return be; > > > +} > > > + > > > +gboolean spice_usb_backend_handle_events(SpiceUsbBackend *be) > > > +{ > > > + SPICE_DEBUG("%s >>", __FUNCTION__); > > > + gboolean ok = FALSE; > > > + if (be->libusb_context) { > > > + SPICE_DEBUG("%s >> libusb", __FUNCTION__); > > > + int res = libusb_handle_events(be->libusb_context); > > > + ok = res == 0; > > > + if (res && res != LIBUSB_ERROR_INTERRUPTED) { > > > + const char *desc = spice_usbutil_libusb_strerror(res); > > > + g_warning("Error handling USB events: %s [%i]", desc, res); > > > + ok = FALSE; > > > > You don't really need 'ok' in that method, just return FALSE; here... > > > > Please let me know whether this change is mandatory. > > > > > > > + } > > > + } > > > + SPICE_DEBUG("%s << %s %d", __FUNCTION__, > > > + be->libusb_context ? "libusb" : "no libusb", ok); > > > + return ok; > > > > ... and return TRUE; there (and I'm not sure how useful the SPICE_DEBUG > > are going to be?) > > > > > Please let me know whether this change is mandatory. > Reformatting the code will make further merger more difficult. Complaining about merge conflicts related to a 10 line function is not a very compelling argument against making code simpler/easier to read in a preliminary patch... </blockquote><div> </div><div>Please let me know whether this change is mandatory. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > > > +gboolean spice_usb_backend_handle_hotplug(SpiceUsbBackend *be, > > > + void *user_data, > > > + usb_hot_plug_callback proc) > > > > Why not > > spice_usb_backend_enable_hotplug/spice_usb_backend_disable_hotplug? > > (and now that we have a proper SpiceUsbBackend struct wrapping the > > libusb stuff, I wonder if it would not make sense to just turn it into a > > gobject, and emit signals when we get new devices). > > > > > Regarding 'why not to split it to two procedures': this is point of > personal preference. If the splitting is mandatory, please let me > know, I'll do that in V3. Well, this function is not handling anything, it's setting up a handler for future hotplug events. So in my opinion, the naming is misleading. > Regarding turning it to GObject - this will discard entire patch and spawn > completely different activity that requires additional debugging. Is > this is mandatory, we will need to reevaluate our ability to deliver > the patches. I agree this is a matter that would have been easier to solve during the initial design/while writing the code. > > > +SpiceUsbBackendDevice > > **spice_usb_backend_get_device_list(SpiceUsbBackend *be) > > > +{ > > > + LOUD_DEBUG("%s >>", __FUNCTION__); > > > + libusb_device **devlist = NULL, **dev; > > > + SpiceUsbBackendDevice *d, **list; > > > + > > > + int n = 0, index; > > > + > > > + if (be && be->libusb_context) { > > > + libusb_get_device_list(be->libusb_context, &devlist); > > > + } > > > + > > > + // add all the libusb device that not present in our list > > > + for (dev = devlist; dev && *dev; dev++) { > > > + n++; > > > + } > > > + > > > + list = g_new0(SpiceUsbBackendDevice*, n + 1); > > > + > > > + index = 0; > > > + > > > + for (dev = devlist; dev && *dev; dev++) { > > > + d = allocate_backend_device(*dev); > > > + if (!d) { > > > + libusb_unref_device(*dev); > > > + } else { > > > + SPICE_DEBUG("created dev %p, usblib dev %p", d, *dev); > > > + list[index++] = d; > > > + } > > > + } > > > + > > > + if (devlist) { > > > + libusb_free_device_list(devlist, 0); > > > + } > > > + > > > + LOUD_DEBUG("%s <<", __FUNCTION__); > > > + return list; > > > +} > > > > I still think this could be made slightly simpler with GArray/GPtrArray. > > > > I do not see any advantage at all, just additional change that makes > further merges more complicated. This makes the method a bit shorter, and avoids the bookkeeping related to list length and position in the list: SpiceUsbBackendDevice **spice_usb_backend_get_device_list(SpiceUsbBackend *be) { LOUD_DEBUG("%s >>", __FUNCTION__); libusb_device **devlist = NULL; libusb_device **dev; GPtrArray *devices = g_ptr_array_new(); if (be && be->libusb_context) { libusb_get_device_list(be->libusb_context, &devlist); } for (dev = devlist; dev && *dev; dev++) { SpiceUsbBackendDevice *d; d = allocate_backend_device(*dev); if (!d) { libusb_unref_device(*dev); } else { SPICE_DEBUG("created dev %p, usblib dev %p", d, *dev); g_ptr_array_add(devices, d); } } g_ptr_array_add(devices, NULL); if (devlist) { libusb_free_device_list(devlist, 0); } LOUD_DEBUG("%s <<", __FUNCTION__); return (SpiceUsbBackendDevice **)g_ptr_array_free(devices, FALSE); } > > > +SpiceUsbBackendChannel > > *spice_usb_backend_channel_initialize(SpiceUsbBackend *be, > > > + const > > SpiceUsbBackendChannelInitData *init_data) > > > +{ > > > + SpiceUsbBackendChannel *ch = g_new0(SpiceUsbBackendChannel, 1); > > > + SPICE_DEBUG("%s >>", __FUNCTION__); > > > + ch->channel_data = *init_data; > > > + if (be->libusb_context) { > > > > You probably can just have g_return_val_if_fail(be->libusb_context != > > NULL, NULL) at the very beginning of that method, > > and only do the allocation after that. > > > > Please let me know whether this is mandatory. > I can do that but then later it should be rewritten again. As your prefer. > > > +typedef struct UsbDeviceInformation > > > +{ > > > + uint16_t bus; > > > + uint16_t address; > > > + uint16_t vid; > > > + uint16_t pid; > > > + uint8_t class; > > > + uint8_t subclass; > > > + uint8_t protocol; > > > + uint8_t isochronous; > > > +} UsbDeviceInformation; > > > > As already mentioned in the past, please let's not duplicate very similar > > information with different names/different types in 2 structures with > > very close names (UsbDeviceInformation and SpiceUsbDeviceInfo) > > > > > I do not see any possibility to do that in context of current commit. > This can be done later when SpiceUsbDevice will have persistent link > to backend device with it's information. For now this information is not > duplicated, it is cached in SpiceUsbDeviceInfo and can be returned > without looking for backend device. > If this is mandatory requirement, please let me know - this discards > current commit and we will need to reevaluate our plans. I think you are explaining why you cannot have a single instance of that data in memory. I don't really mind if we have multiple instances of that data in memory, however having 2 different types being used to store what is essentially the same thing is what I'm objecting to. How complicated would it be to use the same type each time we need to store USB information? </blockquote><div> </div><div>If I would change the name of the structure in usb-device-manager in my initial commit,</div><div>I'd immediately asked NOT to do changes that are not related to current commit.</div><div>Again, I try to make this patch more simple.</div><div>Additional changes (NOT related directly to this patch) IMO can be done later.</div><div>make this patch more complicated.</div><div>Please let me know whether this is mandatory.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > > > + > > > +typedef void (*usb_channel_error_callback)(void *user_data, const char > > *msg); > > > +typedef int (*usb_channel_write_callback)(void *user_data, uint8_t > > *data, int count); > > > +typedef gboolean (*usb_channel_is_ready_callback)(void *user_data); > > > +typedef uint64_t (*usb_channel_get_queue_size)(void *user_data); > > > + > > > +typedef struct SpiceUsbBackendChannelInitData > > > +{ > > > + void *user_data; > > > + usb_channel_error_callback on_error; > > > + usb_channel_write_callback write_callback; > > > + usb_channel_is_ready_callback is_channel_ready; > > > + usb_channel_get_queue_size get_queue_size; > > > > I would not add typedef for these vfuncs, I think the types are only > > used here, so I'd directly use: > > void *user_data; > > void (*on_error)(void *user_data, const char *msg); > > ... > > > > > Please let me know whether this is mandatory. Please change it, unless you plan to make extensive use of these types later on. > > > @@ -122,8 +124,7 @@ static ssize_t > > > g_udev_client_list_devices(GUdevClient *self, GList **devs, > > > GError **err, const gchar *name) > > > { > > > - gssize rc; > > > - libusb_device **lusb_list, **dev; > > > + SpiceUsbBackendDevice **lusb_list, **dev; > > > GUdevClientPrivate *priv; > > > GUdevDeviceInfo *udevinfo; > > > GUdevDevice *udevice; > > > @@ -136,13 +137,8 @@ g_udev_client_list_devices(GUdevClient *self, GList > > **devs, > > > > > > g_return_val_if_fail(self->priv->ctx != NULL, -3); > > > > > > - rc = libusb_get_device_list(priv->ctx, &lusb_list); > > > - if (rc < 0) { > > > - const char *errstr = spice_usbutil_libusb_strerror(rc); > > > - g_warning("%s: libusb_get_device_list failed - %s", name, > > errstr); > > > - g_set_error(err, G_UDEV_CLIENT_ERROR, > > G_UDEV_CLIENT_LIBUSB_FAILED, > > > - "%s: Error getting device list from libusb: %s > > [%"G_GSSIZE_FORMAT"]", > > > - name, errstr, rc); > > > + lusb_list = spice_usb_backend_get_device_list(priv->ctx); > > > + if (!lusb_list) { > > > return -4; > > > } > > > > Why did you remove the g_set_error? > > > > > Possible, just because this never happens, empty list it will receive > anyway. > I can return it back, if you like it. Hmm, should spice_usb_backend_get_device_list() be able to return errors such as what was done before? I think before we could differentiate between "empty list of usb devices" and "an error occurred while getting the list", while now all we know is that we got an empty list. But we still return -4 on an empty list, which I assume indicates an error? So why not stop setting the GError? > Please respond regarding 2 serious questions: > - whether there is change in indirection level or wrong use of obtained > libdev I'll answer this in a separate email, I did not have time to consider it yet. > - whether we discard this commit to avoid caching of device properties > If we do not discard it, please let me know whether specific mentioned > changes are mandatory. and as indicated before, I think there was a misunderstanding on this one, what I object to is introducing a new type to hold USB device information, I don't necessarily object to the additional 'caching'. </blockquote><div> </div><div>New type (public one) is introduced because the usb-device-manager uses</div><div>private data structure to the same, so the backed can't just reuse it.</div><div>Usb-device-manager will be able use this public type, if it will be required at all.</div><div>From my point of view, further simplification of the usb-device-manager will</div><div>remove the need to have any separate data structure.</div><div> </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Christophe </blockquote></div></div>