[LTP] [PATCH 1/1] lib: Rename array used for .all_filesystems flag

[LTP] [PATCH 1/1] lib: Rename array used for .all_filesystems flag

[Petr Vorel]

Although fs_type_whitelist[] is also used for whitelisting with
.skip_filesystems, the main purpose is to be used for .all_filesystems.

Thus rename it to all_filesystems[].

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 lib/tst_supported_fs_types.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/lib/tst_supported_fs_types.c b/lib/tst_supported_fs_types.c
index 23e5ce8775..6f1c46da35 100644
--- a/lib/tst_supported_fs_types.c
+++ b/lib/tst_supported_fs_types.c
@@ -14,7 +14,11 @@
 #include "tst_test.h"
 #include "tst_fs.h"
 
-static const char *const fs_type_whitelist[] = {
+/*
+ * Filesystems to be tested with .all_filesystems and also the only filesystems
+ * which can be whitelisted with .skip_filesystems.
+ */
+static const char *const all_filesystems[] = {
 	"ext2",
 	"ext3",
 	"ext4",
@@ -27,7 +31,7 @@ static const char *const fs_type_whitelist[] = {
 	NULL
 };
 
-static const char *fs_types[ARRAY_SIZE(fs_type_whitelist)];
+static const char *fs_types[ARRAY_SIZE(all_filesystems)];
 
 static int has_mkfs(const char *fs_type)
 {
@@ -151,24 +155,24 @@ const char **tst_get_supported_fs_types(const char *const *skiplist)
 		return fs_types;
 	}
 
-	for (i = 0; fs_type_whitelist[i]; i++) {
-		if (tst_fs_in_skiplist(fs_type_whitelist[i], skiplist)) {
+	for (i = 0; all_filesystems[i]; i++) {
+		if (tst_fs_in_skiplist(all_filesystems[i], skiplist)) {
 			tst_res(TINFO, "Skipping %s as requested by the test",
-				fs_type_whitelist[i]);
+				all_filesystems[i]);
 			continue;
 		}
 
-		sup = tst_fs_is_supported(fs_type_whitelist[i]);
+		sup = tst_fs_is_supported(all_filesystems[i]);
 
 		if (skip_fuse && sup == TST_FS_FUSE) {
 			tst_res(TINFO,
 				"Skipping FUSE based %s as requested by the test",
-				fs_type_whitelist[i]);
+				all_filesystems[i]);
 			continue;
 		}
 
 		if (sup)
-			fs_types[j++] = fs_type_whitelist[i];
+			fs_types[j++] = all_filesystems[i];
 	}
 
 	return fs_types;
-- 
2.34.1


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

Re: [LTP] [PATCH 1/1] lib: Rename array used for .all_filesystems flag

[Petr Vorel]

Hi all,

> Although fs_type_whitelist[] is also used for whitelisting with
> .skip_filesystems, the main purpose is to be used for .all_filesystems.

> Thus rename it to all_filesystems[].

NOTE: the main purpose of this change is to increase code readability.
https://lore.kernel.org/ltp/CAEemH2fNfFes-eUtiQKX9JJxqEQUQ+O5nWQM8G-yNyTo8sxviw@mail.gmail.com/

That's why I added doc.

Kind regards,
Petr

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

Re: [LTP] [PATCH 1/1] lib: Rename array used for .all_filesystems flag

[Cyril Hrubis]

Hi!
> > Although fs_type_whitelist[] is also used for whitelisting with
> > .skip_filesystems, the main purpose is to be used for .all_filesystems.
> 
> > Thus rename it to all_filesystems[].
> 
> NOTE: the main purpose of this change is to increase code readability.
> https://lore.kernel.org/ltp/CAEemH2fNfFes-eUtiQKX9JJxqEQUQ+O5nWQM8G-yNyTo8sxviw@mail.gmail.com/
> 
> That's why I added doc.

I guess that fs_type_whitelist[] may be confusing but all_filesystems[]
is IMHO not that much better since we use that a as base for the
all_filesystem before we filter out these that are not supported. Maybe
we should call it try_filesystems[] instead?

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 1/1] lib: Rename array used for .all_filesystems flag

[Petr Vorel]

Hi Cyril,

> Hi!
> > > Although fs_type_whitelist[] is also used for whitelisting with
> > > .skip_filesystems, the main purpose is to be used for .all_filesystems.

> > > Thus rename it to all_filesystems[].

> > NOTE: the main purpose of this change is to increase code readability.
> > https://lore.kernel.org/ltp/CAEemH2fNfFes-eUtiQKX9JJxqEQUQ+O5nWQM8G-yNyTo8sxviw@mail.gmail.com/

> > That's why I added doc.

> I guess that fs_type_whitelist[] may be confusing but all_filesystems[]
> is IMHO not that much better since we use that a as base for the
> all_filesystem before we filter out these that are not supported. Maybe
> we should call it try_filesystems[] instead?
Well, how I understand it the main feature is to be for .all_filesystems. And
items of this array can be whitelisted. Thus try_filesystems does not evoke much
to me that it's for all_filesystems.

I considered to have array all_filesystems[] and .fs_type_whitelist pointer to
that array, but having pointer just to document things is bad idea.

Kind regards,
Petr

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

Re: [LTP] [PATCH 1/1] lib: Rename array used for .all_filesystems flag

[Cyril Hrubis]

Hi!
> > I guess that fs_type_whitelist[] may be confusing but all_filesystems[]
> > is IMHO not that much better since we use that a as base for the
> > all_filesystem before we filter out these that are not supported. Maybe
> > we should call it try_filesystems[] instead?
> Well, how I understand it the main feature is to be for .all_filesystems. And
> items of this array can be whitelisted. Thus try_filesystems does not evoke much
> to me that it's for all_filesystems.
> 
> I considered to have array all_filesystems[] and .fs_type_whitelist pointer to
> that array, but having pointer just to document things is bad idea.

The reason why I do not think it's reasonable to name the array
all_filesystems is that setting the .all_filesystems flag does not mean
that the test would be run over all these filesytems. We just silently
skip these that are not supported...

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 1/1] lib: Rename array used for .all_filesystems flag

[Petr Vorel]

> Hi!
> > > I guess that fs_type_whitelist[] may be confusing but all_filesystems[]
> > > is IMHO not that much better since we use that a as base for the
> > > all_filesystem before we filter out these that are not supported. Maybe
> > > we should call it try_filesystems[] instead?
> > Well, how I understand it the main feature is to be for .all_filesystems. And
> > items of this array can be whitelisted. Thus try_filesystems does not evoke much
> > to me that it's for all_filesystems.

> > I considered to have array all_filesystems[] and .fs_type_whitelist pointer to
> > that array, but having pointer just to document things is bad idea.

> The reason why I do not think it's reasonable to name the array
> all_filesystems is that setting the .all_filesystems flag does not mean
> that the test would be run over all these filesytems. We just silently
> skip these that are not supported...

Understand. Also try_filesystems is certainly better than fs_type_whitelist.
How about default_all_filesystems or just default_filesystems?

Regardless we rename it or not, at least the comment I put in the patchset would
improve things (although you may phrase it better).

Kind regards,
Petr

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