Re: [LTP] [PATCH v2 2/2] setxattr02: Adapt test for kernel 7.1.0+ socket xattr support

[Petr Vorel <pvorel@suse.cz>]

Hi Jan,

> Starting with kernel 7.1.0, sockets support extended attributes in the
> user.* namespace. This behavior was enabled by dc0876b9846d ("xattr:
> support extended attributes on sockets"), which permits user.* xattrs on
> S_IFSOCK inodes (previously rejected with -EPERM).

> Adapts the test to expect success (exp_err = 0) instead of EPERM when
> testing setxattr(2) on sockets with kernel 7.1.0+.

> Signed-off-by: Jan Polensky <japo@linux.ibm.com>
> ---
>  testcases/kernel/syscalls/setxattr/setxattr02.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

> diff --git a/testcases/kernel/syscalls/setxattr/setxattr02.c b/testcases/kernel/syscalls/setxattr/setxattr02.c
> index 9f5f998da9ba..c905f42dbb9e 100644
> --- a/testcases/kernel/syscalls/setxattr/setxattr02.c
> +++ b/testcases/kernel/syscalls/setxattr/setxattr02.c
> @@ -15,7 +15,7 @@
>   * - EPERM - set attribute to a FIFO
>   * - EPERM - set attribute to a char special file
>   * - EPERM - set attribute to a block special file
> - * - EPERM - set attribute to a UNIX domain socket
> + * - EPERM/SUCCEED - set attribute to a UNIX domain socket (commit dc0876b9846d)
>   */

>  #include "config.h"
> @@ -132,6 +132,10 @@ static void verify_setxattr(unsigned int i)
>  	if (TST_RET == -1 && TST_ERR == EOPNOTSUPP)
>  		tst_brk(TCONF, "setxattr(2) not supported");

> +	/* consider kernels with commit dc0876b9846d */
> +	if ((tst_kvercmp(7, 1, 0) >= 0) && (strncmp(tc[i].fname, SOCK, strlen(SOCK)) == 0))
> +		tc[i].exp_err = 0;
I'm sorry I haven't pointed out it on v1.

1) Kernel detection should be in the setup function (otherwise kernel detection
is done 7x, but needed only 1x, and when run more times with -i1000 it's even
more time wasted), result stored into static variable, see:
testcases/kernel/syscalls/userfaultfd/userfaultfd01.c

2) While checking file name works, we usually try to somehow move the check into
struct test_case.

testcases/kernel/syscalls/finit_module/finit_module02.c
function pointer to in the test struct, function fixes the errno, code run in
the setup.

In this case when a single case requires it, I would combine these 2 approaches:

1) detect kver in the setup

#define BEFORE_7_1 1
#define AFTER_7_1 2
static int kver;

static void setup(void)
{
	...
	if (tst_kvercmp(7, 1, 0) >= 0)
		kver = AFTER_7_1;
	else
		kver = BEFORE_7_1;
}

2) custom function
struct test_case {
	...
	void (*fix_errno)(struct test_case *tc);
};

static void fix_sock(struct tcase *tc)
{
	/* dc0876b9846d ("xattr: support extended attributes on sockets") */
	if (kver == AFTER_7_1)
		tc[i].exp_err = 0;
}

static void verify_setxattr(unsigned int i)
{
	if (tc[i].fix_errno)
		tc[i].fix_errno(&tc[i]);
	...
}

Also, it helps reviewing when your cover letter contains changelog.
On single patch it can be added under --- (so that it's not part of the commit
message.

Kind regards,
Petr
> +
>  	/* success */

>  	if (!tc[i].exp_err) {

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