Re: [LTP] [PATCH v3 2/2] getcwd01: Implement .test_variants

[Petr Vorel <pvorel@suse.cz>]

Hi Wei,

> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>  testcases/kernel/syscalls/getcwd/getcwd01.c | 80 +++++++++++++++------
>  1 file changed, 60 insertions(+), 20 deletions(-)

> NOTE: Cyril give solution for run the test in a child and pass
> the test both on EFAULT and child being killed by SIGSEGV. But
> currently i have no idea how to do it since no SIGSEGV will hapeen
> if NULL buffer give to getcwd. This file just used for give a real
> user case for TST_EXP_FAIL_PTR.

@Cyril, could you please point out test which uses similar approach?
Maybe these?
testcases/kernel/syscalls/fstat/fstat03.c
testcases/kernel/syscalls/ipc/shmat/shmat01.c
testcases/kernel/syscalls/kill/kill11.c
testcases/kernel/syscalls/setrlimit/setrlimit05.c

Or can the current approach be used?

Below only nits (formatting).

> diff --git a/testcases/kernel/syscalls/getcwd/getcwd01.c b/testcases/kernel/syscalls/getcwd/getcwd01.c
> index 218bf4ef2..879c36206 100644
> --- a/testcases/kernel/syscalls/getcwd/getcwd01.c
> +++ b/testcases/kernel/syscalls/getcwd/getcwd01.c
> @@ -3,21 +3,34 @@
>   * Copyright (c) International Business Machines Corp., 2001

IBM in 2001 could be proud on this code :). There should have been some
copyright, I would add:
 * Copyright (c) Linux Test Project, 2002-2024

>   */

> -/*
> - * DESCRIPTION
> +/*\
> + * [Description]
> + *
>   * Testcase to test that getcwd(2) sets errno correctly.
> + *
> + * 1. getcwd(2) fails if buf points to a bad address.
> + * 2. getcwd(2) fails if the size is invalid.
> + * 3. getcwd(2) fails if the size is set to 0.
> + * 4. getcwd(2) fails if the size is set to 1.
> + * 5. getcwd(2) fails if buf points to NULL and the size is set to 1.
>   *
>   * Expected Result:
> + *
> + * linux syscall
> + *
> + * 1. getcwd(2) should return NULL and set errno to EFAULT.
I don't like repeating "getcwd(2) should return NULL and set errno to"

> + * 2. getcwd(2) should return NULL and set errno to EFAULT.
> + * 3. getcwd(2) should return NULL and set errno to ERANGE.
> + * 4. getcwd(2) should return NULL and set errno to ERANGE.
> + * 5. getcwd(2) should return NULL and set errno to ERANGE.
> + *
> + * glibc and uclibc{,-ng}.
Although in the past LTP developers cared only about glibc, now we generally
don't stick to any libc implementation. Thus it should be "libc syscall wrapper"

> + *
> + * 1. getcwd(2) should return NULL and set errno to EFAULT.
> + * 2. getcwd(2) should return NULL and set errno to ENOMEM.
> + * 3. getcwd(2) should return NULL and set errno to EINVAL.
> + * 4. getcwd(2) should return NULL and set errno to ERANGE.
> + * 5. getcwd(2) should return NULL and set errno to ERANGE.
>   */

How about to make it simple like this:
/*\
 * [Description]
 *
 * Testcase to test that getcwd(2) returns NULL and sets errno correctly.
 *
 * 1. getcwd(2) fails if buf points to a bad address (EFAULT).
 * 2. getcwd(2) fails if the size is invalid (syscall: EFAULT, libc wrapper: ENOMEM).
 * 3. getcwd(2) fails if the size is set to 0 (syscall: ERANGE, libc wrapper: EINVAL).
 * 4. getcwd(2) fails if the size is set to 1 (ERANGE).
 * 5. getcwd(2) fails if buf points to NULL and the size is set to 1 (ERANGE).
 */

>  #include <errno.h>
> @@ -32,23 +45,50 @@ static struct t_case {
>  	char *buf;
>  	size_t size;
>  	int exp_err;
> +	int exp_err_libc;
>  } tcases[] = {
> -	{(void *)-1, PATH_MAX, EFAULT},
> -	{NULL, (size_t)-1, EFAULT},
> -	{buffer, 0, ERANGE},
> -	{buffer, 1, ERANGE},
> -	{NULL, 1, ERANGE}
> +	{(void *)-1, PATH_MAX, EFAULT, EFAULT},
> +	{NULL, (size_t)-1, EFAULT, ENOMEM},
> +	{buffer, 0, ERANGE, EINVAL},
> +	{buffer, 1, ERANGE, ERANGE},
> +	{NULL, 1, ERANGE, ERANGE},
>  };

> +static inline void tst_getcwd(char *buf, size_t size, int exp_err, int exp_err_libc)
> +{
> +
> +	if (tst_variant == 0)
> +		TST_EXP_FAIL2(tst_syscall(__NR_getcwd, buf, size), exp_err);
> +	else
> +		TST_EXP_FAIL_PTR(getcwd(buf, size), exp_err_libc);
> +}
+1

> -static void verify_getcwd(unsigned int n)
> +static void run(unsigned int n)
>  {
>  	struct t_case *tc = &tcases[n];

> -	TST_EXP_FAIL2(tst_syscall(__NR_getcwd, tc->buf, tc->size), tc->exp_err);
> +	/* https://github.com/linux-test-project/ltp/issues/1084 */
IMHO this comment should go to the commit message.

> +#if !defined(__GLIBC__) && !defined(__ANDROID__)
I'll ask AOSP developers to check this.

> +	if (tst_variant && !tc->buf) {
> +		tst_res(TCONF, "NULL buffer test skipped on MUSL due different implementation");
> +		return;
> +	}
> +#endif
> +
> +	tst_getcwd(tc->buf, tc->size, tc->exp_err, tc->exp_err_libc);
> +}
> +
> +static void setup(void)
> +{
> +	if (tst_variant == 0)
> +		tst_res(TINFO, "Testing getcwd with raw syscall");
> +	else
> +		tst_res(TINFO, "Testing getcwd with wrap syscall");

"wrap syscall" => "libc syscall wrapper"

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp