Re: [spice-gtk 08/13] usb-redir: change signal prototype of win-usb-dev

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]