diff mbox series

[v4,6/7] syscalls/swapon03: Simply this case

Message ID 20240220074218.13487-6-xuyang2018.jy@fujitsu.com
State Superseded
Headers show
Series [v4,1/7] libltpswap: Add tst_max_swapfiles api | expand

Commit Message

Yang Xu Feb. 20, 2024, 7:42 a.m. UTC
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.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 testcases/kernel/syscalls/swapon/swapon03.c | 168 +++++---------------
 1 file changed, 42 insertions(+), 126 deletions(-)

Comments

Petr Vorel Feb. 23, 2024, 11:48 a.m. UTC | #1
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");
> +	}
diff mbox series

Patch

diff --git a/testcases/kernel/syscalls/swapon/swapon03.c b/testcases/kernel/syscalls/swapon/swapon03.c
index b0d70b4ad..339b11c83 100644
--- a/testcases/kernel/syscalls/swapon/swapon03.c
+++ b/testcases/kernel/syscalls/swapon/swapon03.c
@@ -16,7 +16,7 @@ 
 #include <errno.h>
 #include <stdlib.h>
 #include <sys/wait.h>
-
+#include <sys/swap.h>
 #include "tst_test.h"
 #include "lapi/syscalls.h"
 #include "libswap.h"
@@ -24,88 +24,13 @@ 
 #define MNTPOINT	"mntpoint"
 #define TEST_FILE	MNTPOINT"/testswap"
 
-static int setup_swap(void);
-static int clean_swap(void);
-static int check_and_swapoff(const char *filename);
-
 static int swapfiles;
 
-int testfiles = 3;
-static struct swap_testfile_t {
-	char *filename;
-} swap_testfiles[] = {
-	{"firstswapfile"},
-	{"secondswapfile"},
-	{"thirdswapfile"}
-};
-
-int expected_errno = EPERM;
-
-static void verify_swapon(void)
-{
-	if (setup_swap() < 0) {
-		clean_swap();
-		tst_brk(TBROK, "Setup failed, quitting the test");
-	}
-
-	TEST(tst_syscall(__NR_swapon, swap_testfiles[0].filename, 0));
-
-	if ((TST_RET == -1) && (TST_ERR == expected_errno)) {
-		tst_res(TPASS | TTERRNO, "swapon(2) got expected failure");
-	} else if (TST_RET < 0) {
-		tst_res(TFAIL | TTERRNO,
-			"swapon(2) failed to produce expected error "
-			"(%d). System reboot recommended.",
-			expected_errno);
-	} else {
-		/*
-		 * Probably the system supports MAX_SWAPFILES > 30, let's try with
-		 * MAX_SWAPFILES == 32. Call swapon sys call once again for 32
-		 * now we can't receive an error.
-		 */
-		TEST(tst_syscall(__NR_swapon, swap_testfiles[1].filename, 0));
-
-		/* Check return code (now we're expecting success) */
-		if (TST_RET < 0) {
-			tst_res(TFAIL | TTERRNO,
-				"swapon(2) got an unexpected failure");
-		} else {
-			/*
-			 * Call swapon sys call once again for 33 now we have to receive an error.
-			 */
-			TEST(tst_syscall(__NR_swapon, swap_testfiles[2].filename, 0));
-
-			/* Check return code (should be an error) */
-			if ((TST_RET == -1) && (TST_ERR == expected_errno)) {
-				tst_res(TPASS,
-					"swapon(2) got expected failure;"
-					" Got errno = %d, probably your"
-					" MAX_SWAPFILES is 32",
-					expected_errno);
-			} else {
-				tst_res(TFAIL,
-					"swapon(2) failed to produce"
-					" expected error: %d, got %s."
-					" System reboot after execution of LTP"
-					" test suite is recommended.",
-					expected_errno, strerror(TST_ERR));
-			}
-		}
-	}
-
-	if (clean_swap() < 0)
-		tst_brk(TBROK, "Cleanup failed, quitting the test");
-}
-
-/*
- * Create 33 and activate 30 swapfiles.
- */
 static int setup_swap(void)
 {
 	pid_t pid;
 	int status;
 	int j, max_swapfiles, used_swapfiles;
-	int res = 0;
 	char filename[FILENAME_MAX];
 
 	if (seteuid(0) < 0)
@@ -127,16 +52,7 @@  static int setup_swap(void)
 			make_swapfile(filename, 10, 0);
 
 			/* turn on the swap file */
-			res = tst_syscall(__NR_swapon, filename, 0);
-			if (res != 0) {
-				if (errno == EPERM) {
-					printf("Successfully created %d swapfiles\n", j);
-					break;
-				} else {
-					printf("Failed to create swapfile: %s\n", filename);
-					exit(1);
-				}
-			}
+			TST_EXP_PASS_SILENT(swapon(filename, 0));
 		}
 		exit(0);
 	} else
@@ -145,13 +61,40 @@  static int setup_swap(void)
 	if (WEXITSTATUS(status))
 		tst_brk(TFAIL, "Failed to setup swaps");
 
-	/* 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);
+	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");
+	} else {
+		rc = 0;
+		if (system(cmd_buffer) == 0) {
+			/* now we need to swapoff the file */
+			if (swapoff(filename) != 0) {
+				tst_res(TWARN, "Failed to turn off swap "
+					"file. system reboot after "
+					"execution of LTP test suite "
+					"is recommended");
+				rc = -1;
+			}
+		}
+	}
+
+	return rc;
+}
+
 /*
  * Turn off all swapfiles previously turned on
  */
@@ -169,49 +112,17 @@  static int clean_swap(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");
+		return -1;
 	}
 
 	return 0;
 }
 
-/*
- * Check if the file is at /proc/swaps and remove it giving swapoff
- */
-static int check_and_swapoff(const char *filename)
+static void verify_swapon(void)
 {
-	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");
-	} else {
-
-		rc = 0;
-
-		if (system(cmd_buffer) == 0) {
-
-			/* now we need to swapoff the file */
-			if (tst_syscall(__NR_swapoff, filename) != 0) {
-
-				tst_res(TWARN, "Failed to turn off swap "
-					 "file. system reboot after "
-					 "execution of LTP test suite "
-					 "is recommended");
-				rc = -1;
-
-			}
-
-		}
-	}
-
-	return rc;
+	TST_EXP_FAIL(swapon(TEST_FILE, 0), EPERM, "swapon(%s, 0)", TEST_FILE);
 }
 
 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");
+	}
 }
 
 static void cleanup(void)