[Spice-devel] [PATCH spice-gtk 06/10] Add an USB device manager

[Christophe Fergeau]

On Fri, Aug 12, 2011 at 04:50:33PM +0200, Hans de Goede wrote:
> diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c
> new file mode 100644
> index 0000000..2287383
> --- /dev/null
> +++ b/gtk/usb-device-manager.c
> @@ -0,0 +1,459 @@
> +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> +/*
> +   Copyright (C) 2011 Red Hat, Inc.
> +
> +   Red Hat Authors:
> +   Hans de Goede <hdegoede at redhat.com>
> +
> +   This library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   This library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with this library; if not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +#include "config.h"
> +
> +#include <glib-object.h>
> +
> +#ifdef USE_USBREDIR
> +#include <gusb/gusb-source.h>
> +#include <gusb/gusb-device-list.h>
> +#include "channel-usbredir-priv.h"
> +#endif
> +
> +#include "spice-client.h"
> +#include "spice-marshal.h"
> +
> +#define SPICE_USB_DEVICE_MANAGER_GET_PRIVATE(obj)                                  \
> +    (G_TYPE_INSTANCE_GET_PRIVATE ((obj), SPICE_TYPE_USB_DEVICE_MANAGER, SpiceUsbDeviceManagerPrivate))
> +
> +enum {
> +    PROP_0,
> +    PROP_MAIN_CONTEXT,
> +    PROP_AUTO_CONNECT,
> +};
> +
> +enum
> +{
> +    DEVICE_ADDED_SIGNAL,
> +    DEVICE_REMOVED_SIGNAL,
> +    LAST_SIGNAL,
> +};
> +
> +struct _SpiceUsbDeviceManagerPrivate {
> +    GMainContext *main_context;
> +    gboolean auto_connect;
> +    GError *usb_init_error;
> +#ifdef USE_USBREDIR
> +    GUsbContext *context;
> +    GUsbDeviceList *devlist;
> +    GUsbSource *source;
> +#endif
> +    GPtrArray *devices;
> +    GPtrArray *channels;
> +};
> +
> +#ifdef USE_USBREDIR
> +static void spice_usb_device_manager_dev_added(GUsbDeviceList *devlist,
> +                                               GUsbDevice     *device,
> +                                               GUdevDevice    *udev,
> +                                               gpointer        user_data);
> +static void spice_usb_device_manager_dev_removed(GUsbDeviceList *devlist,
> +                                                 GUsbDevice     *device,
> +                                                 GUdevDevice    *udev,
> +                                                 gpointer        user_data);
> +#endif
> +
> +static guint signals[LAST_SIGNAL] = { 0, };
> +
> +G_DEFINE_TYPE(SpiceUsbDeviceManager, spice_usb_device_manager, G_TYPE_OBJECT)
> +G_DEFINE_BOXED_TYPE(SpiceUsbDevice, spice_usb_device, g_object_ref, g_object_unref)

Why do you introduce this SpiceUsbDevice opaque type? You won't want to
keep the fact that it's a GUsbDevice as an implementation detail so that it
can be changed to an object inheriting from GUsbDevice later?

> +
> +static void spice_usb_device_manager_init(SpiceUsbDeviceManager *manager)
> +{
> +    SpiceUsbDeviceManagerPrivate *priv;
> +
> +    priv = SPICE_USB_DEVICE_MANAGER_GET_PRIVATE(manager);
> +    manager->priv = priv;
> +
> +    priv->main_context = NULL;
> +    priv->channels = g_ptr_array_new();
> +    priv->devices  = g_ptr_array_new_with_free_func((GDestroyNotify)
> +                                                    g_object_unref);

I think I'd have used a GHashTable for the list of devices since
g_ptr_array_remove will have to iterate over the array to find the device
to remove. It could even store the device => channel association, with the
GPtrArray of channels which could become a GList * of non-associated
channels.
However, it's more a matter of personal coding style, your way of doing
this is fine with me too.


> +#ifdef USE_USBREDIR
> +    priv->source = NULL;
> +    priv->devlist = NULL;
> +    priv->usb_init_error = NULL;

For what it's worth, the various fields will be set to NULL by glib when
the object is created.

The rest looked good to me,

Thanks,

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20110817/a0590d2b/attachment.pgp>