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

Petr Vorel
2024-02-23T09:48:55Z

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