[LTP] [PATCH v1] Port `getxattr01.c` to new test API

[LTP] [PATCH v1] Port `getxattr01.c` to new test API

[Marius Kittler]

* Utilize `all_filesystems = 1`-mechanism to test on various file
  systems instead of relying on the temporary directory's file system
  to support xattr (which it probably does not as it is commonly a
  tmpfs)
* Improve error handling
* Simplify code and description
* Related issue: https://github.com/linux-test-project/ltp/issues/583

Signed-off-by: Marius Kittler <mkittler@suse.de>
---
 .../kernel/syscalls/getxattr/getxattr01.c     | 219 ++++++++----------
 1 file changed, 94 insertions(+), 125 deletions(-)

diff --git a/testcases/kernel/syscalls/getxattr/getxattr01.c b/testcases/kernel/syscalls/getxattr/getxattr01.c
index cec802a33..e9d5874d9 100644
--- a/testcases/kernel/syscalls/getxattr/getxattr01.c
+++ b/testcases/kernel/syscalls/getxattr/getxattr01.c
@@ -1,38 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * Copyright (C) 2011  Red Hat, Inc.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of version 2 of the GNU General Public
- * License as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it would be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
- *
- * Further, this software is distributed without any warranty that it
- * is free of the rightful claim of any third person regarding
- * infringement or the like.  Any license provided herein, whether
- * implied or otherwise, applies only to this software file.  Patent
- * licenses, if any, provided herein do not apply to combinations of
- * this program with other software, or any other product whatsoever.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write the Free Software
- * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
- * 02110-1301, USA.
+ * Copyright (c) 2023 Marius Kittler <mkittler@suse.de>
  */
 
-/*
- * Basic tests for getxattr(2) and make sure getxattr(2) handles error
- * conditions correctly.
+/*\
+ * [Description]
  *
- * There are 4 test cases:
+ * Basic tests for getxattr(2), there are 3 test cases:
  * 1. Get an non-existing attribute,
- *    getxattr(2) should return -1 and set errno to ENODATA
+ *    getxattr(2) should return -1 and set errno to ENODATA.
  * 2. Buffer size is smaller than attribute value size,
- *    getxattr(2) should return -1 and set errno to ERANGE
- * 3. Get attribute, getxattr(2) should succeed
- * 4. Verify the attribute got by getxattr(2) is same as the value we set
+ *    getxattr(2) should return -1 and set errno to ERANGE.
+ * 3. getxattr(2) should succeed and return the same value we set
+ *    before.
  */
 
 #include "config.h"
@@ -47,138 +28,126 @@
 #ifdef HAVE_SYS_XATTR_H
 # include <sys/xattr.h>
 #endif
-#include "test.h"
-#include "safe_macros.h"
 
-char *TCID = "getxattr01";
+#include "tst_test.h"
+#include "tst_test_macros.h"
 
-#ifdef HAVE_SYS_XATTR_H
+#define MNTPOINT "mntpoint"
 #define XATTR_TEST_KEY "user.testkey"
 #define XATTR_TEST_VALUE "this is a test value"
 #define XATTR_TEST_VALUE_SIZE 20
 #define BUFFSIZE 64
 
-static void setup(void);
-static void cleanup(void);
-
-char filename[BUFSIZ];
+static char filename[BUFSIZ];
+static char *workdir;
 
 struct test_case {
 	char *fname;
 	char *key;
-	char *value;
+	char value[BUFFSIZE];
 	size_t size;
 	int exp_err;
-};
-struct test_case tc[] = {
-	{			/* case 00, get non-existing attribute */
+} tcases[] = {
+	{ /* case 00, get non-existing attribute */
 	 .fname = filename,
 	 .key = "user.nosuchkey",
-	 .value = NULL,
+	 .value = {0},
 	 .size = BUFFSIZE - 1,
 	 .exp_err = ENODATA,
-	 },
-	{			/* case 01, small value buffer */
+	},
+	{ /* case 01, small value buffer */
 	 .fname = filename,
 	 .key = XATTR_TEST_KEY,
-	 .value = NULL,
+	 .value = {0},
 	 .size = 1,
 	 .exp_err = ERANGE,
-	 },
-	{			/* case 02, get existing attribute */
+	},
+	{ /* case 02, get existing attribute */
 	 .fname = filename,
 	 .key = XATTR_TEST_KEY,
-	 .value = NULL,
+	 .value = {0},
 	 .size = BUFFSIZE - 1,
 	 .exp_err = 0,
-	 },
+	},
 };
 
