Re: [LTP] [PATCH] Fix unlink09 test

[Petr Vorel <pvorel@suse.cz>]

Hi all,

[ Cc the test author more experienced maintainers ]

> Hi Andrea,

> > From: Andrea Cervesato <andrea.cervesato@suse.com>

> > This patch will fix unlink09 test by checking for filesystems which
> > are not supporting inode attributes.

> > Fixes: 2cf78f47a6 (unlink: Add error tests for EPERM and EROFS)
> > Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> > ---
> > This will fix the 2cf78f47a6 and resolve issues on filesystems
> > which are not supporting inode attributes.
> > ---
> >  testcases/kernel/syscalls/unlink/unlink09.c | 38 +++++++++++++++++++----------
> >  1 file changed, 25 insertions(+), 13 deletions(-)

> > diff --git a/testcases/kernel/syscalls/unlink/unlink09.c b/testcases/kernel/syscalls/unlink/unlink09.c
> > index cc4b4a07e..ed6f0adc3 100644
> > --- a/testcases/kernel/syscalls/unlink/unlink09.c
> > +++ b/testcases/kernel/syscalls/unlink/unlink09.c
> > @@ -11,6 +11,8 @@
> >   *
> >   * - EPERM when target file is marked as immutable or append-only
> >   * - EROFS when target file is on a read-only filesystem.
> > + *
> > + * Test won't be executed if inode attributes are not supported.
> While this is good, wouldn't be better to use approach from Avinesh to add
> O_RDWR (or whatever flag) to get test supported everywhere instead of skip?

> https://patchwork.ozlabs.org/project/ltp/patch/20240603124653.31967-1-akumar@suse.de/

OK, I got hint from Andrea, that this is inspired by statx04.c:86, which is
filtering vfat and exfat. Here the problem was on system which has tmpfs, but
just more strict default setup (likely umask). Therefore I still think that
approach from Avinesh is correct.

BTW shouldn't this test use .all_filesystems = 1 ? Or is it unlink() really VFS
only code? I see some specific functions in fs/*/, e.g. btrfs_unlink() or
ext4_unlink(), which are used for struct inode_operations unlink member.
Then, obviously also Andrea's check would be needed (otherwise is unlikely that
somebody would have TMPDIR on vfat or exfat).

Kind regards,
Petr

> >   */

> >  #include <sys/ioctl.h>
> > @@ -22,8 +24,8 @@
> >  #define DIR_EROFS "erofs"
> >  #define TEST_EROFS "erofs/test_erofs"

> > -static int fd_immutable;
> > -static int fd_append_only;
> > +static int fd_immutable = -1;
> > +static int fd_append_only = -1;

> >  static struct test_case_t {
> >  	char *filename;
> > @@ -43,12 +45,18 @@ static void setup(void)
> >  {
> >  	int attr;

> > -	fd_immutable = SAFE_OPEN(TEST_EPERM_IMMUTABLE, O_CREAT, 0600);
> > -	SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr);
> > +	fd_immutable = SAFE_CREAT(TEST_EPERM_IMMUTABLE, 0600);
> > +	TEST(ioctl(fd_immutable, FS_IOC_GETFLAGS, &attr));
> > +
> > +	if (TST_RET == -1 && TST_ERR == ENOTTY) {
> > +		SAFE_CLOSE(fd_immutable);
> > +		tst_brk(TCONF | TTERRNO, "Inode attributes not supported");
> > +	}
> > +
> >  	attr |= FS_IMMUTABLE_FL;
> >  	SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr);

> > -	fd_append_only = SAFE_OPEN(TEST_EPERM_APPEND_ONLY, O_CREAT, 0600);
> > +	fd_append_only = SAFE_CREAT(TEST_EPERM_APPEND_ONLY, 0600);
> >  	SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr);
> >  	attr |= FS_APPEND_FL;
> >  	SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr);
> > @@ -58,15 +66,19 @@ static void cleanup(void)
> >  {
> >  	int attr;

> > -	SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr);
> > -	attr &= ~FS_IMMUTABLE_FL;
> > -	SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr);
> > -	SAFE_CLOSE(fd_immutable);
> > +	if (fd_immutable != -1) {
> I thought we could return when fd_immutable == -1:

> 	if (fd_immutable != -1)
> 		return;

> But obviously this is can be also result of the first test (not only by the
> setup), thus you're correct.

> BTW verify_unlink() could be made simpler to return on if (TST_RET).

> > +		SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr);
> > +		attr &= ~FS_IMMUTABLE_FL;
> > +		SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr);
> > +		SAFE_CLOSE(fd_immutable);
> > +	}

> > -	SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr);
> > -	attr &= ~FS_APPEND_FL;
> > -	SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr);
> > -	SAFE_CLOSE(fd_append_only);
> > +	if (fd_append_only != -1) {
> > +		SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr);
> > +		attr &= ~FS_APPEND_FL;
> > +		SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr);
> > +		SAFE_CLOSE(fd_append_only);
> > +	}
> Am I mistaken that this check should have been added before?

> >  }

> >  static void verify_unlink(unsigned int i)

> > ---
> > base-commit: 66517b89141fc455ed807f3b95e5260dcf9fb90f
> I see useful b4 feature :).

> > change-id: 20240604-unlink09-dc4802f872f9
> But I wonder what is this for (what tool use change-id).

> Kind regards,
> Petr

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