Re: [LTP] [PATCH v1] tst_filesystems01.c: Add test for .filesystems

[Petr Vorel <pvorel@suse.cz>]

Hi Wei, Cyril,

Cyril, single note below can wait after the release.
Also, please have look if this fulfils your idea about testing the feature.

> Fixes: https://github.com/linux-test-project/ltp/issues/1243
> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>  lib/newlib_tests/tst_filesystems01.c | 118 +++++++++++++++++++++++++++
>  1 file changed, 118 insertions(+)
>  create mode 100644 lib/newlib_tests/tst_filesystems01.c

The test is running fine on github, please add it to
lib/newlib_tests/runtest.sh.

+++ lib/newlib_tests/runtest.sh
@@ -24,6 +24,7 @@ tst_checkpoint_wait_timeout
 tst_checkpoint_wake_timeout
 tst_device
 tst_expiration_timer
+tst_filesystems01
 tst_fuzzy_sync0[1-3]
 tst_needs_cmds0[1-36-8]
 tst_res_hexd

...
> +	snprintf(str_size, sizeof(str_size), "%u", size);
> +	snprintf(path, sizeof(path), "tune2fs -l %s 2>&1", tst_device->dev);
> +	tune2fs = SAFE_POPEN(path, "r");
> +	if (tune2fs == NULL)
> +		tst_brk(TBROK, "Can not popen %s", path);
The point of SAFE_* functions/macros is to avoid having to check in the tests :).
Please remove these 2 lines.

> +	while (fgets(line, PATH_MAX, tune2fs)) {
> +		if (*line && strstr(line, "Inode size:") && strstr(line, str_size)) {
Common patter for fgets() is to check for != NULL, then you don't need "*line &&":
	while (fgets(line, PATH_MAX, tune2fs) != NULL) {
		if (strstr(line, "Inode size:") && strstr(line, str_size)) {

> +			tst_res(TPASS, "check inode size pass");
> +			return 0;
> +		}
> +	}
> +
> +	pclose(tune2fs);
> +	tst_res(TFAIL, "check inode size failed");
> +	return -1;
> +}
> +
> +static int check_mnt_data(char *opt)
> +{
> +	FILE *fp;
> +	char line[PATH_MAX];
> +
> +	fp = SAFE_FOPEN("/proc/mounts", "r");
> +	if (fp == NULL)
> +		tst_brk(TBROK, "Can not open /proc/mounts");
Also here.

> +	while (fgets(line, PATH_MAX, fp)) {
> +		if (*line && strstr(line, tst_device->dev) && strstr(line, opt)) {
Also here.
> +			tst_res(TPASS, "check mnt data pass");
> +			return 0;
> +		}
> +	}
> +	SAFE_FCLOSE(fp);
> +	tst_res(TFAIL, "check mnt data failed");
> +	return -1;
> +}
> +
> +static int check_mkfs_size_opt(unsigned int size)
> +{
> +	char path[PATH_MAX];
> +	char line[PATH_MAX];
> +	FILE *dumpe2fs;
> +	char str_size[NAME_MAX];
> +
> +	snprintf(str_size, sizeof(str_size), "%u", size);
> +	snprintf(path, sizeof(path), "dumpe2fs -h %s 2>&1", tst_device->dev);
> +	dumpe2fs = SAFE_POPEN(path, "r");
> +	if (dumpe2fs == NULL)
> +		tst_brk(TBROK, "Can not popen %s", path);
And here.

> +
> +	while (fgets(line, PATH_MAX, dumpe2fs)) {
> +		if (*line && strstr(line, "Block count:") && strstr(line, str_size)) {
And here.

> +			tst_res(TPASS, "check mkfs size opt pass");
> +			return 0;
> +		}
> +	}
> +
> +	pclose(dumpe2fs);
> +	tst_res(TFAIL, "check mkfs size opt failed");
> +	return -1;
> +}
> +
> +static void do_test(void)
> +{
> +	long fs_type;
> +
> +	fs_type = tst_fs_type(MOUNT_POINT);
> +
> +	if (fs_type == TST_EXT234_MAGIC) {
> +		check_inode_size(128);
check_inode_size() and the other 2 test functions return int but you don't use
the return value. Either use it or change return type to void.

> +		check_mkfs_size_opt(10240);
> +	}
> +
> +	if (fs_type == TST_XFS_MAGIC)
> +		check_mnt_data("usrquota");
While this works, it expect that nobody changes .filesystems below.

@Cyril: wouldn't be useful if the test itself had it's struct tst_fs member
from filesystems[i] available? Static variable, either pointer to the member or
instance of i from run_tcases_per_fs()?

Because we always expect filesystems (.type) is unique in .filesystems. But in
the future we might have test which uses several items in .filesystems with a
different data but an unique filesystem (not needed so far).

Kind regards,
Petr

> +}
> +
> +static struct tst_test test = {
> +	.test_all = do_test,
> +	.needs_root = 1,
> +	.mntpoint = MOUNT_POINT,
> +	.mount_device = 1,
> +	.needs_cmds = (const char *[]) {
> +		"tune2fs",
> +		"dumpe2fs",
> +		NULL
> +	},
> +	.filesystems = (struct tst_fs []) {
> +		{
> +			.type = "ext3",
> +			.mkfs_opts = (const char *const []){"-I", "128", NULL},
> +			.mkfs_size_opt = "10240",
> +		},
> +		{
> +			.type = "xfs",
> +			.mnt_data = "usrquota",
> +		},
> +		{}
> +	},
> +
> +};

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