On Sun, Mar 10, 2019 at 04:46:07PM +0200, Yuri Benditovich wrote:
> Changing signal definition from (boxed-boxed) to (pointer,int).
> There is no need for additional referencing of GUdevDevice
> object before signal callback.
I still feel it would be nicer to guarantee the GUdevDevice will stay
alive for the whole duration of the signal emission. For example, 2
callbacks may be attached to this signal, we don't want the first one to
be able to drop the last ref to the GUdevDevice and risking the second
one trying to use freed memory.
> Second parameter (action) is 0 for device removal and 1 for device
> addition.
Imo this should either be a gboolean or an enum.
>
> Signed-off-by: Yuri Benditovich <yuri.benditovich@xxxxxxxxxx>
> ---
> src/usb-device-manager.c | 8 ++++----
> src/win-usb-dev.c | 18 +++++++++---------
> src/win-usb-dev.h | 2 +-
> 3 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index f7b43f0..c1a0c92 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -153,8 +153,8 @@ static void channel_event(SpiceChannel *channel, SpiceChannelEvent event,
> gpointer user_data);
> #ifdef G_OS_WIN32
> static void spice_usb_device_manager_uevent_cb(GUdevClient *client,
> - const gchar *action,
> GUdevDevice *udevice,
> + int add,
> gpointer user_data);
> static void spice_usb_device_manager_add_udev(SpiceUsbDeviceManager *self,
> GUdevDevice *udev);
> @@ -1070,15 +1070,15 @@ static void spice_usb_device_manager_remove_udev(SpiceUsbDeviceManager *self,
> }
>
> static void spice_usb_device_manager_uevent_cb(GUdevClient *client,
> - const gchar *action,
> GUdevDevice *udevice,
> + int add,
> gpointer user_data)
> {
> SpiceUsbDeviceManager *self = SPICE_USB_DEVICE_MANAGER(user_data);
>
> - if (g_str_equal(action, "add"))
> + if (add)
> spice_usb_device_manager_add_udev(self, udevice);
> - else if (g_str_equal (action, "remove"))
> + else
> spice_usb_device_manager_remove_udev(self, udevice);
> }
> #else
> diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> index 2c122cf..c74dd57 100644
> --- a/src/win-usb-dev.c
> +++ b/src/win-usb-dev.c
> @@ -249,7 +249,7 @@ static void g_udev_client_initable_iface_init(GInitableIface *iface)
>
> static void report_one_device(gpointer data, gpointer self)
> {
> - g_signal_emit(self, signals[UEVENT_SIGNAL], 0, "add", data);
> + g_signal_emit(self, signals[UEVENT_SIGNAL], 0, data, TRUE);
Ok, so the signal would use a gboolean
> }
>
> void g_udev_client_report_devices(GUdevClient *self)
> @@ -342,11 +342,11 @@ static void g_udev_client_class_init(GUdevClientClass *klass)
> G_SIGNAL_RUN_FIRST,
> G_STRUCT_OFFSET(GUdevClientClass, uevent),
> NULL, NULL,
> - g_cclosure_user_marshal_VOID__BOXED_BOXED,
> + g_cclosure_user_marshal_VOID__POINTER_INT,
This can be g_cclosure_user_marshal_VOID__POINTER_BOOLEAN once
src/spice-marshal.txt has VOID:POINTER,BOOLEAN
> G_TYPE_NONE,
> 2,
> - G_TYPE_STRING,
> - G_UDEV_TYPE_DEVICE);
> + G_TYPE_POINTER,
> + G_TYPE_INT);
>
> /**
> * GUdevClient::redirecting:
> @@ -408,15 +408,15 @@ static gint gudev_devices_differ(gconstpointer a, gconstpointer b)
> static void notify_dev_state_change(GUdevClient *self,
> GList *old_list,
> GList *new_list,
> - const gchar *action)
> + int add)
> {
> GList *dev;
>
> for (dev = g_list_first(old_list); dev != NULL; dev = g_list_next(dev)) {
> if (g_list_find_custom(new_list, dev->data, gudev_devices_differ) == NULL) {
> /* Found a device that changed its state */
> - g_udev_device_print(dev->data, action);
> - g_signal_emit(self, signals[UEVENT_SIGNAL], 0, action, dev->data);
> + g_udev_device_print(dev->data, add ? "add" : "remove");
> + g_signal_emit(self, signals[UEVENT_SIGNAL], 0, dev->data, add);
> }
> }
> }
> @@ -445,10 +445,10 @@ static void handle_dev_change(GUdevClient *self)
> g_udev_device_print_list(priv->udev_list, "handle_dev_change: previous list:");
>
> /* Unregister devices that are not present anymore */
> - notify_dev_state_change(self, priv->udev_list, now_devs, "remove");
> + notify_dev_state_change(self, priv->udev_list, now_devs, FALSE);
>
> /* Register newly inserted devices */
> - notify_dev_state_change(self, now_devs, priv->udev_list, "add");
> + 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(&priv->udev_list);
> diff --git a/src/win-usb-dev.h b/src/win-usb-dev.h
> index bca8285..96725b8 100644
> --- a/src/win-usb-dev.h
> +++ b/src/win-usb-dev.h
> @@ -75,7 +75,7 @@ struct _GUdevClientClass
> GObjectClass parent_class;
>
> /* signals */
> - void (*uevent)(GUdevClient *client, const gchar *action, GUdevDevice *device);
> + void (*uevent)(GUdevClient *client, GUdevDevice *device, int add);
> };
>
> GType g_udev_client_get_type(void) G_GNUC_CONST;
> --
> 2.17.1
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel