[v2] getcwd01: Implement .test_variants

Signed-off-by: Wei Gao <wegao@suse.com>
---
 testcases/kernel/syscalls/getcwd/getcwd.h   | 80 +++++++++++++++++++++
 testcases/kernel/syscalls/getcwd/getcwd01.c | 35 ++++++---
 2 files changed, 105 insertions(+), 10 deletions(-)
 create mode 100644 testcases/kernel/syscalls/getcwd/getcwd.h

Hi Wei,

> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>  testcases/kernel/syscalls/getcwd/getcwd.h   | 80 +++++++++++++++++++++
>  testcases/kernel/syscalls/getcwd/getcwd01.c | 35 ++++++---
>  2 files changed, 105 insertions(+), 10 deletions(-)
>  create mode 100644 testcases/kernel/syscalls/getcwd/getcwd.h

> diff --git a/testcases/kernel/syscalls/getcwd/getcwd.h b/testcases/kernel/syscalls/getcwd/getcwd.h
> new file mode 100644
> index 000000000..91f229904
> --- /dev/null
> +++ b/testcases/kernel/syscalls/getcwd/getcwd.h
First, I don't think it's a good idea to create getcwd.h, when code is used only
in single source. It should be in getcwd01.c. And looking into other getcwd0*.c
sources, I would use test_variants only in this source.

> @@ -0,0 +1,80 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * Copyright (c) International Business Machines  Corp., 2001
> + * Copyright (c) 2013 Cyril Hrubis <chrubis@suse.cz>
Why these two copyrights?

> + *
> + * This program is free software;  you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY;  without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
> + * the GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program;  if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
You know that SPDX is used to replaced this verbose license text, right?

> + */
> +
> +#ifndef GETCWD_H
> +#define GETCWD_H
We usually add trailing __ to avoid theoretical clash with third party headers.
(GETCWD_H__).

> +#include <stdint.h>
> +#include "config.h"
> +#include "lapi/syscalls.h"
> +
> +static inline void
> +check_getcwd(char *buf, size_t size, int exp_err)
This should be on single line. If you copy pasted it from something, that was
probably where signature was much longer than 80 char, this is even less than 80
chars.

> +{
> +	char *res;
> +
> +	errno = 0;
> +	res = getcwd(buf, size);
Why so complicated code? Why not just use TST_EXP_FAIL2() as you do in
tst_getcwd()? That would be way fewer lines of code. There is no problem to use
TST_EXP_FAIL2() with libc syscall wrappers.

> +	TST_ERR = errno;
> +	if (res) {
> +		tst_res(TFAIL, "getcwd() succeeded unexpectedly");
> +		return;
> +	}
> +
> +	if (TST_ERR != exp_err) {
> +		tst_res(TFAIL | TTERRNO, "getcwd() failed unexpectedly, expected %s",
> +				tst_strerrno(exp_err));
> +		return;
> +	}
> +
> +	tst_res(TPASS | TTERRNO, "getcwd() failed as expected");
> +}
> +
> +static inline void
> +tst_getcwd(char *buf, size_t size, int exp_err, int exp_err2)
And here as well: this should be on single line.
> +{
> +	switch (tst_variant) {
Please, do not use switch for 2 variants, if is much readable in this case.
> +	case 0:
> +		TST_EXP_FAIL2(tst_syscall(__NR_getcwd, buf, size), exp_err);
> +		break;
> +	case 1:
> +#ifdef __GLIBC__

#1084 [1] reported problem on MUSL only. Your original patch [2] to fix #1084
skipped only 2 of tests, which used NULL buffer. Why now skip everything? Please
skip only these two and put comment about musl and #1084 to be obvious why you
do that. You could also add note to the git commit message. Not documenting this
will raise questions in the future.

Also, looking that into bionic [3], it's compatible with glibc and uclibc{,-ng}
(I'll verify it with Edward Liaw from Google), thus I would check like in
run(unsigned int n):

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

	/* https://github.com/linux-test-project/ltp/issues/1084 */
#if !defined(__GLIBC__) && !defined(__ANDROID__)
	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_err2);
}

Or we could skip NULL buffer test on all libc.

[1] https://github.com/linux-test-project/ltp/issues/1084
[2] https://lore.kernel.org/ltp/20230928010808.15862-1-wegao@suse.com/
[3] https://android.googlesource.com/platform/bionic.git/+/refs/tags/android-14.0.0_r18/libc/bionic/getcwd.cpp

> +		check_getcwd(buf, size, exp_err2);
> +#endif
> +		break;
> +	}
> +}
> +
> +static inline void
> +getcwd_info(void)
This should be on single line.
> +{
> +	switch (tst_variant) {
> +	case 0:
> +		tst_res(TINFO, "Testing getcwd with raw syscall");
> +		break;
> +	case 1:
> +		tst_res(TINFO, "Testing getcwd with wrap syscall");
> +		break;
> +	}
Instead of this, I would add direct TINFO messages to the setup function in getcwd01.c.
Again, once this is used in 2 sources, it makes sense to have custom function to
it, otherwise not.
And we should rethink, how to simplify using test_variants on these simple
cases (I did some proposal in the past, I should get back to it; the problem is
with that some tests use clock_adjtime.h, which is a bit complicate than this
raw syscall vs. libc wrapper simple use case).

> +}
> +
> +#define TEST_VARIANTS 2

Instead of this define I would just put 2 into .test_variants =
(again, only used in single file).

> +
> +#endif /* GETCWD_H */
> diff --git a/testcases/kernel/syscalls/getcwd/getcwd01.c b/testcases/kernel/syscalls/getcwd/getcwd01.c
> index 218bf4ef2..6decb961f 100644
> --- a/testcases/kernel/syscalls/getcwd/getcwd01.c
> +++ b/testcases/kernel/syscalls/getcwd/getcwd01.c
> @@ -13,18 +13,26 @@
>   * 5) getcwd(2) fails if buf points to NULL and the size is set to 1.
>   *
>   * Expected Result:
> + * linux syscall
Well, I told you several times, that list requires separating by blank line
otherwise it breaks formatting. Could you please add it and check generated
docparse before sending a patch:
cd metadata && make && chromium ../docparse/*.html
We should check for it in metaparse.c or in testinfo.pl.
>   * 1) getcwd(2) should return NULL and set errno to EFAULT.
>   * 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
FYI #ifdef __GLIBC__ means glibc and uclibc{,-ng}.
Also, again missing blank line before list.
> + * 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.
>   */

>  #include <errno.h>
>  #include <unistd.h>
>  #include <limits.h>
>  #include "tst_test.h"
> -#include "lapi/syscalls.h"
> +#include "getcwd.h"

>  static char buffer[5];

> @@ -32,23 +40,30 @@ static struct t_case {
>  	char *buf;
>  	size_t size;
>  	int exp_err;
> +	int exp_err2;
Maybe exp_err_kernel and exp_err_libc would actually describe the purpose.

>  } 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 void verify_getcwd(unsigned int n)
> +static void run(unsigned int n)
nit: this can stay verify_getcwd().
>  {
>  	struct t_case *tc = &tcases[n];

> -	TST_EXP_FAIL2(tst_syscall(__NR_getcwd, tc->buf, tc->size), tc->exp_err);
> +	tst_getcwd(tc->buf, tc->size, tc->exp_err, tc->exp_err2);
> +}
> +
> +static void setup(void)
> +{
> +	getcwd_info();
There is no point to use wrappers like this (I guess I have told you on
different patch). We would specify .setup = getcwd_info, in the struct tst_test.
But as I wrote above, better would be to move content of getcwd_info() to setup
function.

>  }

>  static struct tst_test test = {
> +	.setup = setup,
>  	.tcnt = ARRAY_SIZE(tcases),
> -	.test = verify_getcwd
> +	.test = run,
> +	.test_variants = TEST_VARIANTS,
	.test_variants = 2,
>  };

Kind regards,
Petr

Hi Li, Cyril,

