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

[Christophe Fergeau]

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 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.

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 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: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20161115/436da9c3/attachment.sig>