[Spice-devel] [spice-gtk] usb-redir: use persistent libusb device on Windows
Frediano Ziglio
fziglio at redhat.com
Tue Feb 26 14:23:52 UTC 2019
> On Fri, Feb 22, 2019 at 2:06 PM Christophe Fergeau <cfergeau at redhat.com>
> wrote:
> >
> > Hi,
> >
> > On Thu, Feb 21, 2019 at 09:37:06AM +0200, Yuri Benditovich wrote:
> > > On Wed, Feb 20, 2019 at 6:03 PM Christophe Fergeau <cfergeau at redhat.com>
> > > wrote:
> > > > > static SpiceUsbDevice*
> > > > > spice_usb_device_manager_find_device(SpiceUsbDeviceManager *self,
> > > > > const int bus, const int
> > > > > address)
> > > > > @@ -970,6 +914,16 @@ static void
> > > > > spice_usb_device_manager_add_dev(SpiceUsbDeviceManager *self,
> > > > > if (desc.bDeviceClass == LIBUSB_CLASS_HUB)
> > > > > return;
> > > > >
> > > > > + if (spice_usb_device_manager_find_device(self,
> > > > > + libusb_get_bus_number(libdev),
> > > > > +
> > > > > libusb_get_device_address(libdev)))
> > > > > {
> > > > > + SPICE_DEBUG("device not added %04x:%04x (%p)",
> > > > > + desc.idVendor,
> > > > > + desc.idProduct,
> > > > > + libdev);
> > > > > + return;
> > > > > + }
> > > > > +
> > > >
> > > > I did not understand why we need this?
> > >
> > > There are several reasons:
> > > 1. It is possible that the hot plug procedure for Linux might be
> > > called more than once for the same device (this is mentioned in libusb
> > > API).
> >
> > It's
> > http://libusb.sourceforge.net/api-1.0/group__hotplug.html#gae6c5f1add6cc754005549c7259dc35ea
> > and this can happen at coldplug time (ie when using
> > LIBUSB_HOTPLUG_ENUMERATE), so exactly the race I was worried about
> > before.
> >
> > > 2. If second instance of usb-device-manager created (as it happens at
> > > time of remote-viewer termination),
> >
> > It sounds weird that we create a new one when exiting, probably
> > something we can/should get rid of if this starts causing problems.
> >
> > > existing one receives each device that already present.
> > > 3. It is much simpler to exclude device duplication than later look
> > > why they present.
> >
> >
> > All of these explanations would be very useful to have in the log of a
> > separate commit.
> >
> > > > > +#ifdef G_OS_WIN32
> > > > > static void spice_usb_device_manager_uevent_cb(GUdevClient
> > > > > *client,
> > > > > - const gchar
> > > > > *action,
> > > > > - GUdevDevice
> > > > > *udevice,
> > > > > + libusb_device
> > > > > *libdev,
> > > > > + gboolean add,
> > > > > gpointer
> > > > > user_data)
> > > > > {
> > > > > SpiceUsbDeviceManager *self =
> > > > > SPICE_USB_DEVICE_MANAGER(user_data);
> > > > >
> > > > > - if (g_str_equal(action, "add"))
> > > > > - spice_usb_device_manager_add_udev(self, udevice);
> > > > > - else if (g_str_equal (action, "remove"))
> > > > > - spice_usb_device_manager_remove_udev(self, udevice);
> > > > > + if (add)
> > > > > + spice_usb_device_manager_add_dev(self, libdev);
> > > > > + else
> > > > > + spice_usb_device_manager_remove_dev(self,
> > > > > + libusb_get_bus_number(libdev),
> > > > > +
> > > > > libusb_get_device_address(libdev));
> > > >
> > > > This could almost reuse spice_usb_device_manager_hotplug_cb somehow,
> > > > but
> > > > I'm not sure this is worth it.
> > >
> > > What I'm expected to do to address this note?
> >
> > Just random thoughts, maybe you'll think this could be a useful
> > improvement, maybe you already considered it and rejected it, maybe it's
> > not a good idea, I don't know. I don't have any expectations, just
> > something that could be useful to mention.
> >
> >
> > > > > @@ -1889,9 +1751,7 @@ static SpiceUsbDeviceInfo
> > > > > *spice_usb_device_new(libusb_device *libdev)
> > > > > info->pid = pid;
> > > > > info->ref = 1;
> > > > > info->isochronous = probe_isochronous_endpoint(libdev);
> > > > > -#ifndef G_OS_WIN32
> > > > > info->libdev = libusb_ref_device(libdev);
> > > > > -#endif
> > > > >
> > > > > return info;
> > > > > }
> > > > > @@ -2027,14 +1887,11 @@ static void
> > > > > spice_usb_device_unref(SpiceUsbDevice *device)
> > > > >
> > > > > ref_count_is_0 = g_atomic_int_dec_and_test(&info->ref);
> > > > > if (ref_count_is_0) {
> > > > > -#ifndef G_OS_WIN32
> > > > > libusb_unref_device(info->libdev);
> > > > > -#endif
> > > >
> > > > Most of the 'persistent' libusb_device changes for Windows are in the
> > > > hunks before this, but they also depend on some rework of the hotplug
> > > > detection in GUdevClient.
> > >
> > > What I'm expected to do to address this note?
> >
> > Just a comment that could be useful to me in the future, or to other
> > people looking at that mail.
> >
> >
> > > > > diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> > > > > index 327976d..ef6804f 100644
> > > > > --- a/src/win-usb-dev.c
> > > > > +++ b/src/win-usb-dev.c
> > > > > @@ -49,31 +49,6 @@ G_DEFINE_TYPE_WITH_CODE(GUdevClient,
> > > > > g_udev_client, G_TYPE_OBJECT,
> > > > > G_ADD_PRIVATE(GUdevClient)
> > > > > G_IMPLEMENT_INTERFACE(G_TYPE_INITABLE,
> > > > > g_udev_client_initable_iface_init))
> > > > >
> > > > > -
> > > > > -typedef struct _GUdevDeviceInfo GUdevDeviceInfo;
> > > > > -
> > > > > -struct _GUdevDeviceInfo {
> > > > > - guint16 bus;
> > > > > - guint16 addr;
> > > > > - guint16 vid;
> > > > > - guint16 pid;
> > > > > - guint16 class;
> > > > > - gchar sclass[4];
> > > > > - gchar sbus[4];
> > > > > - gchar saddr[4];
> > > > > - gchar svid[8];
> > > > > - gchar spid[8];
> > > > > -};
> > > > > -
> > > > > -struct _GUdevDevicePrivate
> > > > > -{
> > > > > - /* FixMe: move above fields to this structure and access them
> > > > > directly */
> > > > > - GUdevDeviceInfo *udevinfo;
> > > > > -};
> > > > > -
> > > > > -G_DEFINE_TYPE_WITH_PRIVATE(GUdevDevice, g_udev_device,
> > > > > G_TYPE_OBJECT)
> > > >
> > > > For what it's worth, adding a libusb_device pointer to
> > > > GUdevDevicePrivate and using this to implement the 'persistent device'
> > > > thing on Windows, and removing GUdevDevice/GUdevInfo in followup
> > > > commits
> > > > greatly reduce the overall size of the patch.
> > > >
> > >
> > > If you find this mandatory, please reject this patch and clearly
> > > request to provide patches that are not bigger than N lines.
> >
> > I don't think we should have a hard limit on the number of lines in a
> > patch, however there should be a maximum of 1 logical change per commit,
> > see
> > https://www.berrange.com/posts/2012/06/27/thoughts-on-improving-openstack-git-commit-practicehistory/
> > for example for a rationale about this.
> > https://gitlab.freedesktop.org/teuf/spice-gtk/tree/gudev would be a
> > rough attempt at such a split (but authorship needs to be set back to
> > you, commit logs needs to be more verbose, ...).
> >
>
> IMO, this is excellent example of why this approach is wrong.
> For example, there is definitely wrong commit
> https://gitlab.freedesktop.org/teuf/spice-gtk/commit/0135d831bc8929e45bac065f47daa1b4470ab7b0
> But nobody notice it as the problem in it fixed toward end of chain.
> Which tests are expected after _each_ commit?
>
One issue is that we are not really good at testing. How many tests, manual
or automatic, do we have?
To extend the concept "this commit was not tested so does not make sense"
I could test every 3 feature I add because I'm lazy, this does not mean
I should have a single commit every 3 feature I add.
Splitting commit is not only a question of tested or not, it's also a
question of logic changes.
However if the coder does not feel sensible to split and instead the
reviewer sees multiple patches, maybe it would be sensible if
the reviewer do the process of splitting with the coordination of
the coder instead of the coder, which does not see the rationale
(which rationale is the logic on how to split).
> > > > > -GUdevClient *g_udev_client_new(const gchar* const *subsystems)
> > > > > +GUdevClient *g_udev_client_new(libusb_context **pctx)
> > > > > {
> > > > > - if (singleton != NULL)
> > > > > + if (singleton != NULL) {
> > > > > + *pctx = singleton->priv->ctx;
> > > > > return g_object_ref(singleton);
> > > >
> > > > Having a g_udev_client_new(void) +
> > > > g_udev_client_get_libusb_context(UdevClient *)
> > > > is more common in gobject APIs.
> > > >
> > >
> > > I do not see any reason to create 2 APIs where one is sufficient and
> > > easy for error handling.
> > > If this change is mandatory, please state it clearly.
> >
> > I'd prefer if this is changed.
> >
> > > > > static void g_udev_client_init(GUdevClient *self)
> > > > > @@ -335,11 +317,11 @@ static void
> > > > > g_udev_client_class_init(GUdevClientClass *klass)
> > > > > G_SIGNAL_RUN_FIRST,
> > > > > G_STRUCT_OFFSET(GUdevClientClass, uevent),
> > > > > NULL, NULL,
> > > > > - g_cclosure_user_marshal_VOID__BOXED_BOXED,
> > > > > + g_cclosure_user_marshal_VOID__POINTER_INT,
> > > >
> > > > libusb_device could/should be boxed
> > >
> > > I do not see any reason for additional ref/unref for libusb_device.
> > > If you can provide one, please do.
> > > If you find this change mandatory for the patch to be accepted, please
> > > state it clearly.
> >
> >
> > It's imo a bit cleaner, this ensures the libusb_device stays alive for
> > the duration of the signal emission/handling, even if the callbacks do
> > weird things (such as drop the last to the libusb_device being handled).
> > Whether to change this or not is your call.
> >
> > > > > -static gboolean get_usb_dev_info(libusb_device *dev, GUdevDeviceInfo
> > > > > *udevinfo)
> > > > > +/* Only bus:addr are compared */
> > > > > +static gint compare_libusb_devices(gconstpointer a, gconstpointer b)
> > > > > {
> > > > > - struct libusb_device_descriptor desc;
> > > > > -
> > > > > - g_return_val_if_fail(dev, FALSE);
> > > > > - g_return_val_if_fail(udevinfo, FALSE);
> > > > > + struct {
> > > > > + uint8_t bus;
> > > > > + uint8_t addr;
> > > > > + } a_info, b_info, *ai = &a_info, *bi = &b_info;
> > > >
> > > > No need for this intermediate structure or pointers:
> > > >
> > > > a_bus = libusb_get_bus_number(dev);
> > > > b_bus = libusb_get_bus_number(dev);
> > > > a_addr = ...
> > > > b_addr = ...
> > > >
> > >
> > > This is done to make the patch more readable.
> > > If changing this is mandatory, please let me know.
> >
> > The resulting code is harder to read, and I think this only saves a
> > few lines in this patch, so please change it.
> >
> > > >
> > > > > + found->data = dev->data;
> > > > > + dev->data = temp;
> > > >
> > > > Looking at handle_dev_change for some context, this code is triggered
> > > > there:
> > > > notify_dev_state_change(self, now_devs, priv->udev_list, TRUE);
> > > >
> > > > /* keep most recent info: free previous list, and keep current
> > > > list */
> > > > g_udev_client_free_device_list(&priv->udev_list);
> > > > priv->udev_list = now_devs;
> > > >
> > > >
> > > > 'found->data' would be coming from 'priv->udev_list' and 'dev->data'
> > > > from 'now_devs'
> > > > After this code runs, we'll be keeping the 'now_devs' list so when we
> > > > get a device change notification, if we have a device which we already
> > > > knew of (same bus/addr), but represented by a potentially different
> > > > libusb_device *, we'll be keeping the old libusb_device *, not the
> > > > newer
> > > > one.
> > > > This has me worried as we are removing a comment saying:
> > > >
> > > > * On win32 we need to do this the hard and slow way, by asking
> > > > libusb to
> > > > * re-enumerate all devices and then finding a matching device.
> > > > * We cannot cache the libusb_device like we do under Linux since
> > > > the
> > > > * driver swap we do under windows invalidates the cached libdev.
> > > > */
> > > >
> > > > and I initially thought this code was precisely fixing that issue.
> > > > Do you remember why you added this 'else if (add)'?
> > >
> > > The goal is to keep this device list and device list in
> > > usb-device-manager fully
> > > synchronized.
> >
> > I guess this should be explicitly mentioned somewhere, this was not
> > obvious to me during the review that this was one of the goals of these
> > patches, andt his helps understand this hunk of code.
> >
> > > If due to some misunderstanding there is the case with the same
> > > bus:addr but different devices, also previous code will not detect it
> > > and will not signal
> > > device change to the usb-device-manager. This can be improved by checking
> > > also vendor:product. But the goal of this patch is not to improve
> > > existing code, but
> > > use persistent libusb_device.
> >
> >
> > > As far as I understand things - nothing can _invalidate_ referenced
> > > libdev.
> >
> > And this is the key question, why did the comment I mentioned above say
> > something different? Can we still hit the case situation described in
> > that comment? I believe the libusb_device * instance itself was still
> > valid, however trying to communicate with the usb device through it was
> > throwing errors.
> >
> > Christophe
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list