Re: [LTP] [PATCH v4 3/7] syscalls/swapon03: use tst_max_swapfiles() and GET_USED_SWAPFILES() api

[Petr Vorel <pvorel@suse.cz>]

Hi Yang Xu,

LGTM, few comments below.

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

> @@ -19,7 +19,6 @@

>  #include "tst_test.h"
>  #include "lapi/syscalls.h"
> -#include "swaponoff.h"
+1

>  #include "libswap.h"

>  #define MNTPOINT	"mntpoint"
> @@ -104,47 +103,20 @@ static void verify_swapon(void)

Here is comment:
/*
 * Create 33 and activate 30 swapfiles.
 */
This would deserve to update, because we use tst_max_swapfiles(), rirght?
Something like: "Create max number of swap files + 1 and activate max number
swap files -2."
>  static int setup_swap(void)
>  {
>  	pid_t pid;
> -	int j, fd;
>  	int status;
> +	int j, max_swapfiles, used_swapfiles;
>  	int res = 0;
>  	char filename[FILENAME_MAX];
> -	char buf[BUFSIZ + 1];
> -
> -	/* Find out how many swapfiles (1 line per entry) already exist */
> -	swapfiles = 0;

>  	if (seteuid(0) < 0)
>  		tst_brk(TFAIL | TERRNO, "Failed to call seteuid");
nit: while at it, could you please use here SAFE_SETEUID(0) ?

> -	/* This includes the first (header) line */
> -	if ((fd = open("/proc/swaps", O_RDONLY)) == -1) {
> -		tst_brk(TFAIL | TERRNO,
> -			"Failed to find out existing number of swap files");
> -	}
> -	do {
> -		char *p = buf;
> -		res = read(fd, buf, BUFSIZ);
> -		if (res < 0) {
> -			tst_brk(TFAIL | TERRNO,
> -				 "Failed to find out existing number of swap files");
> -		}
> -		buf[res] = '\0';
> -		while ((p = strchr(p, '\n'))) {
> -			p++;
> -			swapfiles++;
> -		}
> -	} while (BUFSIZ <= res);
> -	close(fd);
> -	if (swapfiles)
> -		swapfiles--;	/* don't count the /proc/swaps header */
Explicitly mention "don't count the /proc/swaps header" would not hurt in the
previous commit where you use -1.
> -
> -	if (swapfiles < 0)
> -		tst_brk(TFAIL, "Failed to find existing number of swapfiles");
> -
>  	/* Determine how many more files are to be created */
> -	swapfiles = MAX_SWAPFILES - swapfiles;
> -	if (swapfiles > MAX_SWAPFILES)
> -		swapfiles = MAX_SWAPFILES;
> +	max_swapfiles = tst_max_swapfiles();
> +	used_swapfiles = tst_count_swaps();
> +	swapfiles = max_swapfiles - used_swapfiles;
Instead of used_swapfiles directly call tst_count_swaps() can be used.
> +	if (swapfiles > max_swapfiles)
> +		swapfiles = max_swapfiles;

Please add here blank line for readability.
>  	pid = SAFE_FORK();
>  	if (pid == 0) {
>  		/*create and turn on remaining swapfiles */

Kind regards,
Petr

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