[Spice-devel,spice-gtk,v1,3/5] usb-device-manager: handling failure from GUdevClient

Submitted by Victor Toso on Nov. 14, 2016, 11:26 p.m.

Details

Message ID 20161114232655.32459-3-victortoso@redhat.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Nov. 14, 2016, 11:26 p.m.
From: Victor Toso <me@victortoso.com>

g_udev_client_new() can fail in more then one situation in its
initable_init() method.

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 src/usb-device-manager.c | 4 ++++
 1 file changed, 4 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
index 3d4bd98..ae73a02 100644
--- a/src/usb-device-manager.c
+++ b/src/usb-device-manager.c
@@ -321,6 +321,10 @@  static gboolean spice_usb_device_manager_initable_init(GInitable  *initable,
     /* Start listening for usb devices plug / unplug */
 #ifdef USE_GUDEV
     priv->udev = g_udev_client_new(subsystems, err);
+    if (priv->udev == NULL) {
+        g_warning("Error initializing GUdevClient");
+        return FALSE;
+    }
     g_signal_connect(G_OBJECT(priv->udev), "uevent",
                      G_CALLBACK(spice_usb_device_manager_uevent_cb), self);
     /* Do coldplug (detection of already connected devices) */

Comments

Shortlog: "Handle failures from g_udev_client_new()" or "Handle failed
creation of GUdevClient"

On Tue, Nov 15, 2016 at 12:26:53AM +0100, Victor Toso wrote:
> From: Victor Toso <me@victortoso.com>
> 
> g_udev_client_new() can fail in more then one situation in its
> initable_init() method.

"more *than* one situation"
However I'm not sure I understand why it's important that there are
multiple failures possible in initable_init()? Your point is that it can
fail, right? I got distracted by the "in more than one situation".
I would not go into details, the fact that we are using GInitable means
that we should handle initialization failures.

> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  src/usb-device-manager.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index 3d4bd98..ae73a02 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -321,6 +321,10 @@ static gboolean spice_usb_device_manager_initable_init(GInitable  *initable,
>      /* Start listening for usb devices plug / unplug */
>  #ifdef USE_GUDEV
>      priv->udev = g_udev_client_new(subsystems, err);
> +    if (priv->udev == NULL) {
> +        g_warning("Error initializing GUdevClient");
> +        return FALSE;
> +    }

Most of the time the error is ignored by the callers, so I would log
err->message in the g_warning() (being careful that *err can be NULL).

Acked-by: Christophe Fergeau <cfergeau@redhat.com>

Christophe
Hi,

On Tue, Nov 15, 2016 at 11:53:41AM +0100, Christophe Fergeau wrote:
> Shortlog: "Handle failures from g_udev_client_new()" or "Handle failed
> creation of GUdevClient"
>
> On Tue, Nov 15, 2016 at 12:26:53AM +0100, Victor Toso wrote:
> > From: Victor Toso <me@victortoso.com>
> > 
> > g_udev_client_new() can fail in more then one situation in its
> > initable_init() method.
>
> "more *than* one situation"

Ops :)

> However I'm not sure I understand why it's important that there are
> multiple failures possible in initable_init()? Your point is that it can
> fail, right? I got distracted by the "in more than one situation".
> I would not go into details, the fact that we are using GInitable means
> that we should handle initialization failures.

Agreed. Maybe the shortlog is enough.
>
> > 
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> >  src/usb-device-manager.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> > index 3d4bd98..ae73a02 100644
> > --- a/src/usb-device-manager.c
> > +++ b/src/usb-device-manager.c
> > @@ -321,6 +321,10 @@ static gboolean spice_usb_device_manager_initable_init(GInitable  *initable,
> >      /* Start listening for usb devices plug / unplug */
> >  #ifdef USE_GUDEV
> >      priv->udev = g_udev_client_new(subsystems, err);
> > +    if (priv->udev == NULL) {
> > +        g_warning("Error initializing GUdevClient");
> > +        return FALSE;
> > +    }
>
> Most of the time the error is ignored by the callers, so I would log
> err->message in the g_warning() (being careful that *err can be NULL).

err is NULL and it will need more changes to make this look nice as the
err comes from spice_usb_device_manager_get() which is public API an
both spicy and remote-viewer calls with NULL as err!!!

What I'll do for this moment is to improve the error message in
g_udev_client_list_devices() to include the output from
spice_usbutil_libusb_strerror() in a new PATCH.

Of course, I agree with you here and I'll include the (*err)->message if
any in this warning.

I'll send a v2 for this and the next patch.

>
> Acked-by: Christophe Fergeau <cfergeau@redhat.com>
>
> Christophe

Many thanks for the super quick review!

  toso
On Tue, Nov 15, 2016 at 01:56:12PM +0100, Victor Toso wrote:
> Hi,
> 
> On Tue, Nov 15, 2016 at 11:53:41AM +0100, Christophe Fergeau wrote:
> > Shortlog: "Handle failures from g_udev_client_new()" or "Handle failed
> > creation of GUdevClient"
> >
> > On Tue, Nov 15, 2016 at 12:26:53AM +0100, Victor Toso wrote:
> > > From: Victor Toso <me@victortoso.com>
> > > 
> > > g_udev_client_new() can fail in more then one situation in its
> > > initable_init() method.
> >
> > "more *than* one situation"
> 
> Ops :)
> 
> > However I'm not sure I understand why it's important that there are
> > multiple failures possible in initable_init()? Your point is that it can
> > fail, right? I got distracted by the "in more than one situation".
> > I would not go into details, the fact that we are using GInitable means
> > that we should handle initialization failures.
> 
> Agreed. Maybe the shortlog is enough.

Or just mention that the creation can fail, so we need to handle it
otherwise we will get a crash or something like that.

Christophe

> >
> > > 
> > > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > > ---
> > >  src/usb-device-manager.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> > > index 3d4bd98..ae73a02 100644
> > > --- a/src/usb-device-manager.c
> > > +++ b/src/usb-device-manager.c
> > > @@ -321,6 +321,10 @@ static gboolean spice_usb_device_manager_initable_init(GInitable  *initable,
> > >      /* Start listening for usb devices plug / unplug */
> > >  #ifdef USE_GUDEV
> > >      priv->udev = g_udev_client_new(subsystems, err);
> > > +    if (priv->udev == NULL) {
> > > +        g_warning("Error initializing GUdevClient");
> > > +        return FALSE;
> > > +    }
> >
> > Most of the time the error is ignored by the callers, so I would log
> > err->message in the g_warning() (being careful that *err can be NULL).
> 
> err is NULL and it will need more changes to make this look nice as the
> err comes from spice_usb_device_manager_get() which is public API an
> both spicy and remote-viewer calls with NULL as err!!!
> 
> What I'll do for this moment is to improve the error message in
> g_udev_client_list_devices() to include the output from
> spice_usbutil_libusb_strerror() in a new PATCH.
> 
> Of course, I agree with you here and I'll include the (*err)->message if
> any in this warning.
> 
> I'll send a v2 for this and the next patch.
> 
> >
> > Acked-by: Christophe Fergeau <cfergeau@redhat.com>
> >
> > Christophe
> 
> Many thanks for the super quick review!
> 
>   toso