| Message ID | 20220126141152.6428-1-pvorel@suse.cz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] lib: Rename array used for .all_filesystems flag | expand |
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
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?
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
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...
> 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
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;
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(-)