[LTP] [PATCH] tst_supported_fs: Implement checking paths against skiplist

[LTP] [PATCH] tst_supported_fs: Implement checking paths against skiplist

[Martin Doucha]

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---

Implement tst_supported_fs feature suggested by pvorel in his patch:
tst_test.sh: Fix filesystem support detection

Although the tst_fs_type_name() functions could use some improvements,
e.g. ext4 must be specified in skiplist as "ext2/ext3/ext4" to get properly
skipped and vfat is missing from the list of known filesystems.

 testcases/lib/tst_supported_fs.c | 49 ++++++++++++++++++++++++++------
 1 file changed, 41 insertions(+), 8 deletions(-)

diff --git a/testcases/lib/tst_supported_fs.c b/testcases/lib/tst_supported_fs.c
index 70d4d38c7..5873d0ba1 100644
--- a/testcases/lib/tst_supported_fs.c
+++ b/testcases/lib/tst_supported_fs.c
@@ -32,9 +32,13 @@ static void usage(void)
 	fprintf(stderr, "tst_supported_fs -s skip_list fs_type\n");
 	fprintf(stderr, "   if fs_type is in skip_list, return 1 otherwise return 0\n\n");
 
+	fprintf(stderr, "tst_supported_fs -s skip_list -d path\n");
+	fprintf(stderr, "   if filesystem mounted on path is in skip_list, return 1 otherwise return 0\n\n");
+
 	fprintf(stderr, "fs_type - a specified filesystem type\n");
 	fprintf(stderr, "skip_list - filesystems to skip, delimiter: '%c'\n",
 			SKIP_DELIMITER);
+	fprintf(stderr, "path - any valid file or directory\n");
 }
 
 static char **parse_skiplist(char *fs)
