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@xxxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx>
> >
> > Christophe
>
> Many thanks for the super quick review!
>
> toso
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel