* Re: [PATCH v2] selftests/input: add test to cover len > maxlen in bits_to_user()
2023-06-21 8:30 [PATCH v2] selftests/input: add test to cover len > maxlen in bits_to_user() Dana Elfassy
@ 2023-06-22 9:50 ` Enric Balletbo Serra
0 siblings, 0 replies; 2+ messages in thread
From: Enric Balletbo Serra @ 2023-06-22 9:50 UTC (permalink / raw)
To: Dana Elfassy
Cc: shuah, usama.anjum, eballetbo, linux-kselftest, linux-kernel,
Dana Elfassy
Hi Dana,
Thank you for the patch.
Missatge de Dana Elfassy <delfassy@redhat.com> del dia dc., 21 de juny
2023 a les 10:30:
>
> In order to cover this case, setting 'maxlen = 0', with the following
> explanation:
> EVIOCGKEY is executed from evdev_do_ioctl(), which is called from
> evdev_ioctl_handler().
> evdev_ioctl_handler() is called from 2 functions, where by code coverage,
> only the first one is in use.
> ‘compat’ is given the value ‘0’ [1].
> Thus, the condition [2] is always false.
> This means ‘len’ always equals a positive number [3]
> ‘maxlen’ in evdev_handle_get_val [4] is defined locally in
> evdev_do_ioctl() [5], and is sent in the variable 'size' [6]
>
Like the comment in my previous patch I think this is understandable
for someone that has some context, but I am not sure it is understood
for people that don't have such context. So I'd try to rephrase and
explain in a more plain way. I.e.
selftests/input: introduce a test for the EVIOCGKEY ioctl
This patch introduces a specific test case for the EVIOCGKEY ioctl.
The test covers the case where len > maxlen in the
EVIOCGKEY(sizeof(keystate)), keystate) ioctl.
> [1] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L1281
> [2] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L705
> [3] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L707
> [4] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L886
> [5] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L1155
> [6] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L1141
>
Also, usually is not a good idea to add links in commit messages
reference to links that can easily end as dead links.
> Signed-off-by: Dana Elfassy <dangel101@gmail.com>
> ---
> Changes in v2:
> - Added following note about the patch's dependency
>
> This patch depends on '[v3] selftests/input: Introduce basic tests for evdev ioctls' [1] sent to the ML.
> [1] https://patchwork.kernel.org/project/linux-input/patch/20230607153214.15933-1-eballetbo@kernel.org/
>
> tools/testing/selftests/input/evioc-test.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/tools/testing/selftests/input/evioc-test.c b/tools/testing/selftests/input/evioc-test.c
> index ad7b93fe39cf..b94de2ee5596 100644
> --- a/tools/testing/selftests/input/evioc-test.c
> +++ b/tools/testing/selftests/input/evioc-test.c
> @@ -234,4 +234,23 @@ TEST(eviocsrep_set_repeat_settings)
> selftest_uinput_destroy(uidev);
> }
>
> +TEST(eviocgkey_get_global_key_state)
> +{
> + struct selftest_uinput *uidev;
> + int rep_values[2];
nit: rep_values sounds like repeat values, I'd use a variable name
more similar to what we really need to pass to the ioctl, like
keystate. Also I think this can be simply
int keystate;
> + int rc;
> +
> + memset(rep_values, 0, sizeof(rep_values));
> +
and then, this memset is not really needed.
> + rc = selftest_uinput_create_device(&uidev);
This one 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.
> + ASSERT_EQ(0, rc);
> + ASSERT_NE(NULL, uidev);
> +
> + /* ioctl to create the scenario where len > maxlen in bits_to_user() */
> + rc = ioctl(uidev->evdev_fd, EVIOCGKEY(0), rep_values);
> + ASSERT_EQ(0, rc);
> +
while we are here, and as it should be easy, maybe we can also extend
this to cover the normal workflow to get the global key status?
> + selftest_uinput_destroy(uidev);
> +}
> +
> TEST_HARNESS_MAIN
> --
> 2.41.0
>
Thanks,
Enric
^ permalink raw reply [flat|nested] 2+ messages in thread