[LTP] [PATCH v1] getcwd01: Implement .test_variants

[LTP] [PATCH v1] getcwd01: Implement .test_variants

[Wei Gao via ltp]

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

diff --git a/testcases/kernel/syscalls/getcwd/getcwd01.c b/testcases/kernel/syscalls/getcwd/getcwd01.c
index 218bf4ef2..e62254deb 100644
--- a/testcases/kernel/syscalls/getcwd/getcwd01.c
+++ b/testcases/kernel/syscalls/getcwd/getcwd01.c
@@ -1,23 +1,12 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * Copyright (c) International Business Machines Corp., 2001
+ * Copyright (c) 2023 Wei Gao <wegao@suse.com>
  */
 
 /*
  * 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:
- * 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.
+ * Testcase to test that getcwd() sets errno correctly.
  */
 
 #include <errno.h>
@@ -27,28 +16,68 @@
 #include "lapi/syscalls.h"
 
 static char buffer[5];
-
-static struct t_case {
+struct getcwd_variants {
+	void (*getcwd)(char *buf, size_t size, int exp_err);
 	char *buf;
 	size_t size;
 	int exp_err;
-} tcases[] = {
-	{(void *)-1, PATH_MAX, EFAULT},
-	{NULL, (size_t)-1, EFAULT},
-	{buffer, 0, ERANGE},
-	{buffer, 1, ERANGE},
-	{NULL, 1, ERANGE}
 };
 
+static void verify_getcwd_raw_syscall(char *buf, size_t size, int exp_err);
+static void verify_getcwd(char *buf, size_t size, int exp_err);
+
+static struct getcwd_variants variants[] = {
+#ifdef __GLIBC__
+	{ .getcwd = verify_getcwd, .buf = NULL, .size = (size_t)-1, .exp_err = ENOMEM},
+	{ .getcwd = verify_getcwd, .buf = NULL, .size = 1, .exp_err = ERANGE},
+#endif
+	{ .getcwd = verify_getcwd, .buf = (void *)-1, .size = PATH_MAX, .exp_err = EFAULT},
+	{ .getcwd = verify_getcwd, .buf = buffer, .size = 0, .exp_err = EINVAL},
+	{ .getcwd = verify_getcwd, .buf = buffer, .size = 1, .exp_err = ERANGE},
+	{ .getcwd = verify_getcwd_raw_syscall, .buf = buffer, .size = 0, .exp_err = ERANGE},
+	{ .getcwd = verify_getcwd_raw_syscall, .buf = (void *)-1, .size = PATH_MAX, .exp_err = EFAULT},
+	{ .getcwd = verify_getcwd_raw_syscall, .buf = NULL, .size = (size_t)-1, .exp_err = EFAULT},
+	{ .getcwd = verify_getcwd_raw_syscall, .buf = buffer, .size = 0, .exp_err = ERANGE},
+	{ .getcwd = verify_getcwd_raw_syscall, .buf = buffer, .size = 1, .exp_err = ERANGE},
+	{ .getcwd = verify_getcwd_raw_syscall, .buf = NULL, .size = 1, .exp_err = ERANGE},
+};
+
+static void verify_getcwd(char *buf, size_t size, int exp_err)
+{
+	char *res;
+
+	errno = 0;
+	res = getcwd(buf, size);
+	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 void verify_getcwd_raw_syscall(char *buf, size_t size, int exp_err)
+{
+
+	TST_EXP_FAIL2(tst_syscall(__NR_getcwd, buf, size), exp_err);
+}
 
-static void verify_getcwd(unsigned int n)
+static void verify(void)
 {
-	struct t_case *tc = &tcases[n];
+	struct getcwd_variants *tv = &variants[tst_variant];
 
-	TST_EXP_FAIL2(tst_syscall(__NR_getcwd, tc->buf, tc->size), tc->exp_err);
+	tv->getcwd(tv->buf, tv->size, tv->exp_err);
 }
 
 static struct tst_test test = {
-	.tcnt = ARRAY_SIZE(tcases),
-	.test = verify_getcwd
+	.test_variants = ARRAY_SIZE(variants),
+	.test_all = verify,
 };
-- 
2.35.3


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

Re: [LTP] [PATCH v1] getcwd01: Implement .test_variants

[Petr Vorel]

Hi Wei,

>  /*
>   * 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:
> - * 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.
> + * Testcase to test that getcwd() sets errno correctly.

We prefer to keep info about tested errno (you can write errno or errnos if they
are different for syscall into it).

* 1) getcwd(2) fails if buf points to a bad address (EFAULT)
* 2) getcwd(2) fails if the size is invalid (ERANGE)

>  #include <errno.h>
> @@ -27,28 +16,68 @@
>  #include "lapi/syscalls.h"

>  static char buffer[5];
> -
> -static struct t_case {
> +struct getcwd_variants {
> +	void (*getcwd)(char *buf, size_t size, int exp_err);
>  	char *buf;
>  	size_t size;
>  	int exp_err;
> -} tcases[] = {
> -	{(void *)-1, PATH_MAX, EFAULT},
> -	{NULL, (size_t)-1, EFAULT},
> -	{buffer, 0, ERANGE},
> -	{buffer, 1, ERANGE},
> -	{NULL, 1, ERANGE}
>  };

> +static void verify_getcwd_raw_syscall(char *buf, size_t size, int exp_err);
> +static void verify_getcwd(char *buf, size_t size, int exp_err);
> +
> +static struct getcwd_variants variants[] = {
> +#ifdef __GLIBC__
> +	{ .getcwd = verify_getcwd, .buf = NULL, .size = (size_t)-1, .exp_err = ENOMEM},
> +	{ .getcwd = verify_getcwd, .buf = NULL, .size = 1, .exp_err = ERANGE},
> +#endif
> +	{ .getcwd = verify_getcwd, .buf = (void *)-1, .size = PATH_MAX, .exp_err = EFAULT},
> +	{ .getcwd = verify_getcwd, .buf = buffer, .size = 0, .exp_err = EINVAL},
> +	{ .getcwd = verify_getcwd, .buf = buffer, .size = 1, .exp_err = ERANGE},
> +	{ .getcwd = verify_getcwd_raw_syscall, .buf = buffer, .size = 0, .exp_err = ERANGE},
> +	{ .getcwd = verify_getcwd_raw_syscall, .buf = (void *)-1, .size = PATH_MAX, .exp_err = EFAULT},
> +	{ .getcwd = verify_getcwd_raw_syscall, .buf = NULL, .size = (size_t)-1, .exp_err = EFAULT},
> +	{ .getcwd = verify_getcwd_raw_syscall, .buf = buffer, .size = 0, .exp_err = ERANGE},
> +	{ .getcwd = verify_getcwd_raw_syscall, .buf = buffer, .size = 1, .exp_err = ERANGE},
> +	{ .getcwd = verify_getcwd_raw_syscall, .buf = NULL, .size = 1, .exp_err = ERANGE},

Well, this is works, but we don't use test variants this way. We define
.test_variants in struct tst_test. Please have look how getdents01.c for
example.

Why it's important (besides consistency of how we write tests)? We parse
test_variants in our documentation.
https://github.com/linux-test-project/ltp/releases/download/20230929/metadata.20230929.html
=> see metadata.html#test_variants in your locally downloaded file, or run:
cd metadata && make && chromium ../docparse/*.html

Kind regards,
Petr

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

[LTP] [PATCH v2] getcwd01: Implement .test_variants

[Wei Gao via ltp]

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
@@ -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>
+ *
+ * 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
+ */
+
+#ifndef GETCWD_H
+#define 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)
+{
+	char *res;
+
+	errno = 0;
+	res = getcwd(buf, size);
+	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)
+{
+	switch (tst_variant) {
+	case 0:
+		TST_EXP_FAIL2(tst_syscall(__NR_getcwd, buf, size), exp_err);
+		break;
+	case 1:
+#ifdef __GLIBC__
+		check_getcwd(buf, size, exp_err2);
+#endif
+		break;
+	}
+}
+
+static inline void
+getcwd_info(void)
+{
+	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;
+	}
+}
+
+#define TEST_VARIANTS 2
+
+#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
  * 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
+ * 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;
 } 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)
 {
 	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();
 }
 
 static struct tst_test test = {
+	.setup = setup,
 	.tcnt = ARRAY_SIZE(tcases),
-	.test = verify_getcwd
+	.test = run,
+	.test_variants = TEST_VARIANTS,
 };
-- 
2.35.3


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

Re: [LTP] [PATCH v2] getcwd01: Implement .test_variants

[Petr Vorel]

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

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

Re: [LTP] [PATCH v2] getcwd01: Implement .test_variants

[Petr Vorel]

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
...

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

Re: [LTP] [PATCH v2] getcwd01: Implement .test_variants

[Cyril Hrubis]

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.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v2] getcwd01: Implement .test_variants

[Petr Vorel]

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

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

Re: [LTP] [PATCH v2] getcwd01: Implement .test_variants

[Wei Gao via ltp]

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

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

Re: [LTP] [PATCH v2] getcwd01: Implement .test_variants

[Wei Gao via ltp]

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

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

Re: [LTP] [PATCH v2] getcwd01: Implement .test_variants

[Wei Gao via ltp]

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.
> >   */

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

Re: [LTP] [PATCH v2] getcwd01: Implement .test_variants

[Petr Vorel]

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

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

[LTP] [PATCH v3] getcwd01: Implement .test_variants

[Wei Gao via ltp]

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

NOTE:
Postpond create TST_EXP_FAIL_PTR in this patch since i saw some change request on this area:
https://patchwork.ozlabs.org/project/ltp/patch/20240103115700.14585-1-chrubis@suse.cz/

Also this patch will be continue updated base Cyril's feedback on new proposal(pass the test 
both on EFAULT and child being killed by SIGSEGV)


diff --git a/testcases/kernel/syscalls/getcwd/getcwd01.c b/testcases/kernel/syscalls/getcwd/getcwd01.c
index 218bf4ef2..e2df512e3 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
  */
 
-/*
- * 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.
+ *
+ * 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:
- * 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.
+ *
+ * linux syscall
+ *
+ * 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 and uclibc{,-ng}.
+ *
+ * 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>
@@ -32,23 +45,71 @@ 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 check_getcwd(char *buf, size_t size, int exp_err)
+{
+	char *res;
+
+	errno = 0;
+	res = getcwd(buf, size);
+	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;
+	}
 
-static void verify_getcwd(unsigned int n)
+	tst_res(TPASS | TTERRNO, "getcwd() failed as expected");
+
+}
+
+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
+		check_getcwd(buf, size, exp_err_libc);
+}
+
+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 */
+#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_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");
 }
 
 static struct tst_test test = {
+	.setup = setup,
 	.tcnt = ARRAY_SIZE(tcases),
-	.test = verify_getcwd
+	.test = run,
+	.test_variants = 2,
 };
-- 
2.35.3


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