Re: [Spice-devel] [spice-gtk 2/2] win-usb-dev: Use 'stable' libusb_device pointers in GUdevClient::udev_list
Hey, On Wed, Mar 27, 2019 at 10:48:45AM +0200, Yuri Benditovich wrote: > On Tue, Mar 26, 2019 at 3:11 PM Christophe Fergeau > wrote: > > > > On Mon, Mar 25, 2019 at 08:03:05PM +0200, Yuri Benditovich wrote: > > > Yes, it looks like this finally does the same thing. > > > > > From my personal point of view, this is less obvious than previous > > > code, so I would add 2 comments: > > > that we unreference the device from the new list > > > that we add reference to the device from the old list, it will be > > > dereferenced on next line when the old list freed > > > > If the part you find less clear is: > > +libusb_unref_device(dev->data); > > +dev->data = libusb_ref_device(found->data); > > > > Yes, it is. It took me some time to understand the intention, so I suppose > some other (not too smart) readers will also need some thinking and > the comments would make the reading easier. > Again, this is my personal opinion, no objections as soon as the code > does the same thing. And it took me some time to understand the intention of the swap when reviewing your code ;) I changed it back to your initial version, and pushed the whole series! Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk 2/2] win-usb-dev: Use 'stable' libusb_device pointers in GUdevClient::udev_list
On Tue, Mar 26, 2019 at 3:11 PM Christophe Fergeau wrote: > > On Mon, Mar 25, 2019 at 08:03:05PM +0200, Yuri Benditovich wrote: > > Yes, it looks like this finally does the same thing. > > > From my personal point of view, this is less obvious than previous > > code, so I would add 2 comments: > > that we unreference the device from the new list > > that we add reference to the device from the old list, it will be > > dereferenced on next line when the old list freed > > If the part you find less clear is: > +libusb_unref_device(dev->data); > +dev->data = libusb_ref_device(found->data); > Yes, it is. It took me some time to understand the intention, so I suppose some other (not too smart) readers will also need some thinking and the comments would make the reading easier. Again, this is my personal opinion, no objections as soon as the code does the same thing. > I don't really mind changing it back to do the swapping with an > intermediate variable, even if I personally find the unref/ref to be > more readable than the other version ;) > > Christophe ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk 2/2] win-usb-dev: Use 'stable' libusb_device pointers in GUdevClient::udev_list
On Mon, Mar 25, 2019 at 08:03:05PM +0200, Yuri Benditovich wrote: > Yes, it looks like this finally does the same thing. > From my personal point of view, this is less obvious than previous > code, so I would add 2 comments: > that we unreference the device from the new list > that we add reference to the device from the old list, it will be > dereferenced on next line when the old list freed If the part you find less clear is: +libusb_unref_device(dev->data); +dev->data = libusb_ref_device(found->data); I don't really mind changing it back to do the swapping with an intermediate variable, even if I personally find the unref/ref to be more readable than the other version ;) Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk 2/2] win-usb-dev: Use 'stable' libusb_device pointers in GUdevClient::udev_list
Yes, it looks like this finally does the same thing.
From my personal point of view, this is less obvious than previous
code, so I would add 2 comments:
that we unreference the device from the new list
that we add reference to the device from the old list, it will be
dereferenced on next line when the old list freed
On Mon, Mar 25, 2019 at 6:17 PM Christophe Fergeau wrote:
>
> When getting a new list of USB devices after a WM_DEVICECHANGE event,
> libusb_device which were already present before the event may be
> represented by a different instance than the one we got in the new list.
>
> Since other layers of spice-gtk may be using that older instance of
> libusb_device, this commit makes sure that we always keep the older
> instance in GUdevClient::udev_list.
>
> At the moment, this should not be making any difference, but will make
> things more consistent later on.
>
> Based on a patch from Yuri Benditovich.
>
> Signed-off-by: Christophe Fergeau
> ---
>
> Code is a bit longer this way, but imo it makes more sense to modify the
> list when it's being updated, rather than modifying it during
> notification.
>
>
> src/win-usb-dev.c | 22 --
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> index 192ab17f..a16bd3ae 100644
> --- a/src/win-usb-dev.c
> +++ b/src/win-usb-dev.c
> @@ -423,6 +423,25 @@ static gint compare_libusb_devices(gconstpointer a,
> gconstpointer b)
> return (same_bus && same_addr && same_vid && same_pid) ? 0 : -1;
> }
>
> +static void update_device_list(GUdevClient *self, GList *new_device_list)
> +{
> +GList *dev;
> +
> +for (dev = g_list_first(new_device_list); dev != NULL; dev =
> g_list_next(dev)) {
> +GList *found = g_list_find_custom(self->priv->udev_list, dev->data,
> compare_libusb_devices);
> +if (found != NULL) {
> +/* keep old existing libusb_device in the new list, when
> + usb-dev-manager will maintain its own list of libusb_device,
> + these lists will be completely consistent */
> +libusb_unref_device(dev->data);
> +dev->data = libusb_ref_device(found->data);
> +}
> +}
> +
> +g_udev_client_free_device_list(>priv->udev_list);
> +self->priv->udev_list = new_device_list;
> +}
> +
> static void notify_dev_state_change(GUdevClient *self,
> GList *old_list,
> GList *new_list,
> @@ -469,8 +488,7 @@ static void handle_dev_change(GUdevClient *self)
> 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(>udev_list);
> -priv->udev_list = now_devs;
> +update_device_list(self, now_devs);
> }
>
> static LRESULT CALLBACK wnd_proc(HWND hwnd, UINT message, WPARAM wparam,
> LPARAM lparam)
> --
> 2.21.0
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