-int TST_TOTAL = sizeof(tc) / sizeof(tc[0]) + 1;
-
-int main(int argc, char *argv[])
+static void run(unsigned int i)
 {
-	int lc;
-	int i;
-
-	tst_parse_opts(argc, argv, NULL, NULL);
-
-	setup();
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-		tst_count = 0;
-
-		for (i = 0; i < (int)ARRAY_SIZE(tc); i++) {
-			TEST(getxattr(tc[i].fname, tc[i].key, tc[i].value,
-				      tc[i].size));
-
-			if (TEST_ERRNO == tc[i].exp_err) {
-				tst_resm(TPASS | TTERRNO, "expected behavior");
-			} else {
-				tst_resm(TFAIL | TTERRNO, "unexpected behavior"
-					 "- expected errno %d - Got",
-					 tc[i].exp_err);
-			}
-		}
-
-		if (TEST_RETURN != XATTR_TEST_VALUE_SIZE) {
-			tst_resm(TFAIL,
-				 "getxattr() returned wrong size %ld expected %d",
-				 TEST_RETURN, XATTR_TEST_VALUE_SIZE);
-			continue;
+#ifdef HAVE_SYS_XATTR_H
+	SAFE_CHDIR(workdir);
+
+	/* create test file and set xattr */
+	struct test_case *tc = &tcases[i];
+	snprintf(tc->fname, BUFSIZ, "getxattr01testfile-%u", i);
+	int fd = SAFE_CREAT(tc->fname, 0644);
+	SAFE_CLOSE(fd);
+	TEST(setxattr(tc->fname, XATTR_TEST_KEY, XATTR_TEST_VALUE,
+				  strlen(XATTR_TEST_VALUE), XATTR_CREATE));
+	if (TST_RET < 0) {
+		if (TST_ERR == ENOTSUP) {
+			tst_res(TCONF, "no xattr support in file system");
+			return;
 		}
-
-		if (memcmp(tc[i - 1].value, XATTR_TEST_VALUE, XATTR_TEST_VALUE_SIZE))
-			tst_resm(TFAIL, "Wrong value, expect \"%s\" got \"%s\"",
-				 XATTR_TEST_VALUE, tc[i - 1].value);
-		else
-			tst_resm(TPASS, "Got the right value");
+		tst_res(TFAIL | TTERRNO, "unexpected setxattr() return code");
+		return;
 	}
-
-	cleanup();
-	tst_exit();
-}
-
-static void setup(void)
-{
-	int fd;
-	unsigned int i;
-
-	tst_require_root();
-
-	tst_tmpdir();
-
-	/* Create test file and setup initial xattr */
-	snprintf(filename, BUFSIZ, "getxattr01testfile");
-	fd = SAFE_CREAT(cleanup, filename, 0644);
-	close(fd);
-	if (setxattr(filename, XATTR_TEST_KEY, XATTR_TEST_VALUE,
-		     strlen(XATTR_TEST_VALUE), XATTR_CREATE) == -1) {
-		if (errno == ENOTSUP) {
-			tst_brkm(TCONF, cleanup, "No xattr support in fs or "
-				 "mount without user_xattr option");
-		}
+	tst_res(TPASS | TTERRNO, "expected setxattr() return code");
+
+	/* read xattr back */
+	TEST(getxattr(tc->fname, tc->key, tc->value, tc->size));
+	if (TST_ERR == tc->exp_err) {
+		tst_res(TPASS | TTERRNO, "expected getxattr() return code");
+	} else {
+		tst_res(TFAIL | TTERRNO, "unexpected getxattr() return code"
+				" - expected errno %d", tc->exp_err);
 	}
 
-	/* Prepare test cases */
-	for (i = 0; i < ARRAY_SIZE(tc); i++) {
-		tc[i].value = malloc(BUFFSIZE);
-		if (tc[i].value == NULL) {
-			tst_brkm(TBROK | TERRNO, cleanup,
-				 "Cannot allocate memory");
-		}
+	/* verify the value for non-error test cases */
+	if (tc->exp_err) {
+		return;
 	}
-
-	TEST_PAUSE;
+	if (TST_RET != XATTR_TEST_VALUE_SIZE) {
+		tst_res(TFAIL,
+				"getxattr() returned wrong size %ld expected %d",
+				TST_RET, XATTR_TEST_VALUE_SIZE);
+		return;
+	}
+	if (memcmp(tc->value, XATTR_TEST_VALUE, XATTR_TEST_VALUE_SIZE))
+		tst_res(TFAIL, "wrong value, expected \"%s\" got \"%s\"",
+				XATTR_TEST_VALUE, tc->value);
+	else
+		tst_res(TPASS, "right value");
+#endif
 }
 
-static void cleanup(void)
-{
-	tst_rmdir();
-}
-#else /* HAVE_SYS_XATTR_H */
-int main(int argc, char *argv[])
+static void setup(void)
 {
-	tst_brkm(TCONF, NULL, "<sys/xattr.h> does not exist.");
+#ifdef HAVE_SYS_XATTR_H
+	char *cwd = SAFE_GETCWD(NULL, 0);
+	workdir = SAFE_MALLOC(strlen(cwd) + strlen(MNTPOINT) + 2);
+	sprintf(workdir, "%s/%s", cwd, MNTPOINT);
+	free(cwd);
+#else
+	tst_brk(TCONF, "<sys/xattr.h> does not exist.");
+#endif
 }
+
+static struct tst_test test = {
+#ifdef HAVE_SYS_XATTR_H
+	.all_filesystems = 1,
+	.needs_root = 1,
+	.mntpoint = MNTPOINT,
+	.mount_device = 1,
+	.skip_filesystems = (const char *const []) {
+			"exfat",
+			"tmpfs",
+			"ramfs",
+			"nfs",
+			"vfat",
+			NULL
+	},
 #endif
+	.setup = setup,
+	.test = run,
+	.tcnt = ARRAY_SIZE(tcases)
+};
-- 
2.42.0


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

Re: [LTP] [PATCH v1] Port `getxattr01.c` to new test API

[Andrea Cervesato via ltp]

Hi Marius!

On 9/15/23 13:45, Marius Kittler wrote:
> * Utilize `all_filesystems = 1`-mechanism to test on various file
>    systems instead of relying on the temporary directory's file system
>    to support xattr (which it probably does not as it is commonly a
>    tmpfs)
> * Improve error handling
> * Simplify code and description
> * Related issue: https://github.com/linux-test-project/ltp/issues/583
>
> Signed-off-by: Marius Kittler <mkittler@suse.de>
> ---
>   .../kernel/syscalls/getxattr/getxattr01.c     | 219 ++++++++----------
>   1 file changed, 94 insertions(+), 125 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/getxattr/getxattr01.c b/testcases/kernel/syscalls/getxattr/getxattr01.c
> index cec802a33..e9d5874d9 100644
> --- a/testcases/kernel/syscalls/getxattr/getxattr01.c
> +++ b/testcases/kernel/syscalls/getxattr/getxattr01.c
> @@ -1,38 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
>   /*
>    * Copyright (C) 2011  Red Hat, Inc.
> - *
> - * This program is free software; you can redistribute it and/or
> - * modify it under the terms of version 2 of the GNU General Public
> - * License as published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it would be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> - *
> - * Further, this software is distributed without any warranty that it
> - * is free of the rightful claim of any third person regarding
> - * infringement or the like.  Any license provided herein, whether
> - * implied or otherwise, applies only to this software file.  Patent
> - * licenses, if any, provided herein do not apply to combinations of
> - * this program with other software, or any other product whatsoever.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, write the Free Software
> - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> - * 02110-1301, USA.
> + * Copyright (c) 2023 Marius Kittler <mkittler@suse.de>
>    */
>   
> -/*
> - * Basic tests for getxattr(2) and make sure getxattr(2) handles error
> - * conditions correctly.
> +/*\
> + * [Description]
>    *
> - * There are 4 test cases:
> + * Basic tests for getxattr(2), there are 3 test cases:
>    * 1. Get an non-existing attribute,
> - *    getxattr(2) should return -1 and set errno to ENODATA
> + *    getxattr(2) should return -1 and set errno to ENODATA.
>    * 2. Buffer size is smaller than attribute value size,
> - *    getxattr(2) should return -1 and set errno to ERANGE
> - * 3. Get attribute, getxattr(2) should succeed
> - * 4. Verify the attribute got by getxattr(2) is same as the value we set
> + *    getxattr(2) should return -1 and set errno to ERANGE.
> + * 3. getxattr(2) should succeed and return the same value we set
> + *    before.
You can use '-' instead of numbers here. That will be automatically seen 
as a list by asciidoc.
>    */
>   
>   #include "config.h"
> @@ -47,138 +28,126 @@
We can get rid from most of the includes, since tst_test.h more often 
have them already. I would suggest to remove them all and add only what 
it's needed for compile.
>   #ifdef HAVE_SYS_XATTR_H
>   # include <sys/xattr.h>
>   #endif
> -#include "test.h"
> -#include "safe_macros.h"
>   
> -char *TCID = "getxattr01";
> +#include "tst_test.h"
> +#include "tst_test_macros.h"
>   
> -#ifdef HAVE_SYS_XATTR_H
> +#define MNTPOINT "mntpoint"
>   #define XATTR_TEST_KEY "user.testkey"
>   #define XATTR_TEST_VALUE "this is a test value"
>   #define XATTR_TEST_VALUE_SIZE 20
>   #define BUFFSIZE 64
>   
> -static void setup(void);
> -static void cleanup(void);
> -
> -char filename[BUFSIZ];
> +static char filename[BUFSIZ];
> +static char *workdir;
>   
>   struct test_case {
>   	char *fname;
>   	char *key;
> -	char *value;
> +	char value[BUFFSIZE];
>   	size_t size;
>   	int exp_err;
> -};
> -struct test_case tc[] = {
> -	{			/* case 00, get non-existing attribute */
> +} tcases[] = {
> +	{ /* case 00, get non-existing attribute */
>   	 .fname = filename,
>   	 .key = "user.nosuchkey",
> -	 .value = NULL,
> +	 .value = {0},
>   	 .size = BUFFSIZE - 1,
>   	 .exp_err = ENODATA,
> -	 },
> -	{			/* case 01, small value buffer */
> +	},
> +	{ /* case 01, small value buffer */
>   	 .fname = filename,
>   	 .key = XATTR_TEST_KEY,
> -	 .value = NULL,
> +	 .value = {0},
>   	 .size = 1,
>   	 .exp_err = ERANGE,
> -	 },
> -	{			/* case 02, get existing attribute */
> +	},
> +	{ /* case 02, get existing attribute */
>   	 .fname = filename,
>   	 .key = XATTR_TEST_KEY,
> -	 .value = NULL,
> +	 .value = {0},
>   	 .size = BUFFSIZE - 1,
>   	 .exp_err = 0,
> -	 },
> +	},
>   };
>   
> -int TST_TOTAL = sizeof(tc) / sizeof(tc[0]) + 1;
> -
> -int main(int argc, char *argv[])
> +static void run(unsigned int i)
>   {
> -	int lc;
> -	int i;
> -
> -	tst_parse_opts(argc, argv, NULL, NULL);
> -
> -	setup();
> -
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -		tst_count = 0;
> -
> -		for (i = 0; i < (int)ARRAY_SIZE(tc); i++) {
> -			TEST(getxattr(tc[i].fname, tc[i].key, tc[i].value,
> -				      tc[i].size));
> -
> -			if (TEST_ERRNO == tc[i].exp_err) {
> -				tst_resm(TPASS | TTERRNO, "expected behavior");
> -			} else {
> -				tst_resm(TFAIL | TTERRNO, "unexpected behavior"
> -					 "- expected errno %d - Got",
> -					 tc[i].exp_err);
> -			}
> -		}
> -
> -		if (TEST_RETURN != XATTR_TEST_VALUE_SIZE) {
> -			tst_resm(TFAIL,
> -				 "getxattr() returned wrong size %ld expected %d",
> -				 TEST_RETURN, XATTR_TEST_VALUE_SIZE);
> -			continue;
> +#ifdef HAVE_SYS_XATTR_H

We usually check for headers only once at the beginning of the file, 
just after includes, then use:

#else
TST_TEST_TCONF("System doesn't have <sys/xattr.h>");
#endif

Check fgetxattr01.c source code to get the example.

> +	SAFE_CHDIR(workdir);
> +
> +	/* create test file and set xattr */
> +	struct test_case *tc = &tcases[i];
> +	snprintf(tc->fname, BUFSIZ, "getxattr01testfile-%u", i);
> +	int fd = SAFE_CREAT(tc->fname, 0644);
> +	SAFE_CLOSE(fd);
> +	TEST(setxattr(tc->fname, XATTR_TEST_KEY, XATTR_TEST_VALUE,
> +				  strlen(XATTR_TEST_VALUE), XATTR_CREATE));
Here you can use TST_EXP_PASS instead of TEST
> +	if (TST_RET < 0) {
> +		if (TST_ERR == ENOTSUP) {
> +			tst_res(TCONF, "no xattr support in file system");
> +			return;
>   		}
> -
> -		if (memcmp(tc[i - 1].value, XATTR_TEST_VALUE, XATTR_TEST_VALUE_SIZE))
> -			tst_resm(TFAIL, "Wrong value, expect \"%s\" got \"%s\"",
> -				 XATTR_TEST_VALUE, tc[i - 1].value);
> -		else
> -			tst_resm(TPASS, "Got the right value");
> +		tst_res(TFAIL | TTERRNO, "unexpected setxattr() return code");
> +		return;
>   	}
> -
> -	cleanup();
> -	tst_exit();
> -}
> -
> -static void setup(void)
> -{
> -	int fd;
> -	unsigned int i;
> -
> -	tst_require_root();
> -
> -	tst_tmpdir();
> -
> -	/* Create test file and setup initial xattr */
> -	snprintf(filename, BUFSIZ, "getxattr01testfile");
> -	fd = SAFE_CREAT(cleanup, filename, 0644);
> -	close(fd);
> -	if (setxattr(filename, XATTR_TEST_KEY, XATTR_TEST_VALUE,
> -		     strlen(XATTR_TEST_VALUE), XATTR_CREATE) == -1) {
> -		if (errno == ENOTSUP) {
> -			tst_brkm(TCONF, cleanup, "No xattr support in fs or "
> -				 "mount without user_xattr option");
> -		}
> +	tst_res(TPASS | TTERRNO, "expected setxattr() return code");
> +
> +	/* read xattr back */
> +	TEST(getxattr(tc->fname, tc->key, tc->value, tc->size));
> +	if (TST_ERR == tc->exp_err) {
> +		tst_res(TPASS | TTERRNO, "expected getxattr() return code");
> +	} else {
> +		tst_res(TFAIL | TTERRNO, "unexpected getxattr() return code"
> +				" - expected errno %d", tc->exp_err);
>   	}
Also here you can use TST_EXP_PASS instead of TEST
>   
> -	/* Prepare test cases */
> -	for (i = 0; i < ARRAY_SIZE(tc); i++) {
> -		tc[i].value = malloc(BUFFSIZE);
> -		if (tc[i].value == NULL) {
> -			tst_brkm(TBROK | TERRNO, cleanup,
> -				 "Cannot allocate memory");
> -		}
> +	/* verify the value for non-error test cases */
> +	if (tc->exp_err) {
> +		return;
>   	}
> -
> -	TEST_PAUSE;
> +	if (TST_RET != XATTR_TEST_VALUE_SIZE) {
Here you can use TST_EXP_EQ_LI
> +		tst_res(TFAIL,
> +				"getxattr() returned wrong size %ld expected %d",
> +				TST_RET, XATTR_TEST_VALUE_SIZE);
> +		return;
> +	}
> +	if (memcmp(tc->value, XATTR_TEST_VALUE, XATTR_TEST_VALUE_SIZE))
> +		tst_res(TFAIL, "wrong value, expected \"%s\" got \"%s\"",
> +				XATTR_TEST_VALUE, tc->value);
> +	else
> +		tst_res(TPASS, "right value");
> +#endif
>   }
>   
> -static void cleanup(void)
> -{
> -	tst_rmdir();
> -}
> -#else /* HAVE_SYS_XATTR_H */
> -int main(int argc, char *argv[])
> +static void setup(void)
>   {
> -	tst_brkm(TCONF, NULL, "<sys/xattr.h> does not exist.");
> +#ifdef HAVE_SYS_XATTR_H
> +	char *cwd = SAFE_GETCWD(NULL, 0);
> +	workdir = SAFE_MALLOC(strlen(cwd) + strlen(MNTPOINT) + 2);
> +	sprintf(workdir, "%s/%s", cwd, MNTPOINT);
> +	free(cwd);
Here you can just SAFE_TOUCH the file if you aim to create one.
> +#else
> +	tst_brk(TCONF, "<sys/xattr.h> does not exist.");
> +#endif
>   }
> +
> +static struct tst_test test = {
> +#ifdef HAVE_SYS_XATTR_H
> +	.all_filesystems = 1,
> +	.needs_root = 1,
> +	.mntpoint = MNTPOINT,
> +	.mount_device = 1,
> +	.skip_filesystems = (const char *const []) {
> +			"exfat",
> +			"tmpfs",
> +			"ramfs",
> +			"nfs",
> +			"vfat",
> +			NULL
> +	},
>   #endif
> +	.setup = setup,
> +	.test = run,
> +	.tcnt = ARRAY_SIZE(tcases)
> +};

In your case, to understand LTP API a bit better, I would take 
fxgetattr01 as reference, more or less.

Always remember to run "make check-<name of my test>" before sending any 
patches, so you can be 100% the code has no text format issues.

Same comments apply to getxattr02 more or less.

Best regards,
Andrea Cervesato


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

Re: [LTP] [PATCH v1] Port `getxattr01.c` to new test API

[Marius Kittler]

Am Freitag, 15. September 2023, 16:10:11 CEST schrieb Andrea Cervesato via 
ltp:
> > +
> > +	/* create test file and set xattr */
> > +	struct test_case *tc = &tcases[i];
> > +	snprintf(tc->fname, BUFSIZ, "getxattr01testfile-%u", i);
> > +	int fd = SAFE_CREAT(tc->fname, 0644);
> > +	SAFE_CLOSE(fd);
> > +	TEST(setxattr(tc->fname, XATTR_TEST_KEY, XATTR_TEST_VALUE,
> > +				  strlen(XATTR_TEST_VALUE), 
XATTR_CREATE));
> 
> Here you can use TST_EXP_PASS instead of TEST
> 

I wouldn't know how. If this function fails with ENOTSUP specifically, then the 
result should be TCONF. I don't think using TST_EXP_PASS allows that. 
Additionally, if this function fails the remaining tests for this iteration 
should be skipped.

> > +
> > +	/* read xattr back */
> > +	TEST(getxattr(tc->fname, tc->key, tc->value, tc->size));
> > +	if (TST_ERR == tc->exp_err) {
> > +		tst_res(TPASS | TTERRNO, "expected getxattr() return 
code");
> > +	} else {
> > +		tst_res(TFAIL | TTERRNO, "unexpected getxattr() 
return code"
> > +				" - expected errno %d", tc-
>exp_err);
> > 
> >   	}
> 
> Also here you can use TST_EXP_PASS instead of TEST

Also here I wouldn't know how that is possible. There are test cases where an 
error is actually expected.

> > 
> > -	tst_brkm(TCONF, NULL, "<sys/xattr.h> does not exist.");
> > +#ifdef HAVE_SYS_XATTR_H
> > +	char *cwd = SAFE_GETCWD(NULL, 0);
> > +	workdir = SAFE_MALLOC(strlen(cwd) + strlen(MNTPOINT) + 2);
> > +	sprintf(workdir, "%s/%s", cwd, MNTPOINT);
> > +	free(cwd);
> 
> Here you can just SAFE_TOUCH the file if you aim to create one.
> 

Using SAFE_TOUCH() makes sense but I guess the file creation needs to be in 
run() because a file needs to be created per mount. Additionally, one then had 
to create a loop for all test cases.

> In your case, to understand LTP API a bit better, I would take
> fxgetattr01 as reference, more or less.

That test seems very similar, indeed. Do you want me to follow some patterns 
from there specifically? Note that this test also uses just "TEST(…)" similarly 
to my code.

It does a simplification by using SAFE_FSETXATTR and here I could use 
SAFE_SETXATTR. However, then we would lose the ENOTSUP case which I'm not sure 
we want/should lose (especially since I'm mainly porting/refactoring it might 
not be desirable to remove behavior at the same time).



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

Re: [LTP] [PATCH v1] Port `getxattr01.c` to new test API

[Andrea Cervesato via ltp]

Hi!

On 9/18/23 14:50, Marius Kittler wrote:
> Am Freitag, 15. September 2023, 16:10:11 CEST schrieb Andrea Cervesato via
> ltp:
>>> +
>>> +	/* create test file and set xattr */
>>> +	struct test_case *tc = &tcases[i];
>>> +	snprintf(tc->fname, BUFSIZ, "getxattr01testfile-%u", i);
>>> +	int fd = SAFE_CREAT(tc->fname, 0644);
>>> +	SAFE_CLOSE(fd);
>>> +	TEST(setxattr(tc->fname, XATTR_TEST_KEY, XATTR_TEST_VALUE,
>>> +				  strlen(XATTR_TEST_VALUE),
> XATTR_CREATE));
>> Here you can use TST_EXP_PASS instead of TEST
>>
> I wouldn't know how. If this function fails with ENOTSUP specifically, then the
> result should be TCONF. I don't think using TST_EXP_PASS allows that.
> Additionally, if this function fails the remaining tests for this iteration
> should be skipped.
>
>>> +
>>> +	/* read xattr back */
>>> +	TEST(getxattr(tc->fname, tc->key, tc->value, tc->size));
>>> +	if (TST_ERR == tc->exp_err) {
>>> +		tst_res(TPASS | TTERRNO, "expected getxattr() return
> code");
>>> +	} else {
>>> +		tst_res(TFAIL | TTERRNO, "unexpected getxattr()
> return code"
>>> +				" - expected errno %d", tc-
>> exp_err);
>>>    	}
>> Also here you can use TST_EXP_PASS instead of TEST
> Also here I wouldn't know how that is possible. There are test cases where an
> error is actually expected.

If an error is expected you can use TST_EXP_FAIL, but yes, taking a 
closer look at the code,
this case is a bit more specific and TEST can be used.

>
>>> -	tst_brkm(TCONF, NULL, "<sys/xattr.h> does not exist.");
>>> +#ifdef HAVE_SYS_XATTR_H
>>> +	char *cwd = SAFE_GETCWD(NULL, 0);
>>> +	workdir = SAFE_MALLOC(strlen(cwd) + strlen(MNTPOINT) + 2);
>>> +	sprintf(workdir, "%s/%s", cwd, MNTPOINT);
>>> +	free(cwd);
>> Here you can just SAFE_TOUCH the file if you aim to create one.
>>
> Using SAFE_TOUCH() makes sense but I guess the file creation needs to be in
> run() because a file needs to be created per mount. Additionally, one then had
> to create a loop for all test cases.
>
>> In your case, to understand LTP API a bit better, I would take
>> fxgetattr01 as reference, more or less.
> That test seems very similar, indeed. Do you want me to follow some patterns
> from there specifically? Note that this test also uses just "TEST(…)" similarly
> to my code.
>
> It does a simplification by using SAFE_FSETXATTR and here I could use
> SAFE_SETXATTR. However, then we would lose the ENOTSUP case which I'm not sure
> we want/should lose (especially since I'm mainly porting/refactoring it might
> not be desirable to remove behavior at the same time).
>
>
>
You can use fxgetattr01 as reference.
Beware of "make check" before sending the patch.

Andrea


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