Re: [LTP] [PATCH] Fix unlink09 test

[Andrea Cervesato via ltp <ltp@lists.linux.it>]

Hi,

On 6/5/24 09:38, Petr Vorel wrote:
> 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.
statx04.c:86 is an example of what LTP tests do: they check if a feature 
is available or not in the specific environment.
In our case, we need to skip filesystems which are not supporting inode 
attributes, due to the usage of ioctl(FS_IOC_GETFLAGS). And this is what
unlink09 does in my patch.

The Avinesh approach only changes flags for open(), which is still ok, 
but not enough and probably SAFE_CREAT() is better in our case.
But if FS doesn't support inode attributes, test fails generating noise 
that requires debugging on the system as well, just to understand if 
there's a bug or not.

And this is (of course) something that we would like to avoid...

> 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

Andrea


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