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@xxxxxxxxxx> 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, ...).
> > > -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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel