Re: [LTP] [PATCH v3 1/2] libswap: add two methods to create swapfile

[Petr Vorel <pvorel@suse.cz>]

Hi Li,

...
> Signed-off-by: Li Wang <liwang@redhat.com>
> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
...
-int make_swapfile(const char *swapfile, int blocks, int safe)
+int make_swapfile_(const char *file, const int lineno,
+			const char *swapfile, unsigned int num,
+			int safe, enum swapfile_method method)
 {
 	struct statvfs fs_info;
-	unsigned long blk_size, bs;
+	unsigned long blk_size;
+	unsigned int blocks = 0;
 	size_t pg_size = sysconf(_SC_PAGESIZE);
-	char mnt_path[100];
+	char mnt_path[128];

nit: why this increase to 128? Why not PATH_MAX?

>  	/* To guarantee at least one page can be swapped out */
> -	if (blk_size * blocks < pg_size)
> -		bs = pg_size;
> -	else
> -		bs = blk_size;
> +	if (blk_size * blocks < pg_size) {
> +		tst_res(TWARN, "Swapfile size is less than the system page size. \
> +			Using page size (%lu bytes) instead of block size (%lu bytes).",

libswap.c:163: WARNING: Avoid line continuations in quoted strings

This will fix it:

		tst_res(TWARN, "Swapfile size is less than the system page size. "
			"Using page size (%lu bytes) instead of block size (%lu bytes).",

> +			(unsigned long)pg_size, blk_size);
> +		blk_size = pg_size;
> +	}

>  	if (sscanf(swapfile, "%[^/]", mnt_path) != 1)
> -		tst_brk(TBROK, "sscanf failed");
> +		tst_brk_(file, lineno, TBROK, "sscanf failed");

> -	if (!tst_fs_has_free(mnt_path, bs * blocks, TST_BYTES))
> -		tst_brk(TCONF, "Insufficient disk space to create swap file");
> +	if (!tst_fs_has_free(mnt_path, blk_size * blocks, TST_BYTES))
> +		tst_brk_(file, lineno, TCONF, "Insufficient disk space to create swap file");

>  	/* create file */
> -	if (prealloc_contiguous_file(swapfile, bs, blocks) != 0)
> -		tst_brk(TBROK, "Failed to create swapfile");
> +	if (prealloc_contiguous_file(swapfile, blk_size, blocks) != 0)
> +		tst_brk_(file, lineno, TBROK, "Failed to create swapfile");

>  	/* Fill the file if needed (specific to old xfs filesystems) */
>  	if (tst_fs_type(swapfile) == TST_XFS_MAGIC) {
> -		if (tst_fill_file(swapfile, 0, bs, blocks) != 0)
> -			tst_brk(TBROK, "Failed to fill swapfile");
> +		if (tst_fill_file(swapfile, 0, blk_size, blocks) != 0)
> +			tst_brk_(file, lineno, TBROK, "Failed to fill swapfile");
>  	}

>  	/* make the file swapfile */
> -	const char *argv[2 + 1];
> -
> -	argv[0] = "mkswap";
> -	argv[1] = swapfile;
> -	argv[2] = NULL;
> +	const char *argv[] = {"mkswap", swapfile, NULL};

libswap.c:186: WARNING: char * array declaration might be better as static const

This will fix it:

	const char *const argv[] = {"mkswap", swapfile, NULL};

>  	return tst_cmd(argv, "/dev/null", "/dev/null", safe ?
> -				   TST_CMD_PASS_RETVAL | TST_CMD_TCONF_ON_MISSING : 0);
> +			TST_CMD_PASS_RETVAL | TST_CMD_TCONF_ON_MISSING : 0);

The rest LGTM.

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

Kind regards,
Petr

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