Re: [LTP] [PATCH v4 6/7] syscalls/swapon03: Simply this case

[Petr Vorel <pvorel@suse.cz>]

Hi Yang Xu,

> By moving swapfile create stage from verify_swaopon and
> test EPERM error more accurate. Also use glibc wrapper by

> using swapon/swapoff instead of call syscall number directly
> because glibc/musl/binoic also support them since long time ago.
+1 thanks for checking.

FYI uClibc-ng only support with UCLIBC_LINUX_SPECIFIC config enabled, but it's
by default enabled. And I guess nobody runs uClibc-ng on Linux without this
enabled => really safe to depend on libc wrapper.

...
> +			TST_EXP_PASS_SILENT(swapon(filename, 0));
+1

>  		}
>  		exit(0);
>  	} else
> @@ -145,13 +61,40 @@ static int setup_swap(void)
>  	if (WEXITSTATUS(status))
>  		tst_brk(TFAIL, "Failed to setup swaps");
nit: s/swaps/swap files/

> -	/* Create all needed extra swapfiles for testing */
> -	for (j = 0; j < testfiles; j++)
> -		make_swapfile(swap_testfiles[j].filename, 10, 0);
> +	tst_res(TINFO, "Successfully created %d swapfiles", swapfiles);
nit: s/swapfiles/swap files/

> +	make_swapfile(TEST_FILE, 10, 0);

>  	return 0;
>  }

> +/*
> + * Check if the file is at /proc/swaps and remove it giving swapoff
> + */
> +static int check_and_swapoff(const char *filename)
> +{
> +	char cmd_buffer[256];
> +	int rc = -1;
> +
> +	if (snprintf(cmd_buffer, sizeof(cmd_buffer),
> +		"grep -q '%s.*file' /proc/swaps", filename) < 0) {
> +		tst_res(TWARN, "sprintf() failed to create the command string");
nit: we don't have SAFE_SNPRINTF() and don't even check snprintf() / sprintf()
return value. Shouldn't we add SAFE_SNPRINTF() which TBROK?
This can be handled later, thus I would here either use plain snprintf() or
tst_brk(TBROK).

if you add return -1 here, the following block does not have to be in else
(=> fewer indentation => text can be longer fewer string splits).

> +	} else {
> +		rc = 0;
> +		if (system(cmd_buffer) == 0) {
> +			/* now we need to swapoff the file */
> +			if (swapoff(filename) != 0) {

Why not single if?
		if (system(cmd_buffer) == 0) && swapoff(filename) != 0) {

> +				tst_res(TWARN, "Failed to turn off swap "
> +					"file. system reboot after "
> +					"execution of LTP test suite "
> +					"is recommended");
Then this string would not need to be split several times (bad for searching
with 'git grep'). Maybe shorten just to
"Failed to swapoff %", filename"
=> more important than suggest to reboot (which is obvious) is to point out
problematic swap file, which was kept on.


> +				rc = -1;
> +			}
> +		}
> +	}
> +
> +	return rc;
> +}
> +
>  /*
>   * Turn off all swapfiles previously turned on
>   */
> @@ -169,49 +112,17 @@ static int clean_swap(void)
Return code of clean_swap() is not used. How about to make it void?
>  		}
>  	}

> -	for (j = 0; j < testfiles; j++) {
> -		if (check_and_swapoff(swap_testfiles[j].filename) != 0) {
> -			tst_res(TWARN, "Failed to turn off swap file %s.",
> -				 swap_testfiles[j].filename);
> -			return -1;
> -		}
> +	if (check_and_swapoff("testfile") != 0) {
> +		tst_res(TWARN, "Failed to turn off swap file testfile");
We have the warning in the function, why also here?
> +		return -1;
>  	}

>  	return 0;
>  }

...
> +static void verify_swapon(void)
>  {
> +	TST_EXP_FAIL(swapon(TEST_FILE, 0), EPERM, "swapon(%s, 0)", TEST_FILE);
+1

Kind regards,
Petr
>  }

>  static void setup(void)
> @@ -220,6 +131,11 @@ static void setup(void)
>  		tst_brk(TCONF, "swap not supported by kernel");

>  	is_swap_supported(TEST_FILE);
> +
> +	if (setup_swap() < 0) {
> +		clean_swap();
> +		tst_brk(TBROK, "Setup failed, quitting the test");
> +	}

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