...
> > +static inline void
> > +tst_getcwd(char *buf, size_t size, int exp_err, int exp_err2)
> And here as well: this should be on single line.
> > +{
> > +	switch (tst_variant) {
> Please, do not use switch for 2 variants, if is much readable in this case.
> > +	case 0:
> > +		TST_EXP_FAIL2(tst_syscall(__NR_getcwd, buf, size), exp_err);
> > +		break;
> > +	case 1:
> > +#ifdef __GLIBC__

> #1084 [1] reported problem on MUSL only. Your original patch [2] to fix #1084
> skipped only 2 of tests, which used NULL buffer. Why now skip everything? Please
> skip only these two and put comment about musl and #1084 to be obvious why you
> do that. You could also add note to the git commit message. Not documenting this
> will raise questions in the future.

> Also, looking that into bionic [3], it's compatible with glibc and uclibc{,-ng}
> (I'll verify it with Edward Liaw from Google), thus I would check like in
> run(unsigned int n):

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

> 	/* https://github.com/linux-test-project/ltp/issues/1084 */
> #if !defined(__GLIBC__) && !defined(__ANDROID__)
> 	if (tst_variant && !tc->buf) {
> 		tst_res(TCONF, "NULL buffer test skipped on MUSL due different implementation");
> 		return;
> 	}
> #endif

@Li @Cyril: are you ok to test libc getcwd() wrapper implementations on NULL
buffer in getcwd01.c? Or we just skip NULL buffer test on all libc?
I would be ok to test it, because change in the implementation can influence
lots of user space software, although glibc or any other libc can obviously
change it's behavior.

Kind regards,
Petr

> 	tst_getcwd(tc->buf, tc->size, tc->exp_err, tc->exp_err2);
> }

> Or we could skip NULL buffer test on all libc.

> [1] https://github.com/linux-test-project/ltp/issues/1084
> [2] https://lore.kernel.org/ltp/20230928010808.15862-1-wegao@suse.com/
> [3] https://android.googlesource.com/platform/bionic.git/+/refs/tags/android-14.0.0_r18/libc/bionic/getcwd.cpp
...

Hi!
> @Li @Cyril: are you ok to test libc getcwd() wrapper implementations on NULL
> buffer in getcwd01.c? Or we just skip NULL buffer test on all libc?
> I would be ok to test it, because change in the implementation can influence
> lots of user space software, although glibc or any other libc can obviously
> change it's behavior.

I guess that the easiest solution is to run the test in a child and pass
the test both on EFAULT and child being killed by SIGSEGV. Any special
cases and ifdefs are assuming something that is not promised and will
have to be changed sooner or later.

On Wed, Dec 27, 2023 at 02:21:44PM +0100, Petr Vorel wrote:
> Hi Wei,
> 
> > +{
> > +	char *res;
> > +
> > +	errno = 0;
> > +	res = getcwd(buf, size);
> Why so complicated code? Why not just use TST_EXP_FAIL2() as you do in
> tst_getcwd()? That would be way fewer lines of code. There is no problem to use
> TST_EXP_FAIL2() with libc syscall wrappers.
Current TST_EXP_FAIL2 can not handle getcwd (will return NULL). I remember in another
email thread you mention maybe we need TST_EXP_FAIL_PTR.
> 
> > +	TST_ERR = errno;
> > +	if (res) {
> > +		tst_res(TFAIL, "getcwd() succeeded unexpectedly");
> > +		return;
> > +	}
> > +
> > +	if (TST_ERR != exp_err) {
> > +		tst_res(TFAIL | TTERRNO, "getcwd() failed unexpectedly, expected %s",
> > +				tst_strerrno(exp_err));
> > +		return;
> > +	}
> > +
> > +	tst_res(TPASS | TTERRNO, "getcwd() failed as expected");
> > +}
> > +
> > diff --git a/testcases/kernel/syscalls/getcwd/getcwd01.c b/testcases/kernel/syscalls/getcwd/getcwd01.c
> > index 218bf4ef2..6decb961f 100644
> > --- a/testcases/kernel/syscalls/getcwd/getcwd01.c
> > +++ b/testcases/kernel/syscalls/getcwd/getcwd01.c
> > @@ -13,18 +13,26 @@
> >   * 5) getcwd(2) fails if buf points to NULL and the size is set to 1.
> >   *
> >   * Expected Result:
> > + * linux syscall
> Well, I told you several times, that list requires separating by blank line
> otherwise it breaks formatting. Could you please add it and check generated
> docparse before sending a patch:
> cd metadata && make && chromium ../docparse/*.html
> We should check for it in metaparse.c or in testinfo.pl.
The original code seems can not correctly handled by docparse since format is wrong,
so acutally no any description item will appear in metadata file, same issue for 
getcwd02/03/04. I will update to correct format in getcwd01 next patch.
> >   * 1) getcwd(2) should return NULL and set errno to EFAULT.
> >   * 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
> FYI #ifdef __GLIBC__ means glibc and uclibc{,-ng}.
> Also, again missing blank line before list.
> > + * 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.
> >   */

Hi Cyril, all,

> Hi!
> > @Li @Cyril: are you ok to test libc getcwd() wrapper implementations on NULL
> > buffer in getcwd01.c? Or we just skip NULL buffer test on all libc?
> > I would be ok to test it, because change in the implementation can influence
> > lots of user space software, although glibc or any other libc can obviously
> > change it's behavior.

> I guess that the easiest solution is to run the test in a child and pass
> the test both on EFAULT and child being killed by SIGSEGV. Any special
> cases and ifdefs are assuming something that is not promised and will
> have to be changed sooner or later.

Good point. I guess something like testcases/kernel/syscalls/mmap/mmap05.c
approach right? Wei, could you try that?

Kind regards,
Petr

Hi Wei,

> On Wed, Dec 27, 2023 at 02:21:44PM +0100, Petr Vorel wrote:
> > Hi Wei,

> > > +{
> > > +	char *res;
> > > +
> > > +	errno = 0;
> > > +	res = getcwd(buf, size);
> > Why so complicated code? Why not just use TST_EXP_FAIL2() as you do in
> > tst_getcwd()? That would be way fewer lines of code. There is no problem to use
> > TST_EXP_FAIL2() with libc syscall wrappers.
> Current TST_EXP_FAIL2 can not handle getcwd (will return NULL). I remember in another
> email thread you mention maybe we need TST_EXP_FAIL_PTR.

Good point, thanks for working on it. It will use TESTPTR().

...
> > > +++ b/testcases/kernel/syscalls/getcwd/getcwd01.c
> > > @@ -13,18 +13,26 @@
> > >   * 5) getcwd(2) fails if buf points to NULL and the size is set to 1.
> > >   *
> > >   * Expected Result:
> > > + * linux syscall
> > Well, I told you several times, that list requires separating by blank line
> > otherwise it breaks formatting. Could you please add it and check generated
> > docparse before sending a patch:
> > cd metadata && make && chromium ../docparse/*.html
> > We should check for it in metaparse.c or in testinfo.pl.
> The original code seems can not correctly handled by docparse since format is wrong,
> so acutally no any description item will appear in metadata file, same issue for 
> getcwd02/03/04. I will update to correct format in getcwd01 next patch.

Thank you!

Kind regards,
Petr

On Wed, Jan 03, 2024 at 09:44:25AM +0100, Petr Vorel wrote:
> Hi Cyril, all,
> 
> > Hi!
> > > @Li @Cyril: are you ok to test libc getcwd() wrapper implementations on NULL
> > > buffer in getcwd01.c? Or we just skip NULL buffer test on all libc?
> > > I would be ok to test it, because change in the implementation can influence
> > > lots of user space software, although glibc or any other libc can obviously
> > > change it's behavior.
> 
> > I guess that the easiest solution is to run the test in a child and pass
> > the test both on EFAULT and child being killed by SIGSEGV. Any special
> > cases and ifdefs are assuming something that is not promised and will
> > have to be changed sooner or later.
> 
> Good point. I guess something like testcases/kernel/syscalls/mmap/mmap05.c
> approach right? Wei, could you try that?
Sure! Will check mmap05.c firstly.
> 
> Kind regards,
> Petr

On Wed, Jan 03, 2024 at 04:32:59AM -0500, Wei Gao via ltp wrote:
> On Wed, Jan 03, 2024 at 09:44:25AM +0100, Petr Vorel wrote:
> > Hi Cyril, all,
> > 
> > > Hi!
> > > > @Li @Cyril: are you ok to test libc getcwd() wrapper implementations on NULL
> > > > buffer in getcwd01.c? Or we just skip NULL buffer test on all libc?
> > > > I would be ok to test it, because change in the implementation can influence
> > > > lots of user space software, although glibc or any other libc can obviously
> > > > change it's behavior.
> > 
> > > I guess that the easiest solution is to run the test in a child and pass
> > > the test both on EFAULT and child being killed by SIGSEGV. Any special
> > > cases and ifdefs are assuming something that is not promised and will
> > > have to be changed sooner or later.
> > 
> > Good point. I guess something like testcases/kernel/syscalls/mmap/mmap05.c
> > approach right? Wei, could you try that?
> Sure! Will check mmap05.c firstly.
I have checked mmap05.c, got idea how to check SIGSEGV, but in current case NULL buffer
will not trigger any SIGSEGV in musl(I have tested on musl env today). 
Correct me if i have misunderstanding.

BTW:I will continue update patch base Petr's former comments.
> > 
> > Kind regards,
> > Petr
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp