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;
MIME-Version: 1.0
References: <20230621083026.591323-1-dangel101@gmail.com>
In-Reply-To: <20230621083026.591323-1-dangel101@gmail.com>
From:   Enric Balletbo Serra <eballetbo@gmail.com>
Date:   Thu, 22 Jun 2023 11:50:19 +0200
Message-ID: <CAFqH_50Td8iigXOJark5Exoa7bD7azg3E4wKWc=ySXTZ7bch9A@mail.gmail.com>
Subject: Re: [PATCH v2] selftests/input: add test to cover len > maxlen in bits_to_user()
To:     Dana Elfassy <delfassy@redhat.com>
Cc:     shuah@kernel.org, usama.anjum@collabora.com, eballetbo@kernel.org,
        linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
        Dana Elfassy <dangel101@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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 =3D 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.
> =E2=80=98compat=E2=80=99 is given the value =E2=80=980=E2=80=99 [1].
> Thus, the condition [2] is always false.
> This means =E2=80=98len=E2=80=99 always equals a positive number [3]
> =E2=80=98maxlen=E2=80=99 in evdev_handle_get_val [4] is defined locally i=
n
> 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#L1=
281
> [2] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L7=
05
> [3] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L7=
07
> [4] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L8=
86
> [5] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L1=
155
> [6] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L1=
141
>

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 ev=
dev 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/s=
elftests/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 =3D selftest_uinput_create_device(&uidev);

This one without extra arguments need to be:

        rc =3D 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_use=
r() */
> +       rc =3D 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