Re: [LTP] [PATCH 3/8] setxattr01: add setxattrat variant

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> diff --git a/testcases/kernel/syscalls/setxattr/setxattr01.c b/testcases/kernel/syscalls/setxattr/setxattr01.c
> index de3ea67ec4000905651f20e2684a6b0aef493da7..67b053c4a2593df6cd2800c5450b5951ff78ae0d 100644
> --- a/testcases/kernel/syscalls/setxattr/setxattr01.c
> +++ b/testcases/kernel/syscalls/setxattr/setxattr01.c
> @@ -36,6 +36,8 @@
>  # include <sys/xattr.h>
>  #endif
>  #include "tst_test.h"
> +#include "lapi/syscalls.h"
> +#include "lapi/xattr.h"
>  
>  #ifdef HAVE_SYS_XATTR_H
>  #define XATTR_NAME_MAX 255
> @@ -45,11 +47,13 @@
>  #define XATTR_TEST_VALUE "this is a test value"
>  #define XATTR_TEST_VALUE_SIZE 20
>  #define MNTPOINT "mntpoint"
> -#define FNAME MNTPOINT"/setxattr01testfile"
> +#define FNAME_REL "setxattr01testfile"
> +#define FNAME MNTPOINT"/"FNAME_REL
>  
>  static char long_key[XATTR_NAME_LEN];
>  static char *long_value;
>  static char *xattr_value = XATTR_TEST_VALUE;
> +static int mnt_fd = -1;
>  
>  struct test_case {
>  	char *key;
> @@ -128,44 +132,65 @@ struct test_case tc[] = {
>  
>  static void verify_setxattr(unsigned int i)
>  {
> +	char *sysname;
> +
>  	/* some tests might require existing keys for each iteration */
>  	if (tc[i].keyneeded) {
>  		SAFE_SETXATTR(FNAME, tc[i].key, *tc[i].value, tc[i].size,
>  				XATTR_CREATE);
>  	}
>  
> -	TEST(setxattr(FNAME, tc[i].key, *tc[i].value, tc[i].size, tc[i].flags));
> +	if (tst_variant) {
> +		sysname = "setxattrat";
> +
> +		struct xattr_args args = {
> +			.value = tc[i].value,
> +			.size = tc[i].size,
> +			.flags = tc[i].flags,
> +		};
> +
> +		TEST(tst_syscall(__NR_setxattrat,
> +			mnt_fd, FNAME_REL, AT_SYMLINK_NOFOLLOW,
> +			tc[i].key, &args, sizeof(args)));

The setxattrat() function should be put into the lapi/setxattr.h and
enabled only if configure script didn't find setxattrat() in the
sys/setxattr.h. That way we will switch to the system definition of the
function once it's available.

Any time we are adding tests for syscalls that are not in the libc now
but will end up there in the future we have to do this:

* Add AC_CHECK_FUNC() into configure
* Add syscall wrapper inside ifndef HAVE_FUNC block into respective lapi header
* Include the lapi header in the test
* Call the function as func() and not by tst_syscall(__NR_func, ...)

> +	} else {
> +		sysname = "setxattr";
> +
> +		TEST(setxattr(
> +			FNAME,
> +			tc[i].key, *tc[i].value, tc[i].size,
> +			tc[i].flags));
> +	}

This pattern repeats in several tests, so it would make more sense to
write a wrapper that would call the respective syscall based on
tst_variant and put it into a common header. i.e.

static inline int call_setxattr(const char *fname, const char *name,
                                const void *value, size_t line, int flags)
{
	if (tst_variant) {
		struct xattr_args args = {
			.value = tc[i].value,
			.size = tc[i].size,
			.flags = tc[i].flags,
		};

		return setxattrat(...);
	} else {
		return setxattr(...);
	}
}

>  	if (TST_RET == -1 && TST_ERR == EOPNOTSUPP)
> -		tst_brk(TCONF, "setxattr(2) not supported");
> +		tst_brk(TCONF, "%s(2) not supported", sysname);
>  
>  	/* success */
>  
>  	if (!tc[i].exp_err) {
>  		if (TST_RET) {
>  			tst_res(TFAIL | TTERRNO,
> -				"setxattr(2) failed with %li", TST_RET);
> +				"%s(2) failed with %li", sysname, TST_RET);
>  			return;
>  		}
>  
>  		/* this is needed for subsequent iterations */
>  		SAFE_REMOVEXATTR(FNAME, tc[i].key);
>  
> -		tst_res(TPASS, "setxattr(2) passed");
> +		tst_res(TPASS, "%s(2) passed", sysname);
>  
>  		return;
>  	}
>  
>  	if (TST_RET == 0) {
> -		tst_res(TFAIL, "setxattr(2) passed unexpectedly");
> +		tst_res(TFAIL, "%s(2) passed unexpectedly", sysname);
>  		return;
>  	}
>  
>  	/* error */
>  
>  	if (tc[i].exp_err != TST_ERR) {
> -		tst_res(TFAIL | TTERRNO, "setxattr(2) should fail with %s",
> -			tst_strerrno(tc[i].exp_err));
> +		tst_res(TFAIL | TTERRNO, "%s(2) should fail with %s",
> +			sysname, tst_strerrno(tc[i].exp_err));
>  		return;
>  	}
>  
> @@ -173,7 +198,7 @@ static void verify_setxattr(unsigned int i)
>  	if (tc[i].keyneeded)
>  		SAFE_REMOVEXATTR(FNAME, tc[i].key);
>  
> -	tst_res(TPASS | TTERRNO, "setxattr(2) failed");
> +	tst_res(TPASS | TTERRNO, "%s(2) failed", sysname);
>  }
>  
>  static void setup(void)
> @@ -194,12 +219,22 @@ static void setup(void)
>  		if (!tc[i].key)
>  			tc[i].key = tst_get_bad_addr(NULL);
>  	}
> +
> +	mnt_fd = SAFE_OPEN(MNTPOINT, O_DIRECTORY);
> +}
> +
> +static void cleanup(void)
> +{
> +	if (mnt_fd != -1)
> +		SAFE_CLOSE(mnt_fd);
>  }
>  
>  static struct tst_test test = {
>  	.setup = setup,
> +	.cleanup = cleanup,
>  	.test = verify_setxattr,
>  	.tcnt = ARRAY_SIZE(tc),
> +	.test_variants = 2,
>  	.mntpoint = MNTPOINT,
>  	.mount_device = 1,
>  	.all_filesystems = 1,
> 
> -- 
> 2.43.0
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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