[Spice-devel] [spice-gtk Win32 v2 PATCH 3/5] Windows mingw: usb: add win-usb-dev.[ch]: implement GUdevDevice & GUdevClient

[Arnon Gilboa]

Hi
Thanks for the helpful comments. See below.

Marc-André Lureau wrote:
> Hi
>
>   
>> +WIN_USB_FILES= \
>> +       win-usb-dev.h                   \
>> +       win-usb-dev.c                   \
>> +       usbclerk.h                      \
>> +       $(NULL)
>> +
>> +if OS_WIN32
>> +libspice_client_glib_2_0_la_SOURCES += \
>> +       $(WIN_USB_FILES)
>> +else
>> +EXTRA_DIST += $(WIN_USB_FILES)
>> +endif
>>     
>
> usbclerk.h is missing from this patch. It might make sense to call it
> win-usb-clerk.h for consistency.
>
>   
sure
> No need for EXTRA_DIST here, automake takes care of that even inside
> conditionals.
>
> Since win-usb-dev.* includes and use heavily libusb, it might make
> sense to add WITH_USBREDIR condition (otherwise build fails there if
> libusb is missing):
>
>
>   
right.
I am working on removal of all libusb dependency in win-usb-dev, which 
cleanups this mess.
>> diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c
>> index 14b60c9..2b6ce28 100644
>> --- a/gtk/usb-device-manager.c
>> +++ b/gtk/usb-device-manager.c
>> @@ -28,7 +28,14 @@
>>  #ifdef USE_USBREDIR
>>  #include <errno.h>
>>  #include <libusb.h>
>> +#if defined(USE_GUDEV)
>>  #include <gudev/gudev.h>
>> +#elif defined(G_OS_WIN32)
>> +#include "win-usb-dev.h"
>> +#else
>> +#warning "Expecting one of G_OS_WIN32 and USE_GUDEV to be defined"
>> +#endif
>>     
>
> adding a few spaces for indentation could be helpful for readibility,
> something like:
>
> # if defined(USE_GUDEV)
> #  include <gudev/gudev.h>
> # elif defined(G_OS_WIN32)
> #  include "win-usb-dev.h"
> ...
>   
ok
>   
>> +static GUdevClient *singletone = NULL;
>>     
> ...
>   
>> +GUdevClient *g_udev_client_new(const gchar* const *subsystems)
>> +{
>> +    if (!singletone) {
>> +        singletone = g_initable_new(G_UDEV_TYPE_CLIENT, NULL, NULL, NULL);
>> +        return singletone;
>> +    } else {
>> +        return g_object_ref(singletone);
>> +    }
>> +}
>>     
>
> Why it has to be a singleton? Is this a Windows limitation? Or is it a
> kind of optimization? Gudev doesn't seem to return a singleton. Also,
> if it can't be a regular life object, there is a safer way of
> allocating it, as shown in the smartcard-manager using GOnce.
>   
It's used mostly for windows "uevent" generation, so I see no reason for 
having multiple instances with their all internal state/allocs etc.
Will use the GOnce.
>   
>> +    /* FIXME: Using context != NULL results in a crash on spice_usb_device_manager_add_dev().
>> +     * The same happens also when using SpiceUsbDeviceManagerPrivate->context and
>> +     * removing the calls to libusb_init & libusb_exit.
>> +     */
>> +    rc = libusb_init(NULL);
>> +    g_return_val_if_fail(rc >= 0, FALSE);
>>     
>
> Could it be because libusb_init is also called from
> usb-device-manager.c? Should it be consolidated somehow?
>   
As noted above, libusb dependency will be removed.


Thanks for the rest, all will be fixed:
>> +    priv->dev_list_size = libusb_get_device_list(NULL, &priv->dev_list);
>> +    g_return_val_if_fail(priv->dev_list_size >= 0, FALSE);
>>     
>
> I would add g_return_val_if_fail(priv->dev_list != NULL, FALSE);
>
>   
>> +    for (dev = priv->dev_list; *dev; dev++) {
>>     
>
> I would add g_return_val_if_fail(dev != NULL, FALSE)
>
>   
>> +        usbdev = g_new(GUdevDeviceInfo, 1);
>>     
>
> it might be safer to use g_new0() (this is a rule of thumb to allow
> easier debugging later on)
>
>   
>> +GList *g_udev_client_query_by_subsystem(GUdevClient *self, const gchar *subsystem)
>> +{
>>     
>
> We should check given arguments:
>
> g_return_val_if_fail(G_IS_UDEV_CLIENT(self), NULL);
> g_return_val_if_fail(g_strncmp0(subsystem, "usb") == 0, NULL);
>
>
>   
>> +    if (dev_change > 0) {
>> +        usbdev = g_new(GUdevDeviceInfo, 1);
>>     
>
> g_new0()
>
>   
>> +        get_usb_dev_info(changed_dev, usbdev);
>> +        udevice = g_udev_device_new(usbdev);
>>     
>
> Wouldn't it make sense to have a the GudevDevice ctor doing all this?
> You would call  g_udev_device_new(libusb_device), and it would
> allocate device_info and call get_usb_dev_info internally.
>
>