[v2] Rewrite statx04 test

statx04 is supposed verify that inode attribute support in statx() matches
what should actually be implemented. However, we already have functional tests
for this in statx08 and lack a test of the stx_attribute_mask field which
reports inode attribute support to userspace.

Rewrite the test to drop the duplicate code and add the missing coverage.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---

Changes since v1:
- added known-fail tag for XFS in strict mode
- changed copyright header to SUSE since this is a near-complete rewrite

This also fixes Btrfs failures on kernels v4.11 and v4.12 where the attributes
are not supported at all. LTP is supposed to check for bugs, not force people
to backport features from newer kernels.

If anybody want to backport the Btrfs support, the test now has strict mode
which disables kernel version exceptions.

I've also disabled FUSE filesystems because they return wrong errno codes
in the ioctl() check and NTFS3g now has some rudimentary support for inode
attributes but no support for them in statx().

 testcases/kernel/syscalls/statx/statx04.c | 196 +++++++---------------
 1 file changed, 64 insertions(+), 132 deletions(-)

Hi Martin,

> statx04 is supposed verify that inode attribute support in statx() matches
> what should actually be implemented. However, we already have functional tests
> for this in statx08 and lack a test of the stx_attribute_mask field which
> reports inode attribute support to userspace.

> Rewrite the test to drop the duplicate code and add the missing coverage.

> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> ---

> Changes since v1:
> - added known-fail tag for XFS in strict mode
> - changed copyright header to SUSE since this is a near-complete rewrite
Thanks a lot!

> This also fixes Btrfs failures on kernels v4.11 and v4.12 where the attributes
> are not supported at all. LTP is supposed to check for bugs, not force people
> to backport features from newer kernels.

> If anybody want to backport the Btrfs support, the test now has strict mode
> which disables kernel version exceptions.

> I've also disabled FUSE filesystems because they return wrong errno codes
> in the ioctl() check and NTFS3g now has some rudimentary support for inode
> attributes but no support for them in statx().

I'd put this into commit message (can be done during merge).

...
> +	if (TST_RET)
> +		tst_brk(TBROK | TTERRNO, "Unexpected ioctl() error");

> -static void test_unflagged(void)
> -{
> -	struct statx buf;
> +	for (i = 0, expected_mask = 0; i < ARRAY_SIZE(attr_list); i++)
Before merge we should cast to prevent "comparison of integer expressions of
different signedness" warning:
for (i = 0, expected_mask = 0; i < (int)ARRAY_SIZE(attr_list); i++)

> +		expected_mask |= attr_list[i].attr;

...
> -static void setup(void)
> -{
> -	SAFE_MKDIR(TESTDIR_FLAGGED, 0777);
> -	SAFE_MKDIR(TESTDIR_UNFLAGGED, 0777);
> +	for (i = 0; i < ARRAY_SIZE(attr_list); i++) {
And here:
for (i = 0; i < (int)ARRAY_SIZE(attr_list); i++) {
> +		if (!(expected_mask & attr_list[i].attr))
> +			continue;

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

On 19. 01. 22 12:23, Petr Vorel wrote:
> Hi Martin,
> 
>> ...
> 
> I'd put this into commit message (can be done during merge).

Sure.

>> +	for (i = 0, expected_mask = 0; i < ARRAY_SIZE(attr_list); i++)
> Before merge we should cast to prevent "comparison of integer expressions of
> different signedness" warning:
> for (i = 0, expected_mask = 0; i < (int)ARRAY_SIZE(attr_list); i++)

It's better to fix that by changing the type of "i" to size_t or
unsigned int. But GCC could easily deduce that one of the values is a
constant well within the range of signed int and not bother us with the
warning...

> On 19. 01. 22 12:23, Petr Vorel wrote:
> > Hi Martin,

> >> ...

> > I'd put this into commit message (can be done during merge).

> Sure.

> >> +	for (i = 0, expected_mask = 0; i < ARRAY_SIZE(attr_list); i++)
> > Before merge we should cast to prevent "comparison of integer expressions of
> > different signedness" warning:
> > for (i = 0, expected_mask = 0; i < (int)ARRAY_SIZE(attr_list); i++)

> It's better to fix that by changing the type of "i" to size_t or
+1

> unsigned int. But GCC could easily deduce that one of the values is a
> constant well within the range of signed int and not bother us with the
> warning...
Agree :).

Kind regards,
Petr

Hi!
> statx04 is supposed verify that inode attribute support in statx() matches
> what should actually be implemented. However, we already have functional tests
> for this in statx08 and lack a test of the stx_attribute_mask field which
> reports inode attribute support to userspace.
> 
> Rewrite the test to drop the duplicate code and add the missing coverage.
> 
> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> ---
> 
> Changes since v1:
> - added known-fail tag for XFS in strict mode
> - changed copyright header to SUSE since this is a near-complete rewrite
> 
> This also fixes Btrfs failures on kernels v4.11 and v4.12 where the attributes
> are not supported at all. LTP is supposed to check for bugs, not force people
> to backport features from newer kernels.
> 
> If anybody want to backport the Btrfs support, the test now has strict mode
> which disables kernel version exceptions.
> 
> I've also disabled FUSE filesystems because they return wrong errno codes
> in the ioctl() check and NTFS3g now has some rudimentary support for inode
> attributes but no support for them in statx().
> 
>  testcases/kernel/syscalls/statx/statx04.c | 196 +++++++---------------
>  1 file changed, 64 insertions(+), 132 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/statx/statx04.c b/testcases/kernel/syscalls/statx/statx04.c
> index a3ca436f5..6ac47a8bd 100644
> --- a/testcases/kernel/syscalls/statx/statx04.c
> +++ b/testcases/kernel/syscalls/statx/statx04.c
> @@ -1,14 +1,12 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  /*
> - * Copyright (c) Zilogic Systems Pvt. Ltd., 2018
> - * Email: code@zilogic.com
> + * Copyright (c) 2022 SUSE LLC <mdoucha@suse.cz>
>   */
>  
>  /*\
>   * [Description]
>   *
> - * This code tests if the attributes field of statx received expected value.
> - * File set with following flags by using SAFE_IOCTL:
> + * Test whether the kernel properly advertises support for statx() attributes:
>   *
>   * - STATX_ATTR_COMPRESSED: The file is compressed by the filesystem.
>   * - STATX_ATTR_IMMUTABLE: The file cannot be modified.
> @@ -16,9 +14,6 @@
>   * - STATX_ATTR_NODUMP: File is not a candidate for backup when a backup
>   *                        program such as dump(8) is run.
>   *
> - * Two directories are tested.
> - * First directory has all flags set. Second directory has no flags set.
> - *
>   * xfs filesystem doesn't support STATX_ATTR_COMPRESSED flag, so we only test
>   * three other flags.
>   *
> @@ -56,165 +51,102 @@
>  #include "lapi/stat.h"
>  
>  #define MOUNT_POINT "mntpoint"
> -#define TESTDIR_FLAGGED MOUNT_POINT"/test_dir1"
> -#define TESTDIR_UNFLAGGED MOUNT_POINT"/test_dir2"
> +#define TESTDIR MOUNT_POINT "/testdir"
> +
> +#define ATTR(x) {.attr = x, .name = #x}
> +
> +static struct {
> +	uint64_t attr;
> +	const char *name;
> +} attr_list[] = {
> +	ATTR(STATX_ATTR_COMPRESSED),
> +	ATTR(STATX_ATTR_APPEND),
> +	ATTR(STATX_ATTR_IMMUTABLE),
> +	ATTR(STATX_ATTR_NODUMP)
> +};
>  
> -static int fd, clear_flags;
> +static uint64_t expected_mask;
> +static char *strict;
>  
> -static void test_flagged(void)
> +static void setup(void)
>  {
> -	struct statx buf;
> +	int i, fd;
>  
> -	TEST(statx(AT_FDCWD, TESTDIR_FLAGGED, 0, 0, &buf));
> -	if (TST_RET == 0)
> -		tst_res(TPASS,
> -			"sys_statx(AT_FDCWD, %s, 0, 0, &buf)", TESTDIR_FLAGGED);
> -	else
> -		tst_brk(TFAIL | TTERRNO,
> -			"sys_statx(AT_FDCWD, %s, 0, 0, &buf)", TESTDIR_FLAGGED);
> -
> -	if (strcmp(tst_device->fs_type, "xfs")) {
> -		if (buf.stx_attributes & STATX_ATTR_COMPRESSED)
> -			tst_res(TPASS, "STATX_ATTR_COMPRESSED flag is set");
> -		else
> -			tst_res(TFAIL, "STATX_ATTR_COMPRESSED flag is not set");
> -	}
> +	SAFE_MKDIR(TESTDIR, 0777);
>  
> -	if (buf.stx_attributes & STATX_ATTR_APPEND)
> -		tst_res(TPASS, "STATX_ATTR_APPEND flag is set");
> -	else
> -		tst_res(TFAIL, "STATX_ATTR_APPEND flag is not set");
> +	/* Check general inode attribute support */
> +	fd = SAFE_OPEN(TESTDIR, O_RDONLY | O_DIRECTORY);
> +	TEST(ioctl(fd, FS_IOC_GETFLAGS, &i));
> +	SAFE_CLOSE(fd);
>  
> -	if (buf.stx_attributes & STATX_ATTR_IMMUTABLE)
> -		tst_res(TPASS, "STATX_ATTR_IMMUTABLE flag is set");
> -	else
> -		tst_res(TFAIL, "STATX_ATTR_IMMUTABLE flag is not set");
> +	if (TST_RET == -1 && TST_ERR == ENOTTY)
> +		tst_brk(TCONF | TTERRNO, "Inode attributes not supported");
>  
> -	if (buf.stx_attributes & STATX_ATTR_NODUMP)
> -		tst_res(TPASS, "STATX_ATTR_NODUMP flag is set");
> -	else
> -		tst_res(TFAIL, "STATX_ATTR_NODUMP flag is not set");
> -}
> +	if (TST_RET)
> +		tst_brk(TBROK | TTERRNO, "Unexpected ioctl() error");
>  
> -static void test_unflagged(void)
> -{
> -	struct statx buf;
> +	for (i = 0, expected_mask = 0; i < ARRAY_SIZE(attr_list); i++)
> +		expected_mask |= attr_list[i].attr;
>  
> -	TEST(statx(AT_FDCWD, TESTDIR_UNFLAGGED, 0, 0, &buf));
> -	if (TST_RET == 0)
> -		tst_res(TPASS,
> -			"sys_statx(AT_FDCWD, %s, 0, 0, &buf)",
> -			TESTDIR_UNFLAGGED);
> -	else
> -		tst_brk(TFAIL | TTERRNO,
> -			"sys_statx(AT_FDCWD, %s, 0, 0, &buf)",
> -			TESTDIR_UNFLAGGED);
> -
> -	if ((buf.stx_attributes & STATX_ATTR_COMPRESSED) == 0)
> -		tst_res(TPASS, "STATX_ATTR_COMPRESSED flag is not set");
> -	else
> -		tst_res(TFAIL, "STATX_ATTR_COMPRESSED flag is set");
> -
> -	if ((buf.stx_attributes & STATX_ATTR_APPEND) == 0)
> -		tst_res(TPASS, "STATX_ATTR_APPEND flag is not set");
> -	else
> -		tst_res(TFAIL, "STATX_ATTR_APPEND flag is set");
> -
> -	if ((buf.stx_attributes & STATX_ATTR_IMMUTABLE) == 0)
> -		tst_res(TPASS, "STATX_ATTR_IMMUTABLE flag is not set");
> -	else
> -		tst_res(TFAIL, "STATX_ATTR_IMMUTABLE flag is set");
> -
> -	if ((buf.stx_attributes & STATX_ATTR_NODUMP) == 0)
> -		tst_res(TPASS, "STATX_ATTR_NODUMP flag is not set");
> -	else
> -		tst_res(TFAIL, "STATX_ATTR_NODUMP flag is set");
> -}
> +	/* Strict mode: No exceptions, all attributes must be supported */
> +	if (strict)
> +		return;
>  
> -static struct test_cases {
> -	void (*tfunc)(void);
> -} tcases[] = {
> -	{&test_flagged},
> -	{&test_unflagged},
> -};
> +	/* STATX_ATTR_COMPRESSED not supported on XFS */
> +	if (!strcmp(tst_device->fs_type, "xfs"))
> +		expected_mask &= ~STATX_ATTR_COMPRESSED;
>  
> -static void run(unsigned int i)
> -{
> -	tcases[i].tfunc();
> +	/* Attribute support was added to Btrfs statx() in kernel v4.13 */
> +	if (!strcmp(tst_device->fs_type, "btrfs") && tst_kvercmp(4, 13, 0) < 0)
> +		tst_brk(TCONF, "statx() attributes not supported on Btrfs");
>  }
>  
> -static void caid_flags_setup(void)
> +static void run(void)
>  {
> -	int attr, ret;
> -
> -	fd = SAFE_OPEN(TESTDIR_FLAGGED, O_RDONLY | O_DIRECTORY);
> -
> -	ret = ioctl(fd, FS_IOC_GETFLAGS, &attr);
> -	if (ret < 0) {
> -		if (errno == ENOTTY)
> -			tst_brk(TCONF | TERRNO, "FS_IOC_GETFLAGS not supported");
> -
> -		/* ntfs3g fuse fs returns wrong errno for unimplemented ioctls */
> -		if (!strcmp(tst_device->fs_type, "ntfs")) {
> -			tst_brk(TCONF | TERRNO,
> -				"ntfs3g does not support FS_IOC_GETFLAGS");
> -		}
> +	int i;
> +	struct statx buf;
>  
> -		tst_brk(TBROK | TERRNO, "ioctl(%i, FS_IOC_GETFLAGS, ...)", fd);
> -	}
> +	TEST(statx(AT_FDCWD, TESTDIR, 0, 0, &buf));
>  
> -	if (!strcmp(tst_device->fs_type, "xfs"))
> -		attr |= FS_APPEND_FL | FS_IMMUTABLE_FL | FS_NODUMP_FL;
> -	else
> -		attr |= FS_COMPR_FL | FS_APPEND_FL | FS_IMMUTABLE_FL | FS_NODUMP_FL;
> -
> -	ret = ioctl(fd, FS_IOC_SETFLAGS, &attr);
> -	if (ret < 0) {
> -		if (errno == EOPNOTSUPP)
> -			tst_brk(TCONF, "Flags not supported");
> -		tst_brk(TBROK | TERRNO, "ioctl(%i, FS_IOC_SETFLAGS, %i)", fd, attr);
> +	if (TST_RET == -1) {
> +		tst_brk(TBROK | TTERRNO, "sys_statx() failed");
> +	} else if (TST_RET) {
> +		tst_brk(TBROK | TTERRNO,
> +			"sys_statx() returned invalid value %ld", TST_RET);

Why not just TST_EXP_PASS_SILENT()?

> -	clear_flags = 1;
> -}
> -
> -static void setup(void)
> -{
> -	SAFE_MKDIR(TESTDIR_FLAGGED, 0777);
> -	SAFE_MKDIR(TESTDIR_UNFLAGGED, 0777);
> +	for (i = 0; i < ARRAY_SIZE(attr_list); i++) {
> +		if (!(expected_mask & attr_list[i].attr))
> +			continue;
>  
> -	caid_flags_setup();
> -}
> -
> -static void cleanup(void)
> -{
> -	int attr;
> -
> -	if (clear_flags) {
> -		SAFE_IOCTL(fd, FS_IOC_GETFLAGS, &attr);
> -		attr &= ~(FS_COMPR_FL | FS_APPEND_FL | FS_IMMUTABLE_FL | FS_NODUMP_FL);
> -		SAFE_IOCTL(fd, FS_IOC_SETFLAGS, &attr);
> +		if (buf.stx_attributes_mask & attr_list[i].attr)
> +			tst_res(TPASS, "%s is supported", attr_list[i].name);
> +		else
> +			tst_res(TFAIL, "%s not supported", attr_list[i].name);
>  	}
> -
> -	if (fd > 0)
> -		SAFE_CLOSE(fd);
>  }
>  
>  static struct tst_test test = {
> -	.test = run,
> -	.tcnt = ARRAY_SIZE(tcases),
> +	.test_all = run,
>  	.setup = setup,
> -	.cleanup = cleanup,
>  	.needs_root = 1,
>  	.all_filesystems = 1,
>  	.mount_device = 1,
>  	.mntpoint = MOUNT_POINT,
>  	.min_kver = "4.11",
> +	.options = (struct tst_option[]) {
> +		{"s", &strict, "Strict mode (disable special exceptions)"}
> +	},

This looks like it should have been made a global knob. Maybe
environment variable?

> +	.skip_filesystems = (const char *const[]) {
> +		"fuse",
> +		NULL
> +	},
>  	.tags = (const struct tst_tag[]) {
>  		{"linux-git", "93bc420ed41d"},
>  		{"linux-git", "99652ea56a41"},
>  		{"linux-git", "04a87e347282"},
>  		{"linux-git", "5f955f26f3d4"},
> +		{"known-fail", "STATX_ATTR_COMPRESSED is not implemented on XFS (strict mode)"},
>  		{}
>  	},
>  };

Otherwise I like the direction where this is going.

I vote for merging this before the release with the TEST() and if()s
replaced byt the TST_EXP_PASS() macro and we can decide where and how to
put the strict knob later on.

Cyril Hrubis <chrubis@suse.cz> wrote:

> > +     /* STATX_ATTR_COMPRESSED not supported on XFS */
> > +     if (!strcmp(tst_device->fs_type, "xfs"))
> > +             expected_mask &= ~STATX_ATTR_COMPRESSED;

...
> > +     .skip_filesystems = (const char *const[]) {
> > +             "fuse",
> > +             NULL
> > +     },
> >       .tags = (const struct tst_tag[]) {
> >               {"linux-git", "93bc420ed41d"},
> >               {"linux-git", "99652ea56a41"},
> >               {"linux-git", "04a87e347282"},
> >               {"linux-git", "5f955f26f3d4"},
> > +             {"known-fail", "STATX_ATTR_COMPRESSED is not implemented on XFS (strict mode)"},

I'm wondering if it has necessary to add the known-fail tags here
since we have already done XFS filtering in setup().

> Otherwise I like the direction where this is going.
>
> I vote for merging this before the release with the TEST() and if()s

+1 fine to me, the rewrite looks pretty good if refine with reviewer's comments.

> replaced byt the TST_EXP_PASS() macro and we can decide where and how to
> put the strict knob later on.

On Thu, Jan 20, 2022 at 3:18 PM Li Wang <liwang@redhat.com> wrote:
>
> Cyril Hrubis <chrubis@suse.cz> wrote:
>
> > > +     /* STATX_ATTR_COMPRESSED not supported on XFS */
> > > +     if (!strcmp(tst_device->fs_type, "xfs"))
> > > +             expected_mask &= ~STATX_ATTR_COMPRESSED;
>
> ...
> > > +     .skip_filesystems = (const char *const[]) {
> > > +             "fuse",
> > > +             NULL
> > > +     },
> > >       .tags = (const struct tst_tag[]) {
> > >               {"linux-git", "93bc420ed41d"},
> > >               {"linux-git", "99652ea56a41"},
> > >               {"linux-git", "04a87e347282"},
> > >               {"linux-git", "5f955f26f3d4"},
> > > +             {"known-fail", "STATX_ATTR_COMPRESSED is not implemented on XFS (strict mode)"},
>
> I'm wondering if it has necessary to add the known-fail tags here
> since we have already done XFS filtering in setup().

Sorry, let me pull back my question, seems I neglected there is a 'strict mode'.