[Spice-devel] [spice-gtk Win32 PATCH 3/7] Windows: add win-usb-dev.[ch]: implement GUdevDevice & GUdevClient
Arnon Gilboa
agilboa at redhat.com
Tue May 8 01:55:05 PDT 2012
Thanks for the review.
Great comments which surely help a GObject/Gtk-dummy like myself ;)
See below.
Marc-André Lureau wrote:
> Hi
>
> On Mon, May 7, 2012 at 3:15 PM, Uri Lublin <uril at redhat.com> wrote:
>
>> From: Arnon Gilboa <agilboa at redhat.com>
>>
>> With this patch usb-device-manager can work with little change
>> on windows too.
>>
>> -add uevent signal based on WM_DEVICECHANGE
>> -add spice_usb_device_manager_set_window for setting winproc
>> (which is needed for event registration)
>>
>
> You should document any added public API.
> Can't we create a private/hidden window for that?
>
Right, will be nicer & cleaner. Will do that using Win32 API, seems ok?
>
>> +WIN_USB_HDRS = \
>> + win-usb-dev.h \
>> + $(NULL)
>> +
>>
>
> Perhaps the header doesn't need to be conditionally excluded from the
> list of files.
> Also if it's not in the public API, it shouldn't be in the
> libspice_client_glibinclude_HEADERS.
>
ok
>
>> +WIN_USB_LIBS = $(GTK_LIBS)
>>
>
> So the spice-glib lib will depend on Gtk? hmm.. I wish we can find
> something better.
>
Right. The hidden window will remove this too.
>
>> @@ -566,6 +586,7 @@ glib_introspection_files = \
>> channel-usbredir.c \
>> smartcard-manager.c \
>> usb-device-manager.c \
>> + $(WIN_USB_SRCS) \
>>
>
> Does that file need to be introspected? If it doesn't provide a public
> API, I guess not.
>
right
>
>> +#ifdef __linux__
>> #include <gudev/gudev.h>
>> +#elif WIN32
>> +#include "win-usb-dev.h"
>> +#endif
>>
>
> We generally prefer the glib defines G_OS_WIN32 and G_OS_UNIX.
>
> In this case USE_GUDEV could also be more appropriate.
>
ok
>
>> +void spice_usb_device_manager_set_window(SpiceUsbDeviceManager *manager, GdkWindow *win)
>> +{
>> +#ifdef WIN32
>> + g_udev_client_win_init(manager->priv->udev, manager, win);
>> +#endif
>> +}
>>
>
> Please document that API, specify when it should be called, and what
> kind of window.
> Is it safe to be called multiple time, with NULL argument etc.. It
> needs to be annotated for introspection bindings to work correctly.
>
will be done
>
>> +gboolean get_usb_dev_info(libusb_device *dev, GUdevDeviceInfo *usbdev);
>>
>
> Why is this function not static?
>
sure
>
>> +static GObject *g_udev_client_constructor(GType gtype, guint n_properties,
>> + GObjectConstructParam *properties)
>> +{
>> + GObject *obj = G_OBJECT_CLASS(g_udev_client_parent_class)->constructor(
>> + gtype, n_properties, properties);
>> + return obj;
>> +}
>>
>
> This doesn't need to be overriden.
>
ok
>
>> +
>> +static void g_udev_client_init(GUdevClient *self)
>> +{
>> + GUdevClientPrivate *priv;
>> +
>> + self->priv = priv = G_UDEV_CLIENT_GET_PRIVATE(self);
>> + priv->usb_dev_mgr = NULL;
>> + priv->dev_list = NULL;
>> + priv->dev_list_size = 0;
>> + priv->udevice_list = NULL;
>> + priv->gdk_win = NULL;
>>
>
> fields are already set to NULL after creation, you can get rid of this
> init() override.
>
>
right
>> +static void g_udev_client_finalize(GObject *gobject)
>> +{
>> + GUdevClient *self = G_UDEV_CLIENT(gobject);
>> + GUdevClientPrivate *priv = self->priv;
>> +
>> + if (priv->usb_dev_mgr || priv->gdk_win) {
>> + gdk_window_remove_filter(priv->gdk_win, win_message_cb, priv->usb_dev_mgr);
>> + g_list_free_full(priv->udevice_list, g_free);
>> + libusb_free_device_list(priv->dev_list, 1);
>> + libusb_exit(NULL);
>> + }
>>
>
> This looks prone to leaks if the finalize is called in an object
> intermediate state. Instead, it's recommended to check/free each
> pointer individually:
>
> if (priv->ptr)
> free_or_unref(priv->ptr)
> ...
>
> It's also sometime preferable to do all of this in _dispose() and set
> the pointers to NULL. This can solves some cyclic references, although
> perhaps not necessary here.
>
> What is the effect of calling libusb_exit(NULL); ? (it's already
> called by spice_usb_device_manager_finalize)
>
ok. Will add context to init/exit. Is it right to use the context from
SpiceUsbDeviceManagerPrivate ?
>
>> +
>> +gboolean get_usb_dev_info(libusb_device *dev, GUdevDeviceInfo *usbdev)
>> +{
>> + if (!usbdev) {
>> + return FALSE;
>> + }
>>
>
> There is a convenient function g_return_val_if_fail(usbdev != NULL,
> FALSE) for that. It will log if that condition happen too.
>
right
>
>> + if (libusb_get_device_descriptor(dev, &usbdev->desc) < 0) {
>> + return FALSE;
>> + }
>>
>
> It might be worth to add some SPICE_DEBUG?
>
sure
>
>> + usbdev->dev = libusb_ref_device(dev);
>> + sprintf(usbdev->sclass, "%d", usbdev->desc.bDeviceClass);
>> + sprintf(usbdev->sbus, "%d", libusb_get_bus_number(dev));
>> + sprintf(usbdev->saddr, "%d", libusb_get_device_address(dev));
>> + return TRUE;
>>
>
> In general snprintf() is preferred.
>
right
>
>> + for (dev = (dev_change > 0 ? devs : priv->dev_list); *dev && !changed_dev; dev++) {
>> + for (d = (dev_change > 0 ? priv->dev_list : devs); *d && *d != *dev; d++);
>>
>
> You could break inside the loop instead of having complex conditions.
>
ok
>
>> + if (!*d) {
>> + changed_dev = *dev;
>> + }
>> + }
>>
>
>
>> + if (!changed_dev) {
>> + goto leave;
>> + }
>>
>
> if dev_change != 0, shouldn't it also warn it couldn't find any device?
>
right
>
>> + if (dev_change > 0) {
>> + usbdev = g_new(GUdevDeviceInfo, 1);
>> + get_usb_dev_info(changed_dev, usbdev);
>> + udevice = g_udev_device_new(usbdev);
>> + priv->udevice_list = g_list_append(priv->udevice_list, udevice);
>> + SPICE_DEBUG(">>> device inserted: %04x:%04x (bus %s, device %s)\n",
>> + usbdev->desc.idVendor, usbdev->desc.idProduct, usbdev->sbus, usbdev->saddr);
>> + g_signal_emit(self, signals[UEVENT_SIGNAL], 0, "add", udevice);
>> + } else {
>> + dev_found = FALSE;
>> + for (GList *it = g_list_first(priv->udevice_list); it != NULL && !dev_found; it = g_list_next(it)) {
>> + udevice = (GUdevDevice*)it->data;
>> + usbdev = udevice->priv->usbdev;
>> + dev_found = (usbdev->dev == changed_dev);
>> + }
>> + if (!dev_found) {
>> + goto leave;
>> + }
>> + SPICE_DEBUG("<<< device removed: %04x:%04x (bus %s, device %s)\n",
>> + usbdev->desc.idVendor, usbdev->desc.idProduct, usbdev->sbus, usbdev->saddr);
>> + g_signal_emit(self, signals[UEVENT_SIGNAL], 0, "remove", udevice);
>> + priv->udevice_list = g_list_remove(priv->udevice_list, udevice);
>> + g_object_unref(udevice);
>> + }
>>
>
>
>> + if (priv->usb_dev_mgr || priv->gdk_win || !manager || !win) {
>> + return FALSE;
>> + }
>>
>
> g_return_val_if_fail on each condition (one line for each, so we can
> separate failure for each condition if it occurs)
>
ok
>
>
>> +static GUdevDevice *g_udev_device_new(GUdevDeviceInfo *usbdev)
>> +{
>>
>
> Could be worth adding g_return_val_if_fail (usbdev != NULL, NULL)
>
ok
>
>> + GUdevDevice *device = G_UDEV_DEVICE(g_object_new(G_UDEV_TYPE_DEVICE, NULL));
>> +
>> + device->priv->usbdev = usbdev;
>> + return device;
>> +}
>>
>
>
>
>> +const gchar *g_udev_device_get_property(GUdevDevice *udev, const gchar *property)
>> +{
>> + GUdevDeviceInfo* usbdev = udev->priv->usbdev;
>> +
>>
> Checking argument could be worth:
> g_return_val_if_fail(G_UDEV_IS_DEVICE(dev)..)
> g_return_val_if_fail(property != NULL)
>
ok
>
>> + if (strcmp(property, "BUSNUM") == 0) {
>>
>
> We generally prefer g_strcmp0() , a bit safer.
>
ok
>
>> + return usbdev->sbus;
>> + } else if (strcmp(property, "DEVNUM") == 0) {
>> + return usbdev->saddr;
>> + } else if (strcmp(property, "DEVTYPE") == 0) {
>> + return "usb_device";
>> + }
>>
>
> If the value is different it would be worth to warn with
> g_warn_if_reached() for example.
>
ok
>
>> +const gchar *g_udev_device_get_sysfs_attr(GUdevDevice *udev, const gchar *attr)
>> +{
>>
>
> same comments as previous function
>
>
>
ok & thanks again,
Arnon
More information about the Spice-devel
mailing list