On Tue, Mar 12, 2019 at 4:02 PM Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote:
>
> 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.
>
Additional referencing here is not required. The signal callback does not
dereference the GUdevDevice object, so this does not make any difference
how many callbacks attached to the signal.
> > Second parameter (action) is 0 for device removal and 1 for device
> > addition.
>
> Imo this should either be a gboolean or an enum.
I do not see any reason to invent additional marshalling procedure where
existing one can be used without any problem.
>
> >
> > 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
> -----BEGIN PGP SIGNATURE-----
>
> iQIzBAEBCAAdFiEElKn3VmH3emFoZJsjqdjCFCmsbIIFAlyHu/0ACgkQqdjCFCms
> bILnhw/+Nks653B1Drb//RsVGI3bc/f5UKbJMCkZ0UMLg9pbq3kInx9NOYjY7j9S
> hyiiWD3tNzOOxhMZKCh+hfPByeTl8HZVUkMh67XWQG7MOj/eZ/P88k+ym4iWpw7F
> QivAzRUT7ab+z3g7YpYBv3q1EUpeVlRaqQsPTZOfhVdU/aBUGXm0B3T5WDvwZaqp
> /hvMPqsR8QhOHORxgc9Ciqt/mQZSFGhATQ8cPqp3QAi3UogUaqAidL7SUbGTHr+s
> kQXlbp7sE/UU7mb9zOqwKyRx4FHABHnsqhNM12JbkaIwDsp8FYFcG1FO1r5sL6ki
> D3ZuLnkphO1dPVZQKddIjitO3+qUpYD9OzF1bjeUiaR/bMtO9FP9O9CLIVtHZmrx
> pXM+1HJcPSzJAHHyTe0q6p9TS2q/zWoDgpi2NVZ/PB0716zNHKPkAhzMXS1AWPJv
> sT1dVtg2XhFlXbt/d0FLp6W8VijUJjSE5YtPQF6Ka+JvvzelIvtJQqNAosWEuZnc
> W9aCU2HPPSB5qoICMYVzfY+9t+KWfxJR7aJQ6u2GvB52bQ5pGxc/fOVE0EDObVz3
> 4f98AZv6fgu0gCoWJHE8X9x+cu/TYdF/8xHP/zHBq/k86Y87xgURVh9YLBoUfXgY
> LezVr6GsfFM9ZEK8B0eVek8PH0jDjYckX8X0t6q4z9WH5Byre9Q=
> =5UVR
> -----END PGP SIGNATURE-----
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel