Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i7ce144cd:Fastmail
Date:   Tue, 6 Jun 2023 15:04:31 +1000
From:   Peter Hutterer <peter.hutterer@who-t.net>
To:     Enric Balletbo i Serra <eballetbo@kernel.org>
Cc:     linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
        Shuah Khan <shuah@kernel.org>,
        Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        Javier Martinez Canillas <javierm@redhat.com>,
        Dana Elfassy <delfassy@redhat.com>,
        linux-input@vger.kernel.org, phuttere@redhat.com,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>
Subject: Re: [PATCH v2] selftests/input: Introduce basic tests for evdev
 ioctls
Message-ID: <20230606050431.GA3789903@quokka>
References: <20230530102627.87284-1-eballetbo@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230530102627.87284-1-eballetbo@kernel.org>
Precedence: bulk

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