@@ -62,10 +66,11 @@ static char **parse_skiplist(char *fs)
 int main(int argc, char *argv[])
 {
 	const char *const *filesystems;
+	const char *fsname = NULL;
 	int i, ret;
 	char **skiplist = NULL;
 
-	while ((ret = getopt(argc, argv, "hs:"))) {
+	while ((ret = getopt(argc, argv, "hs:d:"))) {
 		if (ret < 0)
 			break;
 
@@ -83,9 +88,26 @@ int main(int argc, char *argv[])
 			if (!skiplist)
 				return 1;
 			break;
+
+		case 'd':
+			if (fsname) {
+				fprintf(stderr,
+					"Can't specify multiple paths\n");
+				usage();
+				return 2;
+			}
+
+			fsname = tst_fs_type_name(tst_fs_type(optarg));
+			break;
 		}
 	}
 
+	if (fsname && !skiplist) {
+		fprintf(stderr, "Parameter -d requires skiplist\n");
+		usage();
+		return 2;
+	}
+
 	if (argc - optind > 1) {
 		fprintf(stderr, "Can't specify multiple fs_type\n");
 		usage();
@@ -94,22 +116,33 @@ int main(int argc, char *argv[])
 
 	/* fs_type */
 	if (optind < argc) {
-		if (argv[optind][0] == '\0')
+		if (fsname) {
+			fprintf(stderr, "Can't specify fs_type and -d together\n");
+			usage();
+			return 2;
+
+		}
+
+		fsname = argv[optind];
+	}
+
+	if (fsname) {
+		if (fsname[0] == '\0')
 			tst_brk(TCONF, "fs_type is empty");
 
 		if (skiplist) {
-			if (tst_fs_in_skiplist(argv[optind], (const char * const*)skiplist))
-				tst_brk(TCONF, "%s is skipped", argv[optind]);
+			if (tst_fs_in_skiplist(fsname, (const char * const*)skiplist))
+				tst_brk(TCONF, "%s is skipped", fsname);
 			else
-				tst_res(TINFO, "%s is not skipped", argv[optind]);
+				tst_res(TINFO, "%s is not skipped", fsname);
 
 			return 0;
 		}
 
-		if (tst_fs_is_supported(argv[optind]) == TST_FS_UNSUPPORTED)
-			tst_brk(TCONF, "%s is not supported", argv[optind]);
+		if (tst_fs_is_supported(fsname) == TST_FS_UNSUPPORTED)
+			tst_brk(TCONF, "%s is not supported", fsname);
 		else
-			tst_res(TINFO, "%s is supported", argv[optind]);
+			tst_res(TINFO, "%s is supported", fsname);
 
 		return 0;
 	}
-- 
2.37.3


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

Re: [LTP] [PATCH] tst_supported_fs: Implement checking paths against skiplist

[Li Wang]

[-- Attachment #1.1: Type: text/plain, Size: 5460 bytes --]

On Wed, Sep 21, 2022 at 11:50 PM Martin Doucha <mdoucha@suse.cz> wrote:

> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> ---
>
> Implement tst_supported_fs feature suggested by pvorel in his patch:
> tst_test.sh: Fix filesystem support detection
>
> Although the tst_fs_type_name() functions could use some improvements,
> e.g. ext4 must be specified in skiplist as "ext2/ext3/ext4" to get properly
>

Yes, that's true, we need to make it keep consistent with two
skipping ways. Otherwise below test output "ext2/ext3/ext4"
looks like a bit mess to remember.

Better going with a single FS for matching (i.e. "ext4" for both fs_type
and '-d path').
But we can solve this in a separate patch later.

$ df -T . | tail -1 | awk '{print $2}'
ext4

$ ./tst_supported_fs -s "ext4" ext4
tst_supported_fs.c:135: TCONF: ext4 is skipped
$ echo $?
32

$ ./tst_supported_fs -s "ext4" -d .
tst_supported_fs.c:137: TINFO: ext2/ext3/ext4 is not skipped
$ echo $?
0

$ ./tst_supported_fs -s "ext2/ext3/ext4" -d .
tst_supported_fs.c:135: TCONF: ext2/ext3/ext4 is skipped
$ echo $?
32



> skipped and vfat is missing from the list of known filesystems.
>
>  testcases/lib/tst_supported_fs.c | 49 ++++++++++++++++++++++++++------
>  1 file changed, 41 insertions(+), 8 deletions(-)
>
> diff --git a/testcases/lib/tst_supported_fs.c
> b/testcases/lib/tst_supported_fs.c
> index 70d4d38c7..5873d0ba1 100644
> --- a/testcases/lib/tst_supported_fs.c
> +++ b/testcases/lib/tst_supported_fs.c
> @@ -32,9 +32,13 @@ static void usage(void)
>         fprintf(stderr, "tst_supported_fs -s skip_list fs_type\n");
>         fprintf(stderr, "   if fs_type is in skip_list, return 1 otherwise
> return 0\n\n");
>
> +       fprintf(stderr, "tst_supported_fs -s skip_list -d path\n");
> +       fprintf(stderr, "   if filesystem mounted on path is in skip_list,
> return 1 otherwise return 0\n\n");

+
>         fprintf(stderr, "fs_type - a specified filesystem type\n");
>         fprintf(stderr, "skip_list - filesystems to skip, delimiter:
> '%c'\n",
>                         SKIP_DELIMITER);
> +       fprintf(stderr, "path - any valid file or directory\n");
>  }
>
>  static char **parse_skiplist(char *fs)
> @@ -62,10 +66,11 @@ static char **parse_skiplist(char *fs)
>  int main(int argc, char *argv[])
>  {
>         const char *const *filesystems;
> +       const char *fsname = NULL;
>         int i, ret;
>         char **skiplist = NULL;
>
> -       while ((ret = getopt(argc, argv, "hs:"))) {
> +       while ((ret = getopt(argc, argv, "hs:d:"))) {
>                 if (ret < 0)
>                         break;
>
> @@ -83,9 +88,26 @@ int main(int argc, char *argv[])
>                         if (!skiplist)
>                                 return 1;
>                         break;
> +
> +               case 'd':
> +                       if (fsname) {
> +                               fprintf(stderr,
> +                                       "Can't specify multiple paths\n");
> +                               usage();
> +                               return 2;
> +                       }
> +
> +                       fsname = tst_fs_type_name(tst_fs_type(optarg));
> +                       break;
>                 }
>         }
>
> +       if (fsname && !skiplist) {
> +               fprintf(stderr, "Parameter -d requires skiplist\n");
> +               usage();
> +               return 2;
> +       }
> +
>         if (argc - optind > 1) {
>                 fprintf(stderr, "Can't specify multiple fs_type\n");
>                 usage();
> @@ -94,22 +116,33 @@ int main(int argc, char *argv[])
>
>         /* fs_type */
>         if (optind < argc) {
> -               if (argv[optind][0] == '\0')
> +               if (fsname) {
> +                       fprintf(stderr, "Can't specify fs_type and -d
> together\n");
> +                       usage();
> +                       return 2;
> +
> +               }
> +
> +               fsname = argv[optind];
> +       }
> +
> +       if (fsname) {
> +               if (fsname[0] == '\0')
>                         tst_brk(TCONF, "fs_type is empty");
>
>                 if (skiplist) {
> -                       if (tst_fs_in_skiplist(argv[optind], (const char *
> const*)skiplist))
> -                               tst_brk(TCONF, "%s is skipped",
> argv[optind]);
> +                       if (tst_fs_in_skiplist(fsname, (const char *
> const*)skiplist))
> +                               tst_brk(TCONF, "%s is skipped", fsname);
>

TCONF does not means return 1, we might need explicitly 'return 1' here.


                        else
> -                               tst_res(TINFO, "%s is not skipped",
> argv[optind]);
> +                               tst_res(TINFO, "%s is not skipped",
> fsname);
>
>                         return 0;
>                 }
>
> -               if (tst_fs_is_supported(argv[optind]) ==
> TST_FS_UNSUPPORTED)
> -                       tst_brk(TCONF, "%s is not supported",
> argv[optind]);
> +               if (tst_fs_is_supported(fsname) == TST_FS_UNSUPPORTED)
> +                       tst_brk(TCONF, "%s is not supported", fsname);
>

Same here.



>                 else
> -                       tst_res(TINFO, "%s is supported", argv[optind]);
> +                       tst_res(TINFO, "%s is supported", fsname);
>
>                 return 0;
>         }
> --
> 2.37.3
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
>

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 9370 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

Re: [LTP] [PATCH] tst_supported_fs: Implement checking paths against skiplist

[Martin Doucha]

On 22. 09. 22 6:16, Li Wang wrote:
>                      if (skiplist) {
>     -                       if (tst_fs_in_skiplist(argv[optind], (const char * const*)skiplist))
>     -                               tst_brk(TCONF, "%s is skipped", argv[optind]);
>     +                       if (tst_fs_in_skiplist(fsname, (const char * const*)skiplist))
>     +                               tst_brk(TCONF, "%s is skipped", fsname);
> 
> 
> TCONF does not means return 1, we might need explicitly 'return 1' here.

Yes, but that's a problem which already existed before my patch and 
affects the other single-FS checks as well. The fix should go into a 
separate patch after release.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
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

Re: [LTP] [PATCH] tst_supported_fs: Implement checking paths against skiplist

[Petr Vorel]

> On 22. 09. 22 6:16, Li Wang wrote:
> >                      if (skiplist) {
> >     -                       if (tst_fs_in_skiplist(argv[optind], (const char * const*)skiplist))
> >     -                               tst_brk(TCONF, "%s is skipped", argv[optind]);
> >     +                       if (tst_fs_in_skiplist(fsname, (const char * const*)skiplist))
> >     +                               tst_brk(TCONF, "%s is skipped", fsname);


> > TCONF does not means return 1, we might need explicitly 'return 1' here.

> Yes, but that's a problem which already existed before my patch and affects
> the other single-FS checks as well. The fix should go into a separate patch
> after release.

Yes, that's not related. I changed it in eb47b4497 ("tst_supported_fs: Support
skip list when query single fs") as with 32 one can detect it's TCONF.
As long as 0 is success, non-zero for failure.
But I forget to update doc.

Thus I'll wait for feedback from others whether we should get back to 1 as error
(thus use TINFO) or update doc to mention to exit 32 (or more generally non-zero
exit).
very nit: I'd use exit instead of return in docs.

Kind regards,
Petr

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

Re: [LTP] [PATCH] tst_supported_fs: Implement checking paths against skiplist

[Li Wang]

[-- Attachment #1.1: Type: text/plain, Size: 1559 bytes --]

On Thu, Sep 22, 2022 at 4:44 PM Petr Vorel <pvorel@suse.cz> wrote:

> > On 22. 09. 22 6:16, Li Wang wrote:
> > >                      if (skiplist) {
> > >     -                       if (tst_fs_in_skiplist(argv[optind],
> (const char * const*)skiplist))
> > >     -                               tst_brk(TCONF, "%s is skipped",
> argv[optind]);
> > >     +                       if (tst_fs_in_skiplist(fsname, (const char
> * const*)skiplist))
> > >     +                               tst_brk(TCONF, "%s is skipped",
> fsname);
>
>
> > > TCONF does not means return 1, we might need explicitly 'return 1'
> here.
>
> > Yes, but that's a problem which already existed before my patch and
> affects
> > the other single-FS checks as well. The fix should go into a separate
> patch
> > after release.
>
> Yes, that's not related. I changed it in eb47b4497 ("tst_supported_fs:
> Support
> skip list when query single fs") as with 32 one can detect it's TCONF.
> As long as 0 is success, non-zero for failure.
>

Looks like there is no difference to use whatever 32 or 1, as long as
it's a non-zero, it will work well in the shell.

Ok, I agree to update the doc (also usage() should be updated) later.

Reviewed-by: Li Wang <liwang@redhat.com>



> But I forget to update doc.
>
> Thus I'll wait for feedback from others whether we should get back to 1 as
> error
> (thus use TINFO) or update doc to mention to exit 32 (or more generally
> non-zero
> exit).
> very nit: I'd use exit instead of return in docs.
>
> Kind regards,
> Petr
>
>

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 2822 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

Re: [LTP] [PATCH] tst_supported_fs: Implement checking paths against skiplist

[Petr Vorel]

...
> > Yes, that's not related. I changed it in eb47b4497 ("tst_supported_fs:
> > Support
> > skip list when query single fs") as with 32 one can detect it's TCONF.
> > As long as 0 is success, non-zero for failure.


> Looks like there is no difference to use whatever 32 or 1, as long as
> it's a non-zero, it will work well in the shell.

> Ok, I agree to update the doc (also usage() should be updated) later.
+1, I actually meant usage() in testcases/lib/tst_supported_fs.c.

Petr

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

Re: [LTP] [PATCH] tst_supported_fs: Implement checking paths against skiplist

[Petr Vorel]

Hi all,

> > Although the tst_fs_type_name() functions could use some improvements,
> > e.g. ext4 must be specified in skiplist as "ext2/ext3/ext4" to get properly

> Yes, that's true, we need to make it keep consistent with two
> skipping ways. Otherwise below test output "ext2/ext3/ext4"
> looks like a bit mess to remember.

> Better going with a single FS for matching (i.e. "ext4" for both fs_type
> and '-d path').
> But we can solve this in a separate patch later.

> $ df -T . | tail -1 | awk '{print $2}'
> ext4

> $ ./tst_supported_fs -s "ext4" ext4
> tst_supported_fs.c:135: TCONF: ext4 is skipped
> $ echo $?
> 32

> $ ./tst_supported_fs -s "ext4" -d .
> tst_supported_fs.c:137: TINFO: ext2/ext3/ext4 is not skipped
> $ echo $?
> 0

> $ ./tst_supported_fs -s "ext2/ext3/ext4" -d .
> tst_supported_fs.c:135: TCONF: ext2/ext3/ext4 is skipped
> $ echo $?
> 32

The same problem is for .skip_filesystems on tests which does not use
.all_filesystems. We only haven't noticed, because there was no reason to skip
ext[234] so far. I'm looking into this.

BTW .skip_filesystems without .all_filesystems have other problems, e.g. using
filesystems which aren't in fs_type_whitelist[] array (e.g. ramfs, nfs).
That'd be nice to fix after the release.

Kind regards,
Petr

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

Re: [LTP] [PATCH] tst_supported_fs: Implement checking paths against skiplist

[Li Wang]

[-- Attachment #1.1: Type: text/plain, Size: 1644 bytes --]

On Thu, Sep 22, 2022 at 5:03 PM Petr Vorel <pvorel@suse.cz> wrote:

> Hi all,
>
> > > Although the tst_fs_type_name() functions could use some improvements,
> > > e.g. ext4 must be specified in skiplist as "ext2/ext3/ext4" to get
> properly
>
> > Yes, that's true, we need to make it keep consistent with two
> > skipping ways. Otherwise below test output "ext2/ext3/ext4"
> > looks like a bit mess to remember.
>
> > Better going with a single FS for matching (i.e. "ext4" for both fs_type
> > and '-d path').
> > But we can solve this in a separate patch later.
>
> > $ df -T . | tail -1 | awk '{print $2}'
> > ext4
>
> > $ ./tst_supported_fs -s "ext4" ext4
> > tst_supported_fs.c:135: TCONF: ext4 is skipped
> > $ echo $?
> > 32
>
> > $ ./tst_supported_fs -s "ext4" -d .
> > tst_supported_fs.c:137: TINFO: ext2/ext3/ext4 is not skipped
> > $ echo $?
> > 0
>
> > $ ./tst_supported_fs -s "ext2/ext3/ext4" -d .
> > tst_supported_fs.c:135: TCONF: ext2/ext3/ext4 is skipped
> > $ echo $?
> > 32
>
> The same problem is for .skip_filesystems on tests which does not use
> .all_filesystems. We only haven't noticed, because there was no reason to
> skip
> ext[234] so far. I'm looking into this.
>

This seems a bit tricky to distinguish EXT2,3,4, from what I know,
they use the same magic number. It will be difficult to get the
FS block and extract smaller granularity of feature without
using fs helper tools.



>
> BTW .skip_filesystems without .all_filesystems have other problems, e.g.
> using
> filesystems which aren't in fs_type_whitelist[] array (e.g. ramfs, nfs).
> That'd be nice to fix after the release.
>

+1


-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 2976 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

Re: [LTP] [PATCH] tst_supported_fs: Implement checking paths against skiplist

[Petr Vorel]

> On Thu, Sep 22, 2022 at 5:03 PM Petr Vorel <pvorel@suse.cz> wrote:

> > Hi all,

> > > > Although the tst_fs_type_name() functions could use some improvements,
> > > > e.g. ext4 must be specified in skiplist as "ext2/ext3/ext4" to get
> > properly

> > > Yes, that's true, we need to make it keep consistent with two
> > > skipping ways. Otherwise below test output "ext2/ext3/ext4"
> > > looks like a bit mess to remember.

> > > Better going with a single FS for matching (i.e. "ext4" for both fs_type
> > > and '-d path').
> > > But we can solve this in a separate patch later.

> > > $ df -T . | tail -1 | awk '{print $2}'
> > > ext4

> > > $ ./tst_supported_fs -s "ext4" ext4
> > > tst_supported_fs.c:135: TCONF: ext4 is skipped
> > > $ echo $?
> > > 32

> > > $ ./tst_supported_fs -s "ext4" -d .
> > > tst_supported_fs.c:137: TINFO: ext2/ext3/ext4 is not skipped
> > > $ echo $?
> > > 0

> > > $ ./tst_supported_fs -s "ext2/ext3/ext4" -d .
> > > tst_supported_fs.c:135: TCONF: ext2/ext3/ext4 is skipped
> > > $ echo $?
> > > 32

> > The same problem is for .skip_filesystems on tests which does not use
> > .all_filesystems. We only haven't noticed, because there was no reason to
> > skip
> > ext[234] so far. I'm looking into this.


> This seems a bit tricky to distinguish EXT2,3,4, from what I know,
> they use the same magic number. It will be difficult to get the
> FS block and extract smaller granularity of feature without
> using fs helper tools.

Yep, magic is the same: 0xef53 (not count the old ext2: 0xef51).
I thought that I'd map ext[235] to "ext2/ext3/ext4".
But maybe it's not a good idea.

OTOH df (coreutils) manages to distinguish them, stat can't.
I looked into df sources, it looks read_file_system_list() from gnulib which
it uses, reads /proc/self/mountinfo. Wouldn't be better to follow this approach
in tst_fs_type_() instead of using statfs()? (using st_dev - major:minor from
stat to match). I guess we always read mounted filesystem, thus it'd be a
drop-in replacement, we'd not need tst_fs_type_name() any more.

Kind regards,
Petr


> > BTW .skip_filesystems without .all_filesystems have other problems, e.g.
> > using
> > filesystems which aren't in fs_type_whitelist[] array (e.g. ramfs, nfs).
> > That'd be nice to fix after the release.


> +1

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

Re: [LTP] [PATCH] tst_supported_fs: Implement checking paths against skiplist

[Cyril Hrubis]

Hi!
> > This seems a bit tricky to distinguish EXT2,3,4, from what I know,
> > they use the same magic number. It will be difficult to get the
> > FS block and extract smaller granularity of feature without
> > using fs helper tools.
> 
> Yep, magic is the same: 0xef53 (not count the old ext2: 0xef51).
> I thought that I'd map ext[235] to "ext2/ext3/ext4".
> But maybe it's not a good idea.
> 
> OTOH df (coreutils) manages to distinguish them, stat can't.
> I looked into df sources, it looks read_file_system_list() from gnulib which
> it uses, reads /proc/self/mountinfo. Wouldn't be better to follow this approach
> in tst_fs_type_() instead of using statfs()? (using st_dev - major:minor from
> stat to match). I guess we always read mounted filesystem, thus it'd be a
> drop-in replacement, we'd not need tst_fs_type_name() any more.

The real problem is that the line between ext2, ext3 and ext4 is really
blurry to an extend where it's hard to say which filesystem are you
working with. For example while ext2 kernel driver still exists and is
in use on embedded hardware modern distributions use single ext driver
to handle all three filesystems. Also I think that it was possible to
mount ext3 with ext2 driver at some point which disabled the journaling.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH] tst_supported_fs: Implement checking paths against skiplist

[Martin Doucha]

On 22. 09. 22 12:55, Cyril Hrubis wrote:
> The real problem is that the line between ext2, ext3 and ext4 is really
> blurry to an extend where it's hard to say which filesystem are you
> working with. For example while ext2 kernel driver still exists and is
> in use on embedded hardware modern distributions use single ext driver
> to handle all three filesystems. Also I think that it was possible to
> mount ext3 with ext2 driver at some point which disabled the journaling.

Still, I'd argue that blindly assuming any FS with ext2/3/4 magic number 
is always mounted as ext4 is a better solution than returning a string 
that never matches skiplists.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
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

Re: [LTP] [PATCH] tst_supported_fs: Implement checking paths against skiplist

[Petr Vorel]

> On 22. 09. 22 12:55, Cyril Hrubis wrote:
> > The real problem is that the line between ext2, ext3 and ext4 is really
> > blurry to an extend where it's hard to say which filesystem are you
> > working with. For example while ext2 kernel driver still exists and is
> > in use on embedded hardware modern distributions use single ext driver
> > to handle all three filesystems. Also I think that it was possible to
> > mount ext3 with ext2 driver at some point which disabled the journaling.

Should I understand this that you don't trust /proc/self/mountinfo output?
(show_mountinfo() in fs/proc_namespace.c in kernel git)?

Petr

> Still, I'd argue that blindly assuming any FS with ext2/3/4 magic number is
> always mounted as ext4 is a better solution than returning a string that
> never matches skiplists.


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

Re: [LTP] [PATCH] tst_supported_fs: Implement checking paths against skiplist

[Cyril Hrubis]

Hi!
> Should I understand this that you don't trust /proc/self/mountinfo output?
> (show_mountinfo() in fs/proc_namespace.c in kernel git)?

Not much as I wouldn't trust it, it's just that the ext2/ext3/ext4 names
are just short names for a subset of additional functionality.

Consider this:

$ dd if=/dev/zero of=fsimage.img bs=1024 count=32768
$ mkfs.ext3 fsimage.img
$ tune2fs -O ^has_journal fsimage.img
$ mount -o loop fsimage.img /mntpoint
$ cat /proc/self/mountinfo
43 19 7:0 / /mntpoint rw,relatime - ext3 /dev/loop0 rw

Now we have mounted ext3 without journaling, is that really ext3?

$ umount /mntpoint
$ mount -t ext2 -o loop fsimage.img /mntpoint
$ cat /proc/self/mountinfo
43 19 7:0 / /mntpoint rw,relatime - ext2 /dev/loop0 rw

Now we mounted the same filesystem as ext2.

And in both cases this is handled by the ext4 kernel driver anyways:

$ dmesg
...
EXT4-fs (loop0): mounting ext3 file system using the ext4 subsystem
...
EXT4-fs (loop0): mounting ext2 file system using the ext4 subsystem
...

So I would argue that these are not three different filesystems, but
just one that can turn on and off various features. And if you look at
'man ext4' you will see that the set of supported features for any extX
variant does change with kernel version.

The question is if we really need to be able to distinguish them or if
we can keep calling them by a common name.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH] tst_supported_fs: Implement checking paths against skiplist

[Petr Vorel]

> Hi!
> > Should I understand this that you don't trust /proc/self/mountinfo output?
> > (show_mountinfo() in fs/proc_namespace.c in kernel git)?

> Not much as I wouldn't trust it, it's just that the ext2/ext3/ext4 names
> are just short names for a subset of additional functionality.

> Consider this:

> $ dd if=/dev/zero of=fsimage.img bs=1024 count=32768
> $ mkfs.ext3 fsimage.img
> $ tune2fs -O ^has_journal fsimage.img
> $ mount -o loop fsimage.img /mntpoint
> $ cat /proc/self/mountinfo
> 43 19 7:0 / /mntpoint rw,relatime - ext3 /dev/loop0 rw

> Now we have mounted ext3 without journaling, is that really ext3?

> $ umount /mntpoint
> $ mount -t ext2 -o loop fsimage.img /mntpoint
> $ cat /proc/self/mountinfo
> 43 19 7:0 / /mntpoint rw,relatime - ext2 /dev/loop0 rw

> Now we mounted the same filesystem as ext2.

> And in both cases this is handled by the ext4 kernel driver anyways:

> $ dmesg
> ...
> EXT4-fs (loop0): mounting ext3 file system using the ext4 subsystem
> ...
> EXT4-fs (loop0): mounting ext2 file system using the ext4 subsystem
> ...

Thanks a lot for a nice example to document the problem.

> So I would argue that these are not three different filesystems, but
> just one that can turn on and off various features. And if you look at
> 'man ext4' you will see that the set of supported features for any extX
> variant does change with kernel version.

Makes sense.

> The question is if we really need to be able to distinguish them or if
> we can keep calling them by a common name.

As I noted before, using Martin's patch require to use ext2/ext3/ext4 for
.skip_filesystems and $TST_SKIP_FILESYSTEMS. I'm OK with it, although it's
a bit misleading.

Kind regards,
Petr

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

Re: [LTP] [PATCH] tst_supported_fs: Implement checking paths against skiplist

[Petr Vorel]

Hi Martin,

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Thanks for implementing this.

I guess after accepting this and my v2 (not yet sent),
we could remove 'tst_supported_fs -s skip_list fs_type'
as nothing uses it. But OTOH it does not harm to keep it.

Kind regards,
Petr

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

Re: [LTP] [PATCH] tst_supported_fs: Implement checking paths against skiplist

[Petr Vorel]

Hi,

> +++ b/testcases/lib/tst_supported_fs.c
...
>  	/* fs_type */
>  	if (optind < argc) {
> -		if (argv[optind][0] == '\0')
> +		if (fsname) {
> +			fprintf(stderr, "Can't specify fs_type and -d together\n");
> +			usage();
> +			return 2;
> +
nit: remove this extra line before merging.
> +		}
> +
> +		fsname = argv[optind];
> +	}
...

Kind regards,
Petr

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

Re: [LTP] [PATCH] tst_supported_fs: Implement checking paths against skiplist

[Petr Vorel]

Hi all,

> Implement tst_supported_fs feature suggested by pvorel in his patch:
> tst_test.sh: Fix filesystem support detection
FYI there is still be need to detect filesystem in particular directory in
ima_setup.sh [1] to avoid df dependency:

if [ "$(df -T $TMPDIR | tail -1 | awk '{print $2}')" != "tmpfs" -a -n "$TST_NEEDS_DEVICE" ]; then
	unset TST_NEEDS_DEVICE
fi

But I guess I'll just reuse the exit code of: ./tst_supported_fs -d $TMPDIR -s "tmpfs"

Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/blob/74e4a5c6fb42a1231c149c57efbca345aa93777d/testcases/kernel/security/integrity/ima/tests/ima_setup.sh#L355

> Although the tst_fs_type_name() functions could use some improvements,
> e.g. ext4 must be specified in skiplist as "ext2/ext3/ext4" to get properly
> skipped and vfat is missing from the list of known filesystems.


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

Re: [LTP] [PATCH] tst_supported_fs: Implement checking paths against skiplist

[Petr Vorel]

Hi all,

merged this one with 2 tiny fixes (sort getopts string, remove blank line).

Kind regards,
Petr

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

Re: [LTP] [PATCH] tst_supported_fs: Implement checking paths against skiplist

[Cyril Hrubis]

Hi!
> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> ---
> 
> Implement tst_supported_fs feature suggested by pvorel in his patch:
> tst_test.sh: Fix filesystem support detection
> 
> Although the tst_fs_type_name() functions could use some improvements,
> e.g. ext4 must be specified in skiplist as "ext2/ext3/ext4" to get properly
> skipped and vfat is missing from the list of known filesystems.
> 
>  testcases/lib/tst_supported_fs.c | 49 ++++++++++++++++++++++++++------
>  1 file changed, 41 insertions(+), 8 deletions(-)
> 
> diff --git a/testcases/lib/tst_supported_fs.c b/testcases/lib/tst_supported_fs.c
> index 70d4d38c7..5873d0ba1 100644
> --- a/testcases/lib/tst_supported_fs.c
> +++ b/testcases/lib/tst_supported_fs.c
> @@ -32,9 +32,13 @@ static void usage(void)
>  	fprintf(stderr, "tst_supported_fs -s skip_list fs_type\n");
>  	fprintf(stderr, "   if fs_type is in skip_list, return 1 otherwise return 0\n\n");
>  
> +	fprintf(stderr, "tst_supported_fs -s skip_list -d path\n");
> +	fprintf(stderr, "   if filesystem mounted on path is in skip_list, return 1 otherwise return 0\n\n");
> +
>  	fprintf(stderr, "fs_type - a specified filesystem type\n");
>  	fprintf(stderr, "skip_list - filesystems to skip, delimiter: '%c'\n",
>  			SKIP_DELIMITER);
> +	fprintf(stderr, "path - any valid file or directory\n");
>  }
>  
>  static char **parse_skiplist(char *fs)
> @@ -62,10 +66,11 @@ static char **parse_skiplist(char *fs)
>  int main(int argc, char *argv[])
>  {
>  	const char *const *filesystems;
> +	const char *fsname = NULL;
>  	int i, ret;
>  	char **skiplist = NULL;
>  
> -	while ((ret = getopt(argc, argv, "hs:"))) {
> +	while ((ret = getopt(argc, argv, "hs:d:"))) {
                                          ^
					  It's customary to sort these
					  in alphabetical order.

Other than that:

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

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