| Message ID | 20220114210034.16177-1-pvorel@suse.cz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] utime03.c: Fix filesystem name | expand |
Hi Petr, On Sat, Jan 15, 2022 at 5:00 AM Petr Vorel <pvorel@suse.cz> wrote: > > Use the same name as in tst_fs_type_name() to actually skip it. > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > "v9fs is a Unix implementation of the Plan 9 9p remote filesystem > protocol." [1] > > NOTE: filesystem is called v9fs, but mount parameter is "9p" > that's probably the reason why named "9p" and not "v9" in > tst_fs_type_name(). > > Kind regards, > Petr > > [1] https://www.kernel.org/doc/html/latest/filesystems/9p.html > > testcases/kernel/syscalls/utime/utime03.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/testcases/kernel/syscalls/utime/utime03.c b/testcases/kernel/syscalls/utime/utime03.c > index 48cc3bae77..823bc5f72a 100644 > --- a/testcases/kernel/syscalls/utime/utime03.c > +++ b/testcases/kernel/syscalls/utime/utime03.c > @@ -93,7 +93,7 @@ static struct tst_test test = { > .mntpoint = MNTPOINT, > .all_filesystems = 1, > .skip_filesystems = (const char *const[]) { > - "v9", > + "9p", I'm wondering does it really take effect with whatever "v9" or "9p"? Because the fs_type_whitelist[] does not include any of them. > "vfat", > "exfat", > NULL > -- > 2.34.1 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp >
> > +++ b/testcases/kernel/syscalls/utime/utime03.c > > @@ -93,7 +93,7 @@ static struct tst_test test = { > > .mntpoint = MNTPOINT, > > .all_filesystems = 1, > > .skip_filesystems = (const char *const[]) { > > - "v9", > > + "9p", > > I'm wondering does it really take effect with whatever "v9" or "9p"? > Because the fs_type_whitelist[] does not include any of them. Unless removing the .all_filesystems as well otherwise, it is impossible has a chance to test on 9p.
Hi Li, Cyril, > > > +++ b/testcases/kernel/syscalls/utime/utime03.c > > > @@ -93,7 +93,7 @@ static struct tst_test test = { > > > .mntpoint = MNTPOINT, > > > .all_filesystems = 1, > > > .skip_filesystems = (const char *const[]) { > > > - "v9", > > > + "9p", > > I'm wondering does it really take effect with whatever "v9" or "9p"? > > Because the fs_type_whitelist[] does not include any of them. +1. Do we want to add 9p to fs_type_whitelist[]? I suppose not, because (despite of the name containing "whitelist" it's the list of filesystems actually being tested - this is a bit confusing to me). > Unless removing the .all_filesystems as well otherwise, it is impossible > has a chance to test on 9p. Yep. I forgot that .skip_filesystems works also on single fs. So correct entry in .skip_filesystems is kind of documentation in case of .all_filesystems being removed. I guess we should just remove the entry. Kind regards, Petr
On Mon, Jan 17, 2022 at 5:04 PM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Li, Cyril, > > > > > +++ b/testcases/kernel/syscalls/utime/utime03.c > > > > @@ -93,7 +93,7 @@ static struct tst_test test = { > > > > .mntpoint = MNTPOINT, > > > > .all_filesystems = 1, > > > > .skip_filesystems = (const char *const[]) { > > > > - "v9", > > > > + "9p", > > > > I'm wondering does it really take effect with whatever "v9" or "9p"? > > > Because the fs_type_whitelist[] does not include any of them. > +1. Do we want to add 9p to fs_type_whitelist[]? I suppose not, because (despite I agree with you, as 9p is not a widely used filesystem for Linux distribution. > of the name containing "whitelist" it's the list of filesystems actually being > tested - this is a bit confusing to me). Yes, it is actually the filesystem list that LTP will be tested on. or maybe rename it to better understand. > > > Unless removing the .all_filesystems as well otherwise, it is impossible > > has a chance to test on 9p. > Yep. I forgot that .skip_filesystems works also on single fs. > So correct entry in .skip_filesystems is kind of documentation in case of > .all_filesystems being removed. I guess we should just remove the entry. Sorry, what does that 'remove the entry' mean? I didn't catch your point here. -- Regards, Li Wang
Hi all, > On Mon, Jan 17, 2022 at 5:04 PM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Li, Cyril, > > > > > +++ b/testcases/kernel/syscalls/utime/utime03.c > > > > > @@ -93,7 +93,7 @@ static struct tst_test test = { > > > > > .mntpoint = MNTPOINT, > > > > > .all_filesystems = 1, > > > > > .skip_filesystems = (const char *const[]) { > > > > > - "v9", > > > > > + "9p", > > > > I'm wondering does it really take effect with whatever "v9" or "9p"? > > > > Because the fs_type_whitelist[] does not include any of them. > > +1. Do we want to add 9p to fs_type_whitelist[]? I suppose not, because (despite > I agree with you, as 9p is not a widely used filesystem for Linux distribution. > > of the name containing "whitelist" it's the list of filesystems actually being > > tested - this is a bit confusing to me). > Yes, it is actually the filesystem list that LTP will be tested on. > or maybe rename it to better understand. +1. I'll try to send patch after release. > > > Unless removing the .all_filesystems as well otherwise, it is impossible > > > has a chance to test on 9p. > > Yep. I forgot that .skip_filesystems works also on single fs. > > So correct entry in .skip_filesystems is kind of documentation in case of > > .all_filesystems being removed. I guess we should just remove the entry. > Sorry, what does that 'remove the entry' mean? I didn't catch your point here. As you pointed out it does not have any effect now to whitelist 9p. It's kind of documentation. Maybe instead of fixing the line we should remove it and put a comment above. /* NOTE: also does not work on 9p */ .skip_filesystems = (const char *const[]) { "vfat", "exfat", NULL } Obviously the best would be to recheck if the limitation still exists, because whole problem is 10 years old: it was added bc5da68248cc963e17862b7a0c556409c29c763e in 2011 by Cyril: The functional tests for utime checks if utime updates the modification and access time to current time, however V9FS, similar to NFS, by default uses the server's localtime if client doesnt specify a new time. The current implentation does not run the test if the underlying filesystem is NFS. A similar check for V9FS is also required, hence this patch. Note later was found that NFS was ok on 2.6.18: d623e2c7fe ("splice01/tee01/utime: add kernel version check for NFS test") and remove during Martin's rewrite in ec3c3e5462. Kind regards, Petr
On Tue, Jan 18, 2022 at 5:01 PM Petr Vorel <pvorel@suse.cz> wrote: > > Hi all, > > > On Mon, Jan 17, 2022 at 5:04 PM Petr Vorel <pvorel@suse.cz> wrote: > > > > Hi Li, Cyril, > > > > > > > +++ b/testcases/kernel/syscalls/utime/utime03.c > > > > > > @@ -93,7 +93,7 @@ static struct tst_test test = { > > > > > > .mntpoint = MNTPOINT, > > > > > > .all_filesystems = 1, > > > > > > .skip_filesystems = (const char *const[]) { > > > > > > - "v9", > > > > > > + "9p", > > > > > > I'm wondering does it really take effect with whatever "v9" or "9p"? > > > > > Because the fs_type_whitelist[] does not include any of them. > > > +1. Do we want to add 9p to fs_type_whitelist[]? I suppose not, because (despite > > > I agree with you, as 9p is not a widely used filesystem for Linux distribution. > > > > of the name containing "whitelist" it's the list of filesystems actually being > > > tested - this is a bit confusing to me). > > > Yes, it is actually the filesystem list that LTP will be tested on. > > or maybe rename it to better understand. > +1. I'll try to send patch after release. > > > > > > Unless removing the .all_filesystems as well otherwise, it is impossible > > > > has a chance to test on 9p. > > > Yep. I forgot that .skip_filesystems works also on single fs. > > > So correct entry in .skip_filesystems is kind of documentation in case of > > > .all_filesystems being removed. I guess we should just remove the entry. > > > Sorry, what does that 'remove the entry' mean? I didn't catch your point here. > As you pointed out it does not have any effect now to whitelist 9p. > It's kind of documentation. Maybe instead of fixing the line we should remove it > and put a comment above. Ah sure, I'm fine with that quick fix (before the new release) unless Cyril has additional comments. > > /* NOTE: also does not work on 9p */ > .skip_filesystems = (const char *const[]) { > "vfat", > "exfat", > NULL > } > > Obviously the best would be to recheck if the limitation still exists, > because whole problem is 10 years old: it was added > bc5da68248cc963e17862b7a0c556409c29c763e in 2011 by Cyril: Agree, we can leave this more time to rethink that. > > The functional tests for utime checks if utime updates the > modification and access time to current time, however V9FS, > similar to NFS, by default uses the server's localtime if > client doesnt specify a new time. The current implentation > does not run the test if the underlying filesystem is NFS. > A similar check for V9FS is also required, hence this patch. > > Note later was found that NFS was ok on 2.6.18: > d623e2c7fe ("splice01/tee01/utime: add kernel version check for NFS test") > and remove during Martin's rewrite in ec3c3e5462. > > Kind regards, > Petr >
Hi! > > > Sorry, what does that 'remove the entry' mean? I didn't catch your point here. > > As you pointed out it does not have any effect now to whitelist 9p. > > It's kind of documentation. Maybe instead of fixing the line we should remove it > > and put a comment above. > > Ah sure, I'm fine with that quick fix (before the new release) > unless Cyril has additional comments. I would go for removing the entry as well.
diff --git a/testcases/kernel/syscalls/utime/utime03.c b/testcases/kernel/syscalls/utime/utime03.c index 48cc3bae77..823bc5f72a 100644 --- a/testcases/kernel/syscalls/utime/utime03.c +++ b/testcases/kernel/syscalls/utime/utime03.c @@ -93,7 +93,7 @@ static struct tst_test test = { .mntpoint = MNTPOINT, .all_filesystems = 1, .skip_filesystems = (const char *const[]) { - "v9", + "9p", "vfat", "exfat", NULL
Use the same name as in tst_fs_type_name() to actually skip it. Signed-off-by: Petr Vorel <pvorel@suse.cz> --- "v9fs is a Unix implementation of the Plan 9 9p remote filesystem protocol." [1] NOTE: filesystem is called v9fs, but mount parameter is "9p" that's probably the reason why named "9p" and not "v9" in tst_fs_type_name(). Kind regards, Petr [1] https://www.kernel.org/doc/html/latest/filesystems/9p.html testcases/kernel/syscalls/utime/utime03.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)