On Fri, Feb 22, 2019 at 2:06 PM Christophe Fergeau <cfergeau@xxxxxxxxxx> 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@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, ...).
>
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?
> > > > -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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel