* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-04 13:44 [LTP] [PATCH] Fix unlink09 test Andrea Cervesato
@ 2024-06-05 6:57 ` Petr Vorel
2024-06-05 7:38 ` Petr Vorel
2024-06-05 14:24 ` Konstantin Ryabitsev
2024-06-05 8:11 ` Cyril Hrubis
` (2 subsequent siblings)
3 siblings, 2 replies; 23+ messages in thread
From: Petr Vorel @ 2024-06-05 6:57 UTC (permalink / raw)
To: Andrea Cervesato; +Cc: Sebastian Chlad, ltp
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/
> */
> #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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-05 6:57 ` Petr Vorel
@ 2024-06-05 7:38 ` Petr Vorel
2024-06-05 7:55 ` Andrea Cervesato via ltp
` (2 more replies)
2024-06-05 14:24 ` Konstantin Ryabitsev
1 sibling, 3 replies; 23+ messages in thread
From: Petr Vorel @ 2024-06-05 7:38 UTC (permalink / raw)
To: Andrea Cervesato, Sebastian Chlad, ltp, Avinesh Kumar
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-05 7:38 ` Petr Vorel
@ 2024-06-05 7:55 ` Andrea Cervesato via ltp
2024-06-05 8:04 ` Cyril Hrubis
2024-06-05 12:02 ` Martin Doucha
2 siblings, 0 replies; 23+ messages in thread
From: Andrea Cervesato via ltp @ 2024-06-05 7:55 UTC (permalink / raw)
To: Petr Vorel, Andrea Cervesato, Sebastian Chlad, ltp, Avinesh Kumar
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-05 7:38 ` Petr Vorel
2024-06-05 7:55 ` Andrea Cervesato via ltp
@ 2024-06-05 8:04 ` Cyril Hrubis
2024-06-05 12:02 ` Martin Doucha
2 siblings, 0 replies; 23+ messages in thread
From: Cyril Hrubis @ 2024-06-05 8:04 UTC (permalink / raw)
To: Petr Vorel; +Cc: Sebastian Chlad, ltp
Hi!
> > 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).
I would say that exfat or vfat is not uncommon in android world so I
would expect that actually having LTP TMPDIR on exfat is not as unlikely
as you may think.
--
Cyril Hrubis
chrubis@suse.cz
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-05 7:38 ` Petr Vorel
2024-06-05 7:55 ` Andrea Cervesato via ltp
2024-06-05 8:04 ` Cyril Hrubis
@ 2024-06-05 12:02 ` Martin Doucha
2024-06-05 12:11 ` Petr Vorel
2 siblings, 1 reply; 23+ messages in thread
From: Martin Doucha @ 2024-06-05 12:02 UTC (permalink / raw)
To: Petr Vorel, Andrea Cervesato, Sebastian Chlad, ltp, Avinesh Kumar
On 05. 06. 24 9:38, Petr Vorel wrote:
> 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).
AFAICT, .all_filesystems and .needs_rofs are mutually exclusive at the
moment.
--
Martin Doucha mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-05 12:02 ` Martin Doucha
@ 2024-06-05 12:11 ` Petr Vorel
2024-06-05 12:27 ` Petr Vorel
0 siblings, 1 reply; 23+ messages in thread
From: Petr Vorel @ 2024-06-05 12:11 UTC (permalink / raw)
To: Martin Doucha; +Cc: Sebastian Chlad, ltp
Hi Martin,
> On 05. 06. 24 9:38, Petr Vorel wrote:
> > 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).
> AFAICT, .all_filesystems and .needs_rofs are mutually exclusive at the
> moment.
Good point, I completely overlook .needs_rofs. That makes things clearer.
ATM we have 3 other tests in syscalls/unlink. Not sure if all are filesystem
specific (I would say yes, but not sure), but at least unlink05.c (tests
deleting with unlink()) should be tested .all_filesystems. unlink07.c and
unlink08.c test errno.
Kind regards,
Petr
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-05 12:11 ` Petr Vorel
@ 2024-06-05 12:27 ` Petr Vorel
2024-06-05 12:34 ` Martin Doucha
0 siblings, 1 reply; 23+ messages in thread
From: Petr Vorel @ 2024-06-05 12:27 UTC (permalink / raw)
To: Martin Doucha, Andrea Cervesato, Sebastian Chlad, ltp,
Avinesh Kumar
Hi Martin,
> Hi Martin,
> > On 05. 06. 24 9:38, Petr Vorel wrote:
> > > 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).
> > AFAICT, .all_filesystems and .needs_rofs are mutually exclusive at the
> > moment.
Also I wonder if having functionality for .all_filesystems + .needs_rofs
wouldn't be useful. @Cyril @Martin WDYT?
Also, there is fallback when prepare_and_mount_ro_fs() fails to use block
device. Although, I don't see the read only mount flags added in this fallback,
IMHO MS_RDONLY is only in prepare_and_mount_ro_fs(), therefore the fallback is
read write and we even didn't get TWARN, just plain TINFO (it should be either
TWARN or TINFO with "WARNING:" at least).
Kind regards,
Petr
lib/tst_test.c
static void prepare_device(void)
{
...
if (tst_test->needs_rofs) {
/* If we failed to mount read-only tmpfs. Fallback to
* using a device with read-only filesystem.
*/
if (prepare_and_mount_ro_fs(NULL, tst_test->mntpoint, "tmpfs")) {
tst_res(TINFO, "Can't mount tmpfs read-only, "
"falling back to block device...");
tst_test->needs_device = 1;
tst_test->format_device = 1;
}
}
static int prepare_and_mount_ro_fs(const char *dev, const char *mntpoint,
const char *fs_type)
{
char buf[PATH_MAX];
if (mount(dev, mntpoint, fs_type, 0, NULL)) {
tst_res(TINFO | TERRNO, "Can't mount %s at %s (%s)",
dev, mntpoint, fs_type);
return 1;
}
mntpoint_mounted = 1;
snprintf(buf, sizeof(buf), "%s/dir/", mntpoint);
SAFE_MKDIR(buf, 0777);
snprintf(buf, sizeof(buf), "%s/file", mntpoint);
SAFE_FILE_PRINTF(buf, "file content");
SAFE_CHMOD(buf, 0777);
SAFE_MOUNT(dev, mntpoint, fs_type, MS_REMOUNT | MS_RDONLY, NULL);
return 0;
}
> Good point, I completely overlook .needs_rofs. That makes things clearer.
> ATM we have 3 other tests in syscalls/unlink. Not sure if all are filesystem
> specific (I would say yes, but not sure), but at least unlink05.c (tests
> deleting with unlink()) should be tested .all_filesystems. unlink07.c and
> unlink08.c test errno.
> Kind regards,
> Petr
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-05 12:27 ` Petr Vorel
@ 2024-06-05 12:34 ` Martin Doucha
2024-06-05 13:21 ` Petr Vorel
0 siblings, 1 reply; 23+ messages in thread
From: Martin Doucha @ 2024-06-05 12:34 UTC (permalink / raw)
To: Petr Vorel, Andrea Cervesato, Sebastian Chlad, ltp, Avinesh Kumar
On 05. 06. 24 14:27, Petr Vorel wrote:
> Hi Martin,
>
> Also I wonder if having functionality for .all_filesystems + .needs_rofs
> wouldn't be useful. @Cyril @Martin WDYT?
>
> Also, there is fallback when prepare_and_mount_ro_fs() fails to use block
> device. Although, I don't see the read only mount flags added in this fallback,
> IMHO MS_RDONLY is only in prepare_and_mount_ro_fs(), therefore the fallback is
> read write and we even didn't get TWARN, just plain TINFO (it should be either
> TWARN or TINFO with "WARNING:" at least).
It would be useful and prepare_device() implements everything that's
needed for it but there's a bug in do_setup() which creates a conflict
between the two attributes. The .all_filesystems attribute forces
.needs_device but a few lines below that is a check that .needs_rofs and
.needs_device are not set at the same time. This can be fixed.
--
Martin Doucha mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-05 12:34 ` Martin Doucha
@ 2024-06-05 13:21 ` Petr Vorel
2024-06-05 13:44 ` Cyril Hrubis
0 siblings, 1 reply; 23+ messages in thread
From: Petr Vorel @ 2024-06-05 13:21 UTC (permalink / raw)
To: Martin Doucha; +Cc: Sebastian Chlad, ltp
> On 05. 06. 24 14:27, Petr Vorel wrote:
> > Hi Martin,
> > Also I wonder if having functionality for .all_filesystems + .needs_rofs
> > wouldn't be useful. @Cyril @Martin WDYT?
> > Also, there is fallback when prepare_and_mount_ro_fs() fails to use block
> > device. Although, I don't see the read only mount flags added in this fallback,
> > IMHO MS_RDONLY is only in prepare_and_mount_ro_fs(), therefore the fallback is
> > read write and we even didn't get TWARN, just plain TINFO (it should be either
> > TWARN or TINFO with "WARNING:" at least).
> It would be useful and prepare_device() implements everything that's needed
> for it but there's a bug in do_setup() which creates a conflict between the
> two attributes. The .all_filesystems attribute forces .needs_device but a
> few lines below that is a check that .needs_rofs and .needs_device are not
> set at the same time. This can be fixed.
Thanks for info, I'll have look into it.
Anyway, you all agreed that splitting the test is needed either way.
And because of other thing Martin found (the third unfixed SAFE_OPEN() in
verify_unlink()) I'm setting this as changes requested.
Kind regards,
Petr
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-05 13:21 ` Petr Vorel
@ 2024-06-05 13:44 ` Cyril Hrubis
2024-06-05 13:53 ` Martin Doucha
2024-06-05 14:12 ` Petr Vorel
0 siblings, 2 replies; 23+ messages in thread
From: Cyril Hrubis @ 2024-06-05 13:44 UTC (permalink / raw)
To: Petr Vorel; +Cc: ltp, Sebastian Chlad
Hi!
> > It would be useful and prepare_device() implements everything that's needed
> > for it but there's a bug in do_setup() which creates a conflict between the
> > two attributes. The .all_filesystems attribute forces .needs_device but a
> > few lines below that is a check that .needs_rofs and .needs_device are not
> > set at the same time. This can be fixed.
>
> Thanks for info, I'll have look into it.
It may not be needed. The counter argument is that if you mix a read
only filesystem tests and all_filesystems in one test you are combining
two unrelated things that are probably better to be done in a separate
tests.
--
Cyril Hrubis
chrubis@suse.cz
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-05 13:44 ` Cyril Hrubis
@ 2024-06-05 13:53 ` Martin Doucha
2024-06-05 14:17 ` Petr Vorel
2024-06-05 14:12 ` Petr Vorel
1 sibling, 1 reply; 23+ messages in thread
From: Martin Doucha @ 2024-06-05 13:53 UTC (permalink / raw)
To: Cyril Hrubis, Petr Vorel; +Cc: Sebastian Chlad, ltp
On 05. 06. 24 15:44, Cyril Hrubis wrote:
> It may not be needed. The counter argument is that if you mix a read
> only filesystem tests and all_filesystems in one test you are combining
> two unrelated things that are probably better to be done in a separate
> tests.
I don't follow that argument. The expected result is that each available
filesystem will be mounted read-only. How is that unrelated? The default
.needs_rofs setup which mounts tmpfs as the single filesystem should be
skipped in this case.
--
Martin Doucha mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-05 13:53 ` Martin Doucha
@ 2024-06-05 14:17 ` Petr Vorel
0 siblings, 0 replies; 23+ messages in thread
From: Petr Vorel @ 2024-06-05 14:17 UTC (permalink / raw)
To: Martin Doucha; +Cc: ltp, Sebastian Chlad
> On 05. 06. 24 15:44, Cyril Hrubis wrote:
> > It may not be needed. The counter argument is that if you mix a read
> > only filesystem tests and all_filesystems in one test you are combining
> > two unrelated things that are probably better to be done in a separate
> > tests.
> I don't follow that argument. The expected result is that each available
> filesystem will be mounted read-only. How is that unrelated? The default
> .needs_rofs setup which mounts tmpfs as the single filesystem should be
> skipped in this case.
+1
Kind regards,
Petr
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-05 13:44 ` Cyril Hrubis
2024-06-05 13:53 ` Martin Doucha
@ 2024-06-05 14:12 ` Petr Vorel
1 sibling, 0 replies; 23+ messages in thread
From: Petr Vorel @ 2024-06-05 14:12 UTC (permalink / raw)
To: Cyril Hrubis; +Cc: ltp, Sebastian Chlad
> Hi!
> > > It would be useful and prepare_device() implements everything that's needed
> > > for it but there's a bug in do_setup() which creates a conflict between the
> > > two attributes. The .all_filesystems attribute forces .needs_device but a
> > > few lines below that is a check that .needs_rofs and .needs_device are not
> > > set at the same time. This can be fixed.
> > Thanks for info, I'll have look into it.
> It may not be needed. The counter argument is that if you mix a read
> only filesystem tests and all_filesystems in one test you are combining
> two unrelated things that are probably better to be done in a separate
> tests.
I thought that we have use case for testing all (or most) filesystems in read
only mode, instead testing just whatever is in the default /tmp. But sure,
I'll will not do that unless others think it's an useful feature.
So does it mean that all read only filesystems behave the same?
So far we have 14 tests with .needs_rofs
testcases/kernel/syscalls/access/access04.c
testcases/kernel/syscalls/acct/acct01.c
testcases/kernel/syscalls/chmod/chmod06.c
testcases/kernel/syscalls/chown/chown04.c
testcases/kernel/syscalls/creat/creat06.c
testcases/kernel/syscalls/fchmod/fchmod06.c
testcases/kernel/syscalls/fchown/fchown04.c
testcases/kernel/syscalls/link/link08.c
testcases/kernel/syscalls/mkdir/mkdir03.c
testcases/kernel/syscalls/mkdirat/mkdirat02.c
testcases/kernel/syscalls/rmdir/rmdir02.c
testcases/kernel/syscalls/unlink/unlink09.c
testcases/kernel/syscalls/utime/utime06.c
testcases/kernel/syscalls/utimes/utimes01.c
From these at least mkdir, rmdir, link, unlink have member in struct
inode_operations [1] (thus custom operation per filesystem, right?)
I suppose update_time might be related to utime* tests.
There are other members, but for these we IMHO don't test wit rofs.
Then, there is struct file_operations with open, read, write,
{compat,unlocked}_ioctl, llseek, ... operations which (at least open) are used
in various tests.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/fs.h#n2062
Kind regards,
Petr
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-05 6:57 ` Petr Vorel
2024-06-05 7:38 ` Petr Vorel
@ 2024-06-05 14:24 ` Konstantin Ryabitsev
2024-06-07 9:36 ` Petr Vorel
1 sibling, 1 reply; 23+ messages in thread
From: Konstantin Ryabitsev @ 2024-06-05 14:24 UTC (permalink / raw)
To: Petr Vorel; +Cc: Sebastian Chlad, ltp
On Wed, Jun 05, 2024 at 08:57:55AM GMT, Petr Vorel wrote:
> > 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).
This allows you to reliably find all revisions of the same series, for example
to do range-diffs between them.
-K
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-05 14:24 ` Konstantin Ryabitsev
@ 2024-06-07 9:36 ` Petr Vorel
0 siblings, 0 replies; 23+ messages in thread
From: Petr Vorel @ 2024-06-07 9:36 UTC (permalink / raw)
To: Konstantin Ryabitsev; +Cc: Sebastian Chlad, ltp
Hi Konstantin,
> On Wed, Jun 05, 2024 at 08:57:55AM GMT, Petr Vorel wrote:
> > > 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).
> This allows you to reliably find all revisions of the same series, for example
> to do range-diffs between them.
Welcome to LTP ML :). Thank you for giving a hint, it forced me to look up your
docs ([1] if anybody interested). BTW it'd be great if patchwork supported it
(somebody asked for something similar [2].
I noticed other things - having version in message-id and having somehow
"encrypted" email part before "at" (@), but keep part after "at" the same looks
interesting. I should find time to explore b4 :).
Kind regards,
Petr
[1] https://b4.docs.kernel.org/en/latest/contributor/prep.html#working-with-series-dependencies
[2] https://github.com/getpatchwork/patchwork/issues/583
> -K
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-04 13:44 [LTP] [PATCH] Fix unlink09 test Andrea Cervesato
2024-06-05 6:57 ` Petr Vorel
@ 2024-06-05 8:11 ` Cyril Hrubis
2024-06-05 10:16 ` Andrea Cervesato via ltp
2024-06-05 11:53 ` Martin Doucha
2024-06-05 12:05 ` Martin Doucha
2024-06-05 12:22 ` Martin Doucha
3 siblings, 2 replies; 23+ messages in thread
From: Cyril Hrubis @ 2024-06-05 8:11 UTC (permalink / raw)
To: Andrea Cervesato; +Cc: ltp
Hi!
> + 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");
> + }
I see one problem with this approach. If kernel accidentally removes a
support for immutable files for a certain filesystem this test will be
green. And the filesystems that miss this support are very unlikely to
gain it, e.g. will vfat get support for immutable files? That would be
an argument for explicit skiplist in the form of
tst_test->skip_filesystems.
--
Cyril Hrubis
chrubis@suse.cz
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-05 8:11 ` Cyril Hrubis
@ 2024-06-05 10:16 ` Andrea Cervesato via ltp
2024-06-05 11:30 ` Cyril Hrubis
2024-06-05 11:53 ` Martin Doucha
1 sibling, 1 reply; 23+ messages in thread
From: Andrea Cervesato via ltp @ 2024-06-05 10:16 UTC (permalink / raw)
To: Cyril Hrubis, Andrea Cervesato; +Cc: ltp
Hi,
On 6/5/24 10:11, Cyril Hrubis wrote:
> Hi!
>> + 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");
>> + }
> I see one problem with this approach. If kernel accidentally removes a
> support for immutable files for a certain filesystem this test will be
> green. And the filesystems that miss this support are very unlikely to
> gain it, e.g. will vfat get support for immutable files? That would be
> an argument for explicit skiplist in the form of
> tst_test->skip_filesystems.
>
That's a valid statement. For now I would like to fix the test first,
then we can fix this other problem with an another patch.
Andrea
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-05 10:16 ` Andrea Cervesato via ltp
@ 2024-06-05 11:30 ` Cyril Hrubis
2024-06-05 11:42 ` Andrea Cervesato via ltp
0 siblings, 1 reply; 23+ messages in thread
From: Cyril Hrubis @ 2024-06-05 11:30 UTC (permalink / raw)
To: Andrea Cervesato; +Cc: ltp
Hi!
> >> + 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");
> >> + }
> > I see one problem with this approach. If kernel accidentally removes a
> > support for immutable files for a certain filesystem this test will be
> > green. And the filesystems that miss this support are very unlikely to
> > gain it, e.g. will vfat get support for immutable files? That would be
> > an argument for explicit skiplist in the form of
> > tst_test->skip_filesystems.
> >
> That's a valid statement. For now I would like to fix the test first,
> then we can fix this other problem with an another patch.
As long as you promise to fix the test properly later on I agree with
adding the temporary workaround with a test for immutable support.
Also I suppose that it would make sense to enable the test for
all_filesystems but we would have to move the EROFS to a separate test
first.
--
Cyril Hrubis
chrubis@suse.cz
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-05 11:30 ` Cyril Hrubis
@ 2024-06-05 11:42 ` Andrea Cervesato via ltp
0 siblings, 0 replies; 23+ messages in thread
From: Andrea Cervesato via ltp @ 2024-06-05 11:42 UTC (permalink / raw)
To: Cyril Hrubis; +Cc: ltp
Hi,
On 6/5/24 13:30, Cyril Hrubis wrote:
> Hi!
>>>> + 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");
>>>> + }
>>> I see one problem with this approach. If kernel accidentally removes a
>>> support for immutable files for a certain filesystem this test will be
>>> green. And the filesystems that miss this support are very unlikely to
>>> gain it, e.g. will vfat get support for immutable files? That would be
>>> an argument for explicit skiplist in the form of
>>> tst_test->skip_filesystems.
>>>
>> That's a valid statement. For now I would like to fix the test first,
>> then we can fix this other problem with an another patch.
> As long as you promise to fix the test properly later on I agree with
> adding the temporary workaround with a test for immutable support.
The most important thing is that we can fix test so it won't show false
negative on certain FS, then we can
create patches to split into 2 tests (one for immutable and one for
EROFS), using .all_filesystems flag.
>
> Also I suppose that it would make sense to enable the test for
> all_filesystems but we would have to move the EROFS to a separate test
> first.
>
Andrea
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-05 8:11 ` Cyril Hrubis
2024-06-05 10:16 ` Andrea Cervesato via ltp
@ 2024-06-05 11:53 ` Martin Doucha
1 sibling, 0 replies; 23+ messages in thread
From: Martin Doucha @ 2024-06-05 11:53 UTC (permalink / raw)
To: Cyril Hrubis, Andrea Cervesato; +Cc: ltp
On 05. 06. 24 10:11, Cyril Hrubis wrote:
> Hi!
>> + 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");
>> + }
>
> I see one problem with this approach. If kernel accidentally removes a
> support for immutable files for a certain filesystem this test will be
> green. And the filesystems that miss this support are very unlikely to
> gain it, e.g. will vfat get support for immutable files? That would be
> an argument for explicit skiplist in the form of
> tst_test->skip_filesystems.
This condition doesn't check support of only the immutable flag, though.
It checks that the filesystem supports querying and setting inode flags
in general. We should add a new test for ioctl(FS_IOC_FLAGS) into the
ioctl test directory, this is not the appropriate place for it.
--
Martin Doucha mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-04 13:44 [LTP] [PATCH] Fix unlink09 test Andrea Cervesato
2024-06-05 6:57 ` Petr Vorel
2024-06-05 8:11 ` Cyril Hrubis
@ 2024-06-05 12:05 ` Martin Doucha
2024-06-05 12:22 ` Martin Doucha
3 siblings, 0 replies; 23+ messages in thread
From: Martin Doucha @ 2024-06-05 12:05 UTC (permalink / raw)
To: Andrea Cervesato, ltp
Hi,
I'm not sure whether removing the FS_APPEND_FL flag in cleanup() is
actually needed but it's a question for another patch.
Reviewed-by: Martin Doucha <mdoucha@suse.cz>
On 04. 06. 24 15:44, Andrea Cervesato wrote:
> 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.
> */
>
> #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) {
> + 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);
> + }
> }
>
> static void verify_unlink(unsigned int i)
>
> ---
> base-commit: 66517b89141fc455ed807f3b95e5260dcf9fb90f
> change-id: 20240604-unlink09-dc4802f872f9
>
> Best regards,
--
Martin Doucha mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [LTP] [PATCH] Fix unlink09 test
2024-06-04 13:44 [LTP] [PATCH] Fix unlink09 test Andrea Cervesato
` (2 preceding siblings ...)
2024-06-05 12:05 ` Martin Doucha
@ 2024-06-05 12:22 ` Martin Doucha
3 siblings, 0 replies; 23+ messages in thread
From: Martin Doucha @ 2024-06-05 12:22 UTC (permalink / raw)
To: Andrea Cervesato, ltp
Hi,
actually, there's one more invalid SAFE_OPEN() in verify_unlink() that
also needs to be fixed by this patch. It'd be best to move the
SAFE_CREATE()+ioctl() to a special function that'll take just a filename
and the inode flag that needs to be set. Please send a v2.
On 04. 06. 24 15:44, Andrea Cervesato wrote:
> 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.
> */
>
> #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) {
> + 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);
> + }
> }
>
> static void verify_unlink(unsigned int i)
>
> ---
> base-commit: 66517b89141fc455ed807f3b95e5260dcf9fb90f
> change-id: 20240604-unlink09-dc4802f872f9
>
> Best regards,
--
Martin Doucha mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 23+ messages in thread