Re: [PATCH libinput] evdev: fail before open_restricted if the devnode doesn't exist

[Jeffrey Smith]

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

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

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.

 -- Jeff
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput] evdev: fail before open_restricted if the devnode doesn't exist

[Peter Hutterer]

On Tue, Feb 13, 2018 at 01:43:27PM +1000, Peter Hutterer wrote:
> Checking these bugs before I was about to push:
> 
> On Fri, Feb 09, 2018 at 07:59:08PM +1000, Peter Hutterer wrote:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1536633
> > https://bugzilla.redhat.com/show_bug.cgi?id=1539046
> > https://bugzilla.redhat.com/show_bug.cgi?id=1539783
> > https://bugzilla.redhat.com/show_bug.cgi?id=1540662
> > https://bugs.freedesktop.org/show_bug.cgi?id=104278
>  
> afaict, these should all fixed by cbb4ec1e3e76b64ec53c25036976e0374aaf41de.
> Did you actually have a case of a null path? 

sigh, sorry about the noise. this is a different rotation than the cbb4e
patch fixed. I checked the backtraces with addr2line and yes, the functions
in the backtrace just seem bogus, your patch does address the issue.

pushed as:
   1b21a431..cb186abc  master -> master

Thanks heaps for debugging that one!

Cheers,
   Peter

> > Signed-off-by: Peter Hutterer 
> > ---
> > Can you give this one a try please? Quick fix but it passes the test suite.
> > 
> >  src/evdev.c | 8 
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/src/evdev.c b/src/evdev.c
> > index d1ca243d..d412eeb0 100644
> > --- a/src/evdev.c
> > +++ b/src/evdev.c
> > @@ -1917,6 +1917,11 @@ evdev_device_create(struct libinput_seat *seat,
> > const char *devnode = udev_device_get_devnode(udev_device);
> > const char *sysname = udev_device_get_sysname(udev_device);
> >  
> > +   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;
> > @@ -2434,6 +2439,9 @@ evdev_device_resume(struct evdev_device *device)
> > return -ENODEV;
> >  
> > devnode = udev_device_get_devnode(device->udev_device);
> > +   if (devnode == NULL)
> > +   return -ENODEV;
> > +
> > fd = open_restricted(libinput, devnode,
> >  O_RDWR | O_NONBLOCK | O_CLOEXEC);
> >  
> > -- 
> > 2.14.3
> > 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput] evdev: fail before open_restricted if the devnode doesn't exist

[Peter Hutterer]

Checking these bugs before I was about to push:

On Fri, Feb 09, 2018 at 07:59:08PM +1000, Peter Hutterer wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1536633
> https://bugzilla.redhat.com/show_bug.cgi?id=1539046
> https://bugzilla.redhat.com/show_bug.cgi?id=1539783
> https://bugzilla.redhat.com/show_bug.cgi?id=1540662
> https://bugs.freedesktop.org/show_bug.cgi?id=104278
 
afaict, these should all fixed by cbb4ec1e3e76b64ec53c25036976e0374aaf41de.
Did you actually have a case of a null path? 

Cheers,
   Peter

> Signed-off-by: Peter Hutterer 
> ---
> Can you give this one a try please? Quick fix but it passes the test suite.
> 
>  src/evdev.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/evdev.c b/src/evdev.c
> index d1ca243d..d412eeb0 100644
> --- a/src/evdev.c
> +++ b/src/evdev.c
> @@ -1917,6 +1917,11 @@ evdev_device_create(struct libinput_seat *seat,
>   const char *devnode = udev_device_get_devnode(udev_device);
>   const char *sysname = udev_device_get_sysname(udev_device);
>  
> + 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;
> @@ -2434,6 +2439,9 @@ evdev_device_resume(struct evdev_device *device)
>   return -ENODEV;
>  
>   devnode = udev_device_get_devnode(device->udev_device);
> + if (devnode == NULL)
> + return -ENODEV;
> +
>   fd = open_restricted(libinput, devnode,
>O_RDWR | O_NONBLOCK | O_CLOEXEC);
>  
> -- 
> 2.14.3
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput] evdev: fail before open_restricted if the devnode doesn't exist

[Peter Hutterer]

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