[PATCH v3] input/uinput: add UI_GET_SYSNAME ioctl to retrieve the sysfs path

[PATCH v3] input/uinput: add UI_GET_SYSNAME ioctl to retrieve the sysfs path

[Benjamin Tissoires]

Evemu [1] uses uinput to replay devices traces it has recorded. However,
the way evemu uses uinput is slightly different from how uinput is
supposed to be used.
Evemu relies on libevdev, which creates the device node through uinput.
It then injects events through the input device node directly (and it
completely skips the uinput node).

Currently, libevdev relies on an heuristic to guess which input node was
created. The problem is that is heuristic is subjected to races between
different uinput devices or even with physical devices. Having a way
to retrieve the sysfs path allows us to find the event node without
having to rely on this heuristic.

[1] http://www.freedesktop.org/wiki/Evemu/

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---

Ok, I am resurrecting this. The original patch was sent last July here:
https://patchwork.kernel.org/patch/2827524/

changes since v2:
- the ioctl returns only the device name, thus I renamed the ioctl to UI_GET_SYSNAME
- reordered uinput_str_to_user() arguments
- be sure to terminate the user string we send by \0
- abort if udev->state is not UIST_CREATED
- dropped the patch 1/2 (adding resolution to uinput) because I think David has
  already it in one of his queues (ABS2 IIRC)

I also posted the corresponding libevdev here:
http://lists.freedesktop.org/archives/input-tools/2014-January/000757.html

Cheers,
Benjamin

 drivers/input/misc/uinput.c | 46 ++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/uinput.h      |  2 ++
 include/uapi/linux/uinput.h | 13 ++++++++++++-
 3 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 7728359..0203219 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -20,6 +20,8 @@
  * Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
  *
  * Changes/Revisions:
+ *	0.4	01/09/2014 (Benjamin Tissoires <benjamin.tissoires@redhat.com>)
+ *		- add UI_GET_SYSNAME ioctl
  *	0.3	09/04/2006 (Anssi Hannula <anssi.hannula@gmail.com>)
  *		- updated ff support for the changes in kernel interface
  *		- added MODULE_VERSION
@@ -670,6 +672,27 @@ static int uinput_ff_upload_from_user(const char __user *buffer,
 	__ret;						\
 })
 
+static int uinput_str_to_user(void __user *dest, const char *str,
+			      unsigned int maxlen)
+{
+	int len, ret;
+
+	if (!str)
+		return -ENOENT;
+
+	len = strlen(str) + 1;
+	if (len > maxlen)
+		len = maxlen;
+
+	ret = copy_to_user(dest, str, len);
+	if (ret)
+		return -EFAULT;
+
+	/* force terminating '\0' */
+	ret = copy_to_user(dest + len - 1, "\0", 1);
+	return ret ? -EFAULT : len;
+}
+
 static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
 				 unsigned long arg, void __user *p)
 {
@@ -679,6 +702,8 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
 	struct uinput_ff_erase  ff_erase;
 	struct uinput_request   *req;
 	char			*phys;
+	const char		*name;
+	unsigned int		size;
 
 	retval = mutex_lock_interruptible(&udev->mutex);
 	if (retval)
@@ -831,7 +856,26 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
 			break;
 
 		default:
-			retval = -EINVAL;
+			retval = -EAGAIN;
+	}
+
+	if (retval == -EAGAIN) {
+		size = _IOC_SIZE(cmd);
+
+		/* Now check variable-length commands */
+		switch (cmd & ~IOCSIZE_MASK) {
+			case UI_GET_SYSNAME(0):
+				if (udev->state != UIST_CREATED) {
+					retval = -ENOENT;
+					goto out;
+				}
+				name = dev_name(&udev->dev->dev);
+				retval = uinput_str_to_user(p, name, size);
+				break;
+
+			default:
+				retval = -EINVAL;
+		}
 	}
 
  out:
diff --git a/include/linux/uinput.h b/include/linux/uinput.h
index 0a4487d..0994c0d 100644
--- a/include/linux/uinput.h
+++ b/include/linux/uinput.h
@@ -20,6 +20,8 @@
  * Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
  *
  * Changes/Revisions:
+ *	0.4	01/09/2014 (Benjamin Tissoires <benjamin.tissoires@redhat.com>)
+ *		- add UI_GET_SYSNAME ioctl
  *	0.3	24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
  *		- update ff support for the changes in kernel interface
  *		- add UINPUT_VERSION
diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
index fe46431..0389b48 100644
--- a/include/uapi/linux/uinput.h
+++ b/include/uapi/linux/uinput.h
@@ -20,6 +20,8 @@
  * Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
  *
  * Changes/Revisions:
+ *	0.4	01/09/2014 (Benjamin Tissoires <benjamin.tissoires@redhat.com>)
+ *		- add UI_GET_SYSNAME ioctl
  *	0.3	24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
  *		- update ff support for the changes in kernel interface
  *		- add UINPUT_VERSION
@@ -35,7 +37,7 @@
 #include <linux/types.h>
 #include <linux/input.h>
 
-#define UINPUT_VERSION		3
+#define UINPUT_VERSION		4
 
 
 struct uinput_ff_upload {
@@ -73,6 +75,15 @@ struct uinput_ff_erase {
 #define UI_BEGIN_FF_ERASE	_IOWR(UINPUT_IOCTL_BASE, 202, struct uinput_ff_erase)
 #define UI_END_FF_ERASE		_IOW(UINPUT_IOCTL_BASE, 203, struct uinput_ff_erase)
 
+/**
+ * UI_GET_SYSNAME - get the sysfs name of the created uinput device
+ *
+ * @return the sysfs name of the created virtual input device.
+ * The complete sysfs path is then /sys/devices/virtual/input/--NAME--
+ * Usually, it is in the form "inputN"
+ */
+#define UI_GET_SYSNAME(len)	_IOC(_IOC_READ, UINPUT_IOCTL_BASE, 300, len)
+
 /*
  * To write a force-feedback-capable driver, the upload_effect
  * and erase_effect callbacks in input_dev must be implemented.
-- 
1.8.3.1

Re: [PATCH v3] input/uinput: add UI_GET_SYSNAME ioctl to retrieve the sysfs path

[David Herrmann]

Hi

On Fri, Jan 17, 2014 at 8:12 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> Evemu [1] uses uinput to replay devices traces it has recorded. However,
> the way evemu uses uinput is slightly different from how uinput is
> supposed to be used.
> Evemu relies on libevdev, which creates the device node through uinput.
> It then injects events through the input device node directly (and it
> completely skips the uinput node).
>
> Currently, libevdev relies on an heuristic to guess which input node was
> created. The problem is that is heuristic is subjected to races between
> different uinput devices or even with physical devices. Having a way
> to retrieve the sysfs path allows us to find the event node without
> having to rely on this heuristic.
>
> [1] http://www.freedesktop.org/wiki/Evemu/
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>
> Ok, I am resurrecting this. The original patch was sent last July here:
> https://patchwork.kernel.org/patch/2827524/
>
> changes since v2:
> - the ioctl returns only the device name, thus I renamed the ioctl to UI_GET_SYSNAME
> - reordered uinput_str_to_user() arguments
> - be sure to terminate the user string we send by \0
> - abort if udev->state is not UIST_CREATED
> - dropped the patch 1/2 (adding resolution to uinput) because I think David has
>   already it in one of his queues (ABS2 IIRC)
>
> I also posted the corresponding libevdev here:
> http://lists.freedesktop.org/archives/input-tools/2014-January/000757.html
>
> Cheers,
> Benjamin
>
>  drivers/input/misc/uinput.c | 46 ++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/uinput.h      |  2 ++
>  include/uapi/linux/uinput.h | 13 ++++++++++++-
>  3 files changed, 59 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 7728359..0203219 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -20,6 +20,8 @@
>   * Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
>   *
>   * Changes/Revisions:
> + *     0.4     01/09/2014 (Benjamin Tissoires <benjamin.tissoires@redhat.com>)
> + *             - add UI_GET_SYSNAME ioctl
>   *     0.3     09/04/2006 (Anssi Hannula <anssi.hannula@gmail.com>)
>   *             - updated ff support for the changes in kernel interface
>   *             - added MODULE_VERSION
> @@ -670,6 +672,27 @@ static int uinput_ff_upload_from_user(const char __user *buffer,
>         __ret;                                          \
>  })
>
> +static int uinput_str_to_user(void __user *dest, const char *str,
> +                             unsigned int maxlen)
> +{
> +       int len, ret;
> +
> +       if (!str)
> +               return -ENOENT;
> +
> +       len = strlen(str) + 1;
> +       if (len > maxlen)
> +               len = maxlen;
> +
> +       ret = copy_to_user(dest, str, len);
> +       if (ret)
> +               return -EFAULT;
> +
> +       /* force terminating '\0' */
> +       ret = copy_to_user(dest + len - 1, "\0", 1);

You must make sure "maxlen != 0", otherwise you write beyond buffer
boundaries here. I'd say return -EINVAL in case maxlen == 0.

btw., you can also use: put_user(0, (char*)dest + len - 1);

> +       return ret ? -EFAULT : len;
> +}
> +
>  static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>                                  unsigned long arg, void __user *p)
>  {
> @@ -679,6 +702,8 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>         struct uinput_ff_erase  ff_erase;
>         struct uinput_request   *req;
>         char                    *phys;
> +       const char              *name;
> +       unsigned int            size;
>
>         retval = mutex_lock_interruptible(&udev->mutex);
>         if (retval)
> @@ -831,7 +856,26 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>                         break;
>
>                 default:
> -                       retval = -EINVAL;
> +                       retval = -EAGAIN;
> +       }
> +
> +       if (retval == -EAGAIN) {

Ehh, in case another ioctl returns -EAGAIN, we overwrite the error
with -EINVAL below in the "default:" clause. Maybe we should first
convert all the "break;" commands to "goto out;".

Patch looks good to me.
Thanks
David

> +               size = _IOC_SIZE(cmd);
> +
> +               /* Now check variable-length commands */
> +               switch (cmd & ~IOCSIZE_MASK) {
> +                       case UI_GET_SYSNAME(0):
> +                               if (udev->state != UIST_CREATED) {
> +                                       retval = -ENOENT;
> +                                       goto out;
> +                               }
> +                               name = dev_name(&udev->dev->dev);
> +                               retval = uinput_str_to_user(p, name, size);
> +                               break;
> +
> +                       default:
> +                               retval = -EINVAL;
> +               }
>         }
>
>   out:
> diff --git a/include/linux/uinput.h b/include/linux/uinput.h
> index 0a4487d..0994c0d 100644
> --- a/include/linux/uinput.h
> +++ b/include/linux/uinput.h
> @@ -20,6 +20,8 @@
>   * Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
>   *
>   * Changes/Revisions:
> + *     0.4     01/09/2014 (Benjamin Tissoires <benjamin.tissoires@redhat.com>)
> + *             - add UI_GET_SYSNAME ioctl
>   *     0.3     24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
>   *             - update ff support for the changes in kernel interface
>   *             - add UINPUT_VERSION
> diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> index fe46431..0389b48 100644
> --- a/include/uapi/linux/uinput.h
> +++ b/include/uapi/linux/uinput.h
> @@ -20,6 +20,8 @@
>   * Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
>   *
>   * Changes/Revisions:
> + *     0.4     01/09/2014 (Benjamin Tissoires <benjamin.tissoires@redhat.com>)
> + *             - add UI_GET_SYSNAME ioctl
>   *     0.3     24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
>   *             - update ff support for the changes in kernel interface
>   *             - add UINPUT_VERSION
> @@ -35,7 +37,7 @@
>  #include <linux/types.h>
>  #include <linux/input.h>
>
> -#define UINPUT_VERSION         3
> +#define UINPUT_VERSION         4
>
>
>  struct uinput_ff_upload {
> @@ -73,6 +75,15 @@ struct uinput_ff_erase {
>  #define UI_BEGIN_FF_ERASE      _IOWR(UINPUT_IOCTL_BASE, 202, struct uinput_ff_erase)
>  #define UI_END_FF_ERASE                _IOW(UINPUT_IOCTL_BASE, 203, struct uinput_ff_erase)
>
> +/**
> + * UI_GET_SYSNAME - get the sysfs name of the created uinput device
> + *
> + * @return the sysfs name of the created virtual input device.
> + * The complete sysfs path is then /sys/devices/virtual/input/--NAME--
> + * Usually, it is in the form "inputN"
> + */
> +#define UI_GET_SYSNAME(len)    _IOC(_IOC_READ, UINPUT_IOCTL_BASE, 300, len)
> +
>  /*
>   * To write a force-feedback-capable driver, the upload_effect
>   * and erase_effect callbacks in input_dev must be implemented.
> --
> 1.8.3.1
>

Re: [PATCH v3] input/uinput: add UI_GET_SYSNAME ioctl to retrieve the sysfs path

[Benjamin Tissoires]

Hi David,

thanks for the review

On Sat, Jan 18, 2014 at 5:51 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Fri, Jan 17, 2014 at 8:12 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> Evemu [1] uses uinput to replay devices traces it has recorded. However,
>> the way evemu uses uinput is slightly different from how uinput is
>> supposed to be used.
>> Evemu relies on libevdev, which creates the device node through uinput.
>> It then injects events through the input device node directly (and it
>> completely skips the uinput node).
>>
>> Currently, libevdev relies on an heuristic to guess which input node was
>> created. The problem is that is heuristic is subjected to races between
>> different uinput devices or even with physical devices. Having a way
>> to retrieve the sysfs path allows us to find the event node without
>> having to rely on this heuristic.
>>
>> [1] http://www.freedesktop.org/wiki/Evemu/
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>>
>> Ok, I am resurrecting this. The original patch was sent last July here:
>> https://patchwork.kernel.org/patch/2827524/
>>
>> changes since v2:
>> - the ioctl returns only the device name, thus I renamed the ioctl to UI_GET_SYSNAME
>> - reordered uinput_str_to_user() arguments
>> - be sure to terminate the user string we send by \0
>> - abort if udev->state is not UIST_CREATED
>> - dropped the patch 1/2 (adding resolution to uinput) because I think David has
>>   already it in one of his queues (ABS2 IIRC)
>>
>> I also posted the corresponding libevdev here:
>> http://lists.freedesktop.org/archives/input-tools/2014-January/000757.html
>>
>> Cheers,
>> Benjamin
>>
>>  drivers/input/misc/uinput.c | 46 ++++++++++++++++++++++++++++++++++++++++++++-
>>  include/linux/uinput.h      |  2 ++
>>  include/uapi/linux/uinput.h | 13 ++++++++++++-
>>  3 files changed, 59 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
>> index 7728359..0203219 100644
>> --- a/drivers/input/misc/uinput.c
>> +++ b/drivers/input/misc/uinput.c
>> @@ -20,6 +20,8 @@
>>   * Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
>>   *
>>   * Changes/Revisions:
>> + *     0.4     01/09/2014 (Benjamin Tissoires <benjamin.tissoires@redhat.com>)
>> + *             - add UI_GET_SYSNAME ioctl
>>   *     0.3     09/04/2006 (Anssi Hannula <anssi.hannula@gmail.com>)
>>   *             - updated ff support for the changes in kernel interface
>>   *             - added MODULE_VERSION
>> @@ -670,6 +672,27 @@ static int uinput_ff_upload_from_user(const char __user *buffer,
>>         __ret;                                          \
>>  })
>>
>> +static int uinput_str_to_user(void __user *dest, const char *str,
>> +                             unsigned int maxlen)
>> +{
>> +       int len, ret;
>> +
>> +       if (!str)
>> +               return -ENOENT;
>> +
>> +       len = strlen(str) + 1;
>> +       if (len > maxlen)
>> +               len = maxlen;
>> +
>> +       ret = copy_to_user(dest, str, len);
>> +       if (ret)
>> +               return -EFAULT;
>> +
>> +       /* force terminating '\0' */
>> +       ret = copy_to_user(dest + len - 1, "\0", 1);
>
> You must make sure "maxlen != 0", otherwise you write beyond buffer
> boundaries here. I'd say return -EINVAL in case maxlen == 0.

oh, yes, you are right.

>
> btw., you can also use: put_user(0, (char*)dest + len - 1);

k, will amend

>
>> +       return ret ? -EFAULT : len;
>> +}
>> +
>>  static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>>                                  unsigned long arg, void __user *p)
>>  {
>> @@ -679,6 +702,8 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>>         struct uinput_ff_erase  ff_erase;
>>         struct uinput_request   *req;
>>         char                    *phys;
>> +       const char              *name;
>> +       unsigned int            size;
>>
>>         retval = mutex_lock_interruptible(&udev->mutex);
>>         if (retval)
>> @@ -831,7 +856,26 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>>                         break;
>>
>>                 default:
>> -                       retval = -EINVAL;
>> +                       retval = -EAGAIN;
>> +       }
>> +
>> +       if (retval == -EAGAIN) {
>
> Ehh, in case another ioctl returns -EAGAIN, we overwrite the error
> with -EINVAL below in the "default:" clause. Maybe we should first
> convert all the "break;" commands to "goto out;".

yep, that was one of the thing which also made me some trouble by
re-reading the patch after this amount of time.
Either I can make a "goto out" patch before, or I can just add a bool
probe_variable_length which would be set be the previous default case.
The latter implies less changes, but might be more ugly than the previous.
Any preferences?


>
> Patch looks good to me.

Thanks!

Cheers,
Benjamin

> Thanks
> David
>
>> +               size = _IOC_SIZE(cmd);
>> +
>> +               /* Now check variable-length commands */
>> +               switch (cmd & ~IOCSIZE_MASK) {
>> +                       case UI_GET_SYSNAME(0):
>> +                               if (udev->state != UIST_CREATED) {
>> +                                       retval = -ENOENT;
>> +                                       goto out;
>> +                               }
>> +                               name = dev_name(&udev->dev->dev);
>> +                               retval = uinput_str_to_user(p, name, size);
>> +                               break;
>> +
>> +                       default:
>> +                               retval = -EINVAL;
>> +               }
>>         }
>>
>>   out:
>> diff --git a/include/linux/uinput.h b/include/linux/uinput.h
>> index 0a4487d..0994c0d 100644
>> --- a/include/linux/uinput.h
>> +++ b/include/linux/uinput.h
>> @@ -20,6 +20,8 @@
>>   * Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
>>   *
>>   * Changes/Revisions:
>> + *     0.4     01/09/2014 (Benjamin Tissoires <benjamin.tissoires@redhat.com>)
>> + *             - add UI_GET_SYSNAME ioctl
>>   *     0.3     24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
>>   *             - update ff support for the changes in kernel interface
>>   *             - add UINPUT_VERSION
>> diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
>> index fe46431..0389b48 100644
>> --- a/include/uapi/linux/uinput.h
>> +++ b/include/uapi/linux/uinput.h
>> @@ -20,6 +20,8 @@
>>   * Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
>>   *
>>   * Changes/Revisions:
>> + *     0.4     01/09/2014 (Benjamin Tissoires <benjamin.tissoires@redhat.com>)
>> + *             - add UI_GET_SYSNAME ioctl
>>   *     0.3     24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
>>   *             - update ff support for the changes in kernel interface
>>   *             - add UINPUT_VERSION
>> @@ -35,7 +37,7 @@
>>  #include <linux/types.h>
>>  #include <linux/input.h>
>>
>> -#define UINPUT_VERSION         3
>> +#define UINPUT_VERSION         4
>>
>>
>>  struct uinput_ff_upload {
>> @@ -73,6 +75,15 @@ struct uinput_ff_erase {
>>  #define UI_BEGIN_FF_ERASE      _IOWR(UINPUT_IOCTL_BASE, 202, struct uinput_ff_erase)
>>  #define UI_END_FF_ERASE                _IOW(UINPUT_IOCTL_BASE, 203, struct uinput_ff_erase)
>>
>> +/**
>> + * UI_GET_SYSNAME - get the sysfs name of the created uinput device
>> + *
>> + * @return the sysfs name of the created virtual input device.
>> + * The complete sysfs path is then /sys/devices/virtual/input/--NAME--
>> + * Usually, it is in the form "inputN"
>> + */
>> +#define UI_GET_SYSNAME(len)    _IOC(_IOC_READ, UINPUT_IOCTL_BASE, 300, len)
>> +
>>  /*
>>   * To write a force-feedback-capable driver, the upload_effect
>>   * and erase_effect callbacks in input_dev must be implemented.
>> --
>> 1.8.3.1
>>

Re: [PATCH v3] input/uinput: add UI_GET_SYSNAME ioctl to retrieve the sysfs path

[Dmitry Torokhov]

Hi Benjamin,

On Fri, Jan 17, 2014 at 02:12:51PM -0500, Benjamin Tissoires wrote:
> Evemu [1] uses uinput to replay devices traces it has recorded. However,
> the way evemu uses uinput is slightly different from how uinput is
> supposed to be used.
> Evemu relies on libevdev, which creates the device node through uinput.
> It then injects events through the input device node directly (and it
> completely skips the uinput node).
> 
> Currently, libevdev relies on an heuristic to guess which input node was
> created. The problem is that is heuristic is subjected to races between
> different uinput devices or even with physical devices. Having a way
> to retrieve the sysfs path allows us to find the event node without
> having to rely on this heuristic.

I have been thinking about it and I think that providing tight coupling
between uinput and resulting event device is wrong thing to do. We do
allow sending input events through uinput interface and I think evemu
should be using it, instead of going halfway through uinput and halfway
though evdev. Replaying though uinput would actually be more correct as
it would involve the same code paths throgugh input core as with using
real devices (see input_event() vs. input_inject_event() that is used by
input handlers).

Thanks.

-- 
Dmitry

Re: [PATCH v3] input/uinput: add UI_GET_SYSNAME ioctl to retrieve the sysfs path

[Benjamin Tissoires]

On Mon, Jan 20, 2014 at 4:53 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Benjamin,
>
> On Fri, Jan 17, 2014 at 02:12:51PM -0500, Benjamin Tissoires wrote:
>> Evemu [1] uses uinput to replay devices traces it has recorded. However,
>> the way evemu uses uinput is slightly different from how uinput is
>> supposed to be used.
>> Evemu relies on libevdev, which creates the device node through uinput.
>> It then injects events through the input device node directly (and it
>> completely skips the uinput node).
>>
>> Currently, libevdev relies on an heuristic to guess which input node was
>> created. The problem is that is heuristic is subjected to races between
>> different uinput devices or even with physical devices. Having a way
>> to retrieve the sysfs path allows us to find the event node without
>> having to rely on this heuristic.
>
> I have been thinking about it and I think that providing tight coupling
> between uinput and resulting event device is wrong thing to do. We do
> allow sending input events through uinput interface and I think evemu
> should be using it, instead of going halfway through uinput and halfway
> though evdev. Replaying though uinput would actually be more correct as
> it would involve the same code paths throgugh input core as with using
> real devices (see input_event() vs. input_inject_event() that is used by
> input handlers).
>

Yes, I am perfectly aware of the fact that evemu is not using uinput
in the way it is intended to be.
I agree that it should be using the uinput node to inject events but
this means that only the process which has created the virtual device
can access it. It seems weird, I know, but the typical use of evemu is
the following:
- in a first terminal: $> sudo evemu-device mydevice.desc
- In a second: $> sudo evemu-play /dev/input/event12 < mydevice.events

It looks weird here, but it allows to inject different events
recording for the same virtual device node. Using the uinput node to
inject events will force us to change the user "interface" and rely on
pipes to get the same separation of describe/inject.
Note that I am modifying evemu-play to be able to also create the
virtual device, so I am not entirely convinced about this argument
(but we have users).

The other use case I should have mentioned in the commit message is
that we extensively rely on evemu for the xorg-integration-tests (and
the upcoming wayland test suite if I am not wrong).
The tests are fully automatized, and we need to know which input node
has just been created to record the correct one and test against it.


Ok, I am stopping here because Peter already answered about this in his mail :)

Cheers,
Benjamin

Re: [PATCH v3] input/uinput: add UI_GET_SYSNAME ioctl to retrieve the sysfs path

[Peter Hutterer]

On Mon, Jan 20, 2014 at 05:17:08PM -0500, Benjamin Tissoires wrote:
> On Mon, Jan 20, 2014 at 4:53 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Hi Benjamin,
> >
> > On Fri, Jan 17, 2014 at 02:12:51PM -0500, Benjamin Tissoires wrote:
> >> Evemu [1] uses uinput to replay devices traces it has recorded. However,
> >> the way evemu uses uinput is slightly different from how uinput is
> >> supposed to be used.
> >> Evemu relies on libevdev, which creates the device node through uinput.
> >> It then injects events through the input device node directly (and it
> >> completely skips the uinput node).
> >>
> >> Currently, libevdev relies on an heuristic to guess which input node was
> >> created. The problem is that is heuristic is subjected to races between
> >> different uinput devices or even with physical devices. Having a way
> >> to retrieve the sysfs path allows us to find the event node without
> >> having to rely on this heuristic.
> >
> > I have been thinking about it and I think that providing tight coupling
> > between uinput and resulting event device is wrong thing to do. We do
> > allow sending input events through uinput interface and I think evemu
> > should be using it, instead of going halfway through uinput and halfway
> > though evdev. Replaying though uinput would actually be more correct as
> > it would involve the same code paths throgugh input core as with using
> > real devices (see input_event() vs. input_inject_event() that is used by
> > input handlers).
> >
> 
> Yes, I am perfectly aware of the fact that evemu is not using uinput
> in the way it is intended to be.
> I agree that it should be using the uinput node to inject events but
> this means that only the process which has created the virtual device
> can access it. It seems weird, I know, but the typical use of evemu is
> the following:
> - in a first terminal: $> sudo evemu-device mydevice.desc
> - In a second: $> sudo evemu-play /dev/input/event12 < mydevice.events
> 
> It looks weird here, but it allows to inject different events
> recording for the same virtual device node. 

it also allows replaying an event through the device it was recorded on.
it's not always necessary or desirable to create a uinput device, sometimes
replaying it through the actual device is better to reproduce a certain bug.

Cheers,
   Peter



> Using the uinput node to
> inject events will force us to change the user "interface" and rely on
> pipes to get the same separation of describe/inject.
> Note that I am modifying evemu-play to be able to also create the
> virtual device, so I am not entirely convinced about this argument
> (but we have users).
> 
> The other use case I should have mentioned in the commit message is
> that we extensively rely on evemu for the xorg-integration-tests (and
> the upcoming wayland test suite if I am not wrong).
> The tests are fully automatized, and we need to know which input node
> has just been created to record the correct one and test against it.
> 
> 
> Ok, I am stopping here because Peter already answered about this in his mail :)
> 
> Cheers,
> Benjamin

Re: [PATCH v3] input/uinput: add UI_GET_SYSNAME ioctl to retrieve the sysfs path

[Dmitry Torokhov]

On Tue, Jan 21, 2014 at 08:56:51AM +1000, Peter Hutterer wrote:
> On Mon, Jan 20, 2014 at 05:17:08PM -0500, Benjamin Tissoires wrote:
> > On Mon, Jan 20, 2014 at 4:53 PM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > Hi Benjamin,
> > >
> > > On Fri, Jan 17, 2014 at 02:12:51PM -0500, Benjamin Tissoires wrote:
> > >> Evemu [1] uses uinput to replay devices traces it has recorded. However,
> > >> the way evemu uses uinput is slightly different from how uinput is
> > >> supposed to be used.
> > >> Evemu relies on libevdev, which creates the device node through uinput.
> > >> It then injects events through the input device node directly (and it
> > >> completely skips the uinput node).
> > >>
> > >> Currently, libevdev relies on an heuristic to guess which input node was
> > >> created. The problem is that is heuristic is subjected to races between
> > >> different uinput devices or even with physical devices. Having a way
> > >> to retrieve the sysfs path allows us to find the event node without
> > >> having to rely on this heuristic.
> > >
> > > I have been thinking about it and I think that providing tight coupling
> > > between uinput and resulting event device is wrong thing to do. We do
> > > allow sending input events through uinput interface and I think evemu
> > > should be using it, instead of going halfway through uinput and halfway
> > > though evdev. Replaying though uinput would actually be more correct as
> > > it would involve the same code paths throgugh input core as with using
> > > real devices (see input_event() vs. input_inject_event() that is used by
> > > input handlers).
> > >
> > 
> > Yes, I am perfectly aware of the fact that evemu is not using uinput
> > in the way it is intended to be.
> > I agree that it should be using the uinput node to inject events but
> > this means that only the process which has created the virtual device
> > can access it. It seems weird, I know, but the typical use of evemu is
> > the following:
> > - in a first terminal: $> sudo evemu-device mydevice.desc
> > - In a second: $> sudo evemu-play /dev/input/event12 < mydevice.events
> > 
> > It looks weird here, but it allows to inject different events
> > recording for the same virtual device node. 
> 
> it also allows replaying an event through the device it was recorded on.
> it's not always necessary or desirable to create a uinput device, sometimes
> replaying it through the actual device is better to reproduce a certain bug.

I was not saying that we should remove ability to inject events through
evdev nodes, so I am not sure why you are bringing your last point, but
form your and Benjamin's other mails I can see why going through evdev
(that has a separate device node) might be beneficial.

Benjamin, please clean up the issues brought up by David and I should be
able to apply the patch.

Thanks.

-- 
Dmitry

Re: [PATCH v3] input/uinput: add UI_GET_SYSNAME ioctl to retrieve the sysfs path

[Peter Hutterer]

On Mon, Jan 20, 2014 at 01:53:13PM -0800, Dmitry Torokhov wrote:
> Hi Benjamin,
> 
> On Fri, Jan 17, 2014 at 02:12:51PM -0500, Benjamin Tissoires wrote:
> > Evemu [1] uses uinput to replay devices traces it has recorded. However,
> > the way evemu uses uinput is slightly different from how uinput is
> > supposed to be used.
> > Evemu relies on libevdev, which creates the device node through uinput.
> > It then injects events through the input device node directly (and it
> > completely skips the uinput node).
> > 
> > Currently, libevdev relies on an heuristic to guess which input node was
> > created. The problem is that is heuristic is subjected to races between
> > different uinput devices or even with physical devices. Having a way
> > to retrieve the sysfs path allows us to find the event node without
> > having to rely on this heuristic.
> 
> I have been thinking about it and I think that providing tight coupling
> between uinput and resulting event device is wrong thing to do. We do
> allow sending input events through uinput interface and I think evemu
> should be using it, instead of going halfway through uinput and halfway
> though evdev. Replaying though uinput would actually be more correct as
> it would involve the same code paths throgugh input core as with using
> real devices (see input_event() vs. input_inject_event() that is used by
> input handlers).

this isn't just about evemu. I've got a fair number of tests that create a
uinput device and then pass the device node to the next level. for example,
you may want to create a synaptics touchpad through uinput and then test the
xorg driver against it. I can't pass the uinput fd to the xorg driver, it
only takes a device node. in fact, virtually all the interactions I have
with uinput is of that form - create a device, hook something up to the
device and then do stuff. it's not the writing side that I need this ioctl
for, it's making sure whatever is _reading_ from it is hooked up to the
right device.

Cheers,
   Peter