Re: [PATCH v2] selftests/input: Introduce basic tests for evdev ioctls

[Peter Hutterer <peter.hutterer@who-t.net>]

On Tue, May 30, 2023 at 12:26:27PM +0200, Enric Balletbo i Serra wrote:
> This provides a basic infrastructure for the creation of tests for the evdev
> interface. Most of this code is adapted from the libevdev wrapper library. While
> most of evdev ioctls are covered and tested using libevdev tests there are some
> evdev ioctls that aren't because are not supported (and will not be supported)
> by libevdev [1]. So, adding, at least those tests, would make sense.
> 
> The test creates an uinput device (and an evdev device) so you can
> call the wanted ioctl from userspace. So, to run those tests you need
> to have support for uinput and evdev as well.
> 
> [1] For example, libevdev doesn't support setting EV_REP because it's inherently
> racy - one libevdev context to set those values via the ioctl would cause all
> other libevdev contexts on the same device to be out of sync. Since we do not
> get notifications when the values changed, libevdev's buffered values for EV_REP
> will remain whatever they were initially.
> 
> Signed-off-by: Enric Balletbo i Serra <eballetbo@kernel.org>

thanks, this mostly LGTM but there's still a bug left in the vararg
handling.

[...]

> +#include <dirent.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <linux/uinput.h>
> +#include <poll.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <time.h>
> +#include <unistd.h>
> +
> +#include "../kselftest_harness.h"
> +
> +#define TEST_DEVICE_NAME "selftest input device"
> +
> +struct selftest_uinput {
> +	int uinput_fd; /** file descriptor to uinput */
> +	int evdev_fd; /** file descriptor to evdev */
> +	char *name; /** device name */
> +	char *syspath; /** /sys path */
> +	char *devnode; /** device node */

nitpick: none of name, syspath, devnode are used in the tests and it's
likely they'll never need to be so there's no reason to strdup them
here. You could change fetch_syspath_and_devnode() to open_devnode() and
return the opened fd, meaning you can reduce the code even more.

[...]

> +
> +TEST(eviocgname_get_device_name)
> +{
> +	struct selftest_uinput *uidev;
> +	char buf[256];
> +	int rc;
> +
> +	rc = selftest_uinput_create_device(&uidev);

this one and the others without extra arguments need to be:

	rc = selftest_uinput_create_device(&uidev, -1);

otherwise the vararg loop is going to keep the room warm for no reason.

Cheers,
  Peter