[Spice-devel] [spice-gtk Win32 v4 03/17] Make SpiceUsbDevice a box for SpiceUsbDeviceInfo, instead of a box for libusb_device

[Hans de Goede]

Hi,

On 07/05/2012 10:43 PM, Uri Lublin wrote:
> Note that this change may affect performance a bit, as sometimes there is
> a need to find the libusb_device or the SpiceUsbDevice. Likely it's negligible.
> ---
>   gtk/channel-usbredir.c        |    2 +-
>   gtk/usb-device-manager-priv.h |   10 ++-
>   gtk/usb-device-manager.c      |  188 +++++++++++++++++++++++++++++-----------
>   3 files changed, 146 insertions(+), 54 deletions(-)
>
> diff --git a/gtk/channel-usbredir.c b/gtk/channel-usbredir.c
> index 3d57152..354d2e1 100644
> --- a/gtk/channel-usbredir.c
> +++ b/gtk/channel-usbredir.c

<snip>

> @@ -549,16 +557,18 @@ static void spice_usb_device_manager_auto_connect_cb(GObject      *gobject,
>           g_signal_emit(self, signals[AUTO_CONNECT_FAILED], 0, device, err);
>           g_error_free(err);
>       }
> -    libusb_unref_device(device);
> +    spice_usb_device_unref(device);
>   }
>
>   static void spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
>                                                GUdevDevice            *udev)
>   {
>       SpiceUsbDeviceManagerPrivate *priv = self->priv;
> -    libusb_device *device = NULL, **dev_list = NULL;
> +    libusb_device **dev_list = NULL;
> +    SpiceUsbDevice *device = NULL;
>       const gchar *devtype, *devclass;
>       int i, bus, address;
> +    gboolean filter_ok = FALSE;
>
>       devtype = g_udev_device_get_property(udev, "DEVTYPE");
>       /* Check if this is a usb device (and not an interface) */
> @@ -583,11 +593,19 @@ static void spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
>       for (i = 0; dev_list && dev_list[i]; i++) {
>           if (libusb_get_bus_number(dev_list[i]) == bus &&
>               libusb_get_device_address(dev_list[i]) == address) {
> -            device = libusb_ref_device(dev_list[i]);
> +            device = (SpiceUsbDevice*)spice_usb_device_set_info(dev_list[i]);
>               break;
>           }
>       }
>
> +    if (device && priv->auto_connect) {
> +        /* check filter before unref'ing dev_list[i] */
> +        filter_ok = usbredirhost_check_device_filter(
> +                                priv->auto_conn_filter_rules,
> +                                priv->auto_conn_filter_rules_count,
> +                                dev_list[i], 0) == 0;
> +    }
> +
>       if (!priv->coldplug_list)
>           libusb_free_device_list(dev_list, 1);
>
> @@ -600,21 +618,17 @@ static void spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
>       g_ptr_array_add(priv->devices, device);
>
>       if (priv->auto_connect) {
> -        gboolean can_redirect, auto_ok;
> +        gboolean can_redirect;
>
>           can_redirect = spice_usb_device_manager_can_redirect_device(
> -                                        self, (SpiceUsbDevice *)device, NULL);
> -
> -        auto_ok = usbredirhost_check_device_filter(
> -                            priv->auto_conn_filter_rules,
> -                            priv->auto_conn_filter_rules_count,
> -                            device, 0) == 0;
> +                                        self, device, NULL);
>
> -        if (can_redirect && auto_ok)
> +        if (can_redirect && filter_ok) {
>               spice_usb_device_manager_connect_device_async(self,
> -                                   (SpiceUsbDevice *)device, NULL,
> +                                   device, NULL,
>                                      spice_usb_device_manager_auto_connect_cb,
> -                                   libusb_ref_device(device));
> +                                   g_object_ref(device));
> +        }
>       }
>
>       SPICE_DEBUG("device added %p", device);

The g_object_ref here must be a spice_usb_device_ref!!

<snip>

> @@ -980,13 +1005,20 @@ spice_usb_device_manager_can_redirect_device(SpiceUsbDeviceManager  *self,
>           g_ptr_array_index(priv->channels, 0),
>           &guest_filter_rules, &guest_filter_rules_count);
>
> -    if (guest_filter_rules &&
> -            usbredirhost_check_device_filter(
> +    if (guest_filter_rules) {
> +        gboolean filter_ok;
> +        libusb_device *ldev;
> +        ldev = spice_usb_device_manager_device_to_libdev(self, device);
> +        g_return_val_if_fail(ldev != NULL, FALSE);
> +        filter_ok = (usbredirhost_check_device_filter(
>                               guest_filter_rules, guest_filter_rules_count,
> -                            (libusb_device *)device, 0) != 0) {
> -        g_set_error_literal(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> -                            _("Some USB devices are blocked by host policy"));
> -        return FALSE;
> +                            ldev, 0) == 0);
> +        libusb_unref_device(ldev);
> +        if (!filter_ok) {
> +            g_set_error_literal(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> +                                _("Some USB devices are blocked by host policy"));
> +            return FALSE;
> +        }
>       }
>
>       /* Check if there are free channels */

You're using libdev for the actual libusb_device everywhere and now here you are using ldev, please
make it libdev for consistency.

Regards,

Hans