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

[Victor Toso]

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 at 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 at 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 at redhat.com>
>
> Christophe

Many thanks for the super quick review!

  toso
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20161115/e936da1f/attachment.sig>