[LTP] [PATCH] Fix unlink09 test
Petr Vorel
pvorel@suse.cz
Wed Jun 5 09:38:06 CEST 2024
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
More information about the ltp
mailing list