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

Petr Vorel
2024-02-23T11:48:43Z

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");
> + }