On Fri, Feb 09, 2018 at 12:00:47PM -0600, Jeffrey Smith wrote:
> > +       if (!devnode) {
> > +               log_info(libinput, "%s: no device node associated\n", 
> > sysname);
> > +               return NULL;
> > +       }
> > +
> >         if (udev_device_should_be_ignored(udev_device)) {
> >                 log_debug(libinput, "%s: device is ignored\n", sysname);
> >                 return NULL;
> 
> As long as you are intentionally wanting this reported even if the
> device "should be ignored", then it LGTM.

I think that's a good idea, it'll help us figure out where the issue is
coming from. udev_device_should_be_ignored() is only true where
LIBINPUT_IGNORE_DEVICE is set and that's a niche-case to start with.

> > +       if (devnode == NULL)
> > +               return -ENODEV;
> > +
> 
> Checking (!devnode) would be better as it matches the other change,
> and in evdev.c implicit NULL comparison is much more common than
> explicit NULL comparison.

fixed, thanks.
 
> AFAICT, the return value from evdev_device_resume is never checked, so
> it seems that this may be a good place for a log message too.  Would
> it be noisy?  If so, maybe a log_debug would be enough.

It shouldn't be too noisy but I don't think there's much of a need for it.
You're resuming a device here that has since gone away (in a manner that I
can't figure out or reproduce though...).  Which means that you should get
the udev event to properly remove the device soon. Extra logs aren't really
needed here, IMO, this just papers over what is likely a short-window race
condition.

Cheers,
   Peter
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Reply via email to