diff mbox series

[v2,1/2] libswap: add two methods to create swapfile

Message ID 20240319100822.3243785-2-liwang@redhat.com
State Changes Requested
Headers show
Series add method to create swapfile by MB size | expand

Commit Message

Li Wang March 19, 2024, 10:08 a.m. UTC 
This introduces new functionality to the libswap library by adding
two methods for creating a swapfile: SWAPFILE_BY_SIZE and SWAPFILE_BY_BLOCKS.
The make_swapfile function is updated to accept an additional enum
swapfile_method parameter to specify the creation method.

Two macros, MAKE_SWAPFILE_SIZE and MAKE_SWAPFILE_BLKS, are defined
to simplify the interface for creating swapfiles by size and by
blocks respectively.

Signed-off-by: Li Wang <liwang@redhat.com>
Signed-off-by: Wei Gao <wegao@suse.com>
Reviewed-by: Wei Gao <wegao@suse.com>
---
 include/libswap.h                             | 16 ++++++++++++++--
 libs/libltpswap/libswap.c                     | 14 +++++++++++---
 testcases/kernel/syscalls/swapoff/swapoff01.c |  2 +-
 testcases/kernel/syscalls/swapoff/swapoff02.c |  2 +-
 testcases/kernel/syscalls/swapon/swapon01.c   |  2 +-
 testcases/kernel/syscalls/swapon/swapon02.c   |  4 ++--
 testcases/kernel/syscalls/swapon/swapon03.c   |  4 ++--
 7 files changed, 32 insertions(+), 12 deletions(-)

Comments

Petr Vorel March 20, 2024, 7:31 a.m. UTC | #1 
Hi Li,

Generally LGTM.

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

...
>  /*
> - * Make a swap file
> + * Create a swapfile of a specified size or number of blocks.
>   */
> -int make_swapfile(const char *swapfile, int blocks, int safe);
> +int make_swapfile(const char *swapfile, unsigned int num,
> +			int safe, enum swapfile_method method);
I wonder if it would help to add const char *file, const int lineno here.

> +
> +#define MAKE_SWAPFILE_SIZE(swapfile, size, safe) \
> +    make_swapfile(swapfile, size, safe, SWAPFILE_BY_SIZE)
nit: I like the name but one have to search which units (kB vs. MB vs. GB) are used.

> +
> +#define MAKE_SWAPFILE_BLKS(swapfile, blocks, safe) \
> +    make_swapfile(swapfile, blocks, safe, SWAPFILE_BY_BLKS)

And we could also have SAFE_ variants.

Therefore maybe rename make_swapfile() to make_swapfile_()
(approach in LTP for functions to be wrapped) and define macros:

int make_swapfile_(const char *file, const int lineno,
	const char *swapfile, unsigned int num,
	int safe, enum swapfile_method method);

#define MAKE_SWAPFILE_SIZE(swapfile, size, safe) \
    make_swapfile_(__FILE__, __LINE__, swapfile, size, 0, SWAPFILE_BY_SIZE)

#define MAKE_SWAPFILE_BLKS(swapfile, blocks, safe) \
    make_swapfile_(__FILE__, __LINE__, swapfile, blocks, 0, SWAPFILE_BY_BLKS)

#define SAFE_MAKE_SWAPFILE_SIZE(swapfile, size, safe) \
    make_swapfile_(__FILE__, __LINE__, swapfile, size, 1, SWAPFILE_BY_SIZE)

#define SAFE_MAKE_SWAPFILE_BLKS(swapfile, blocks, safe) \
    make_swapfile_(__FILE__, __LINE__, swapfile, blocks, 1, SWAPFILE_BY_BLKS)


>  /*
>   * Check swapon/swapoff support status of filesystems or files
> diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c
> index a26ea25e4..0e2476ec2 100644
> --- a/libs/libltpswap/libswap.c
> +++ b/libs/libltpswap/libswap.c
> @@ -133,18 +133,26 @@ out:
>  	return contiguous;
>  }

> -int make_swapfile(const char *swapfile, int blocks, int safe)
> +int make_swapfile(const char *swapfile, unsigned int num, int safe, enum swapfile_method method)
>  {
>  	struct statvfs fs_info;
>  	unsigned long blk_size, bs;
>  	size_t pg_size = sysconf(_SC_PAGESIZE);
>  	char mnt_path[100];
> +	unsigned int blocks = 0;

>  	if (statvfs(".", &fs_info) == -1)
>  		return -1;

>  	blk_size = fs_info.f_bsize;

> +	if (method == SWAPFILE_BY_SIZE)
> +		blocks = num * 1024 * 1024 / blk_size;
> +	else if (method == SWAPFILE_BY_BLKS)
> +		blocks = num;
> +	else
> +		tst_brk(TBROK, "Invalid method, please see include/libswap.h");

nit: I would print the method.

Using const char *file, const int lineno and tst_brk_() would help
later to point out which file actually contains wrong method.

...

Kind regards,
Petr
Li Wang March 21, 2024, 9:49 a.m. UTC | #2 
On Wed, Mar 20, 2024 at 3:31 PM Petr Vorel <pvorel@suse.cz> wrote:

> Hi Li,
>
> Generally LGTM.
>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>
> ...
> >  /*
> > - * Make a swap file
> > + * Create a swapfile of a specified size or number of blocks.
> >   */
> > -int make_swapfile(const char *swapfile, int blocks, int safe);
> > +int make_swapfile(const char *swapfile, unsigned int num,
> > +                     int safe, enum swapfile_method method);
> I wonder if it would help to add const char *file, const int lineno here.
>
> > +
> > +#define MAKE_SWAPFILE_SIZE(swapfile, size, safe) \
> > +    make_swapfile(swapfile, size, safe, SWAPFILE_BY_SIZE)
> nit: I like the name but one have to search which units (kB vs. MB vs. GB)
> are used.
>

Maybe we can add code comments like:

+/**
+ * Macro to create swapfile size in megabytes (MB)
+ */
 #define MAKE_SWAPFILE_SIZE(swapfile, size, safe) \
    ...

+/**
+ * Macro to create swapfile size in block numbers
+ */
 #define MAKE_SWAPFILE_BLKS(swapfile, blocks, safe) \
      ...


> > +
> > +#define MAKE_SWAPFILE_BLKS(swapfile, blocks, safe) \
> > +    make_swapfile(swapfile, blocks, safe, SWAPFILE_BY_BLKS)
>
> And we could also have SAFE_ variants.
>
> Therefore maybe rename make_swapfile() to make_swapfile_()
> (approach in LTP for functions to be wrapped) and define macros:
>
> int make_swapfile_(const char *file, const int lineno,
>         const char *swapfile, unsigned int num,
>         int safe, enum swapfile_method method);
>
> #define MAKE_SWAPFILE_SIZE(swapfile, size, safe) \
>     make_swapfile_(__FILE__, __LINE__, swapfile, size, 0, SWAPFILE_BY_SIZE)
>
> #define MAKE_SWAPFILE_BLKS(swapfile, blocks, safe) \
>     make_swapfile_(__FILE__, __LINE__, swapfile, blocks, 0,
> SWAPFILE_BY_BLKS)
>
> #define SAFE_MAKE_SWAPFILE_SIZE(swapfile, size, safe) \
>     make_swapfile_(__FILE__, __LINE__, swapfile, size, 1, SWAPFILE_BY_SIZE)
>
> #define SAFE_MAKE_SWAPFILE_BLKS(swapfile, blocks, safe) \
>     make_swapfile_(__FILE__, __LINE__, swapfile, blocks, 1,
> SWAPFILE_BY_BLKS)
>


Good suggestions.


>
> >  /*
> >   * Check swapon/swapoff support status of filesystems or files
> > diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c
> > index a26ea25e4..0e2476ec2 100644
> > --- a/libs/libltpswap/libswap.c
> > +++ b/libs/libltpswap/libswap.c
> > @@ -133,18 +133,26 @@ out:
> >       return contiguous;
> >  }
>
> > -int make_swapfile(const char *swapfile, int blocks, int safe)
> > +int make_swapfile(const char *swapfile, unsigned int num, int safe,
> enum swapfile_method method)
> >  {
> >       struct statvfs fs_info;
> >       unsigned long blk_size, bs;
> >       size_t pg_size = sysconf(_SC_PAGESIZE);
> >       char mnt_path[100];
> > +     unsigned int blocks = 0;
>
> >       if (statvfs(".", &fs_info) == -1)
> >               return -1;
>
> >       blk_size = fs_info.f_bsize;
>
> > +     if (method == SWAPFILE_BY_SIZE)
> > +             blocks = num * 1024 * 1024 / blk_size;
> > +     else if (method == SWAPFILE_BY_BLKS)
> > +             blocks = num;
> > +     else
> > +             tst_brk(TBROK, "Invalid method, please see
> include/libswap.h");
>
> nit: I would print the method.
>

+1


> Using const char *file, const int lineno and tst_brk_() would help
> later to point out which file actually contains wrong method.
>

Yes, that should be better. Thanks!
Petr Vorel March 21, 2024, 12:41 p.m. UTC | #3 
...
> > > -int make_swapfile(const char *swapfile, int blocks, int safe);
> > > +int make_swapfile(const char *swapfile, unsigned int num,
> > > +                     int safe, enum swapfile_method method);
> > I wonder if it would help to add const char *file, const int lineno here.

> > > +
> > > +#define MAKE_SWAPFILE_SIZE(swapfile, size, safe) \
> > > +    make_swapfile(swapfile, size, safe, SWAPFILE_BY_SIZE)
> > nit: I like the name but one have to search which units (kB vs. MB vs. GB)
> > are used.


> Maybe we can add code comments like:

> +/**
> + * Macro to create swapfile size in megabytes (MB)
> + */
>  #define MAKE_SWAPFILE_SIZE(swapfile, size, safe) \
>     ...

> +/**
> + * Macro to create swapfile size in block numbers
> + */
>  #define MAKE_SWAPFILE_BLKS(swapfile, blocks, safe) \
>       ...

Sure, that's enough.

Kind regards,
Petr
diff mbox series

Patch

diff --git a/include/libswap.h b/include/libswap.h
index 8c75e20ae..85ba88ed6 100644
--- a/include/libswap.h
+++ b/include/libswap.h
@@ -11,10 +11,22 @@ 
 #ifndef __LIBSWAP_H__
 #define __LIBSWAP_H__
 
+enum swapfile_method {
+    SWAPFILE_BY_SIZE,
+    SWAPFILE_BY_BLKS
+};
+
 /*
- * Make a swap file
+ * Create a swapfile of a specified size or number of blocks.
  */
-int make_swapfile(const char *swapfile, int blocks, int safe);
+int make_swapfile(const char *swapfile, unsigned int num,
+			int safe, enum swapfile_method method);
+
+#define MAKE_SWAPFILE_SIZE(swapfile, size, safe) \
+    make_swapfile(swapfile, size, safe, SWAPFILE_BY_SIZE)
+
+#define MAKE_SWAPFILE_BLKS(swapfile, blocks, safe) \
+    make_swapfile(swapfile, blocks, safe, SWAPFILE_BY_BLKS)
 
 /*
  * Check swapon/swapoff support status of filesystems or files
diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c
index a26ea25e4..0e2476ec2 100644
--- a/libs/libltpswap/libswap.c
+++ b/libs/libltpswap/libswap.c
@@ -133,18 +133,26 @@  out:
 	return contiguous;
 }
 
-int make_swapfile(const char *swapfile, int blocks, int safe)
+int make_swapfile(const char *swapfile, unsigned int num, int safe, enum swapfile_method method)
 {
 	struct statvfs fs_info;
 	unsigned long blk_size, bs;
 	size_t pg_size = sysconf(_SC_PAGESIZE);
 	char mnt_path[100];
+	unsigned int blocks = 0;
 
 	if (statvfs(".", &fs_info) == -1)
 		return -1;
 
 	blk_size = fs_info.f_bsize;
 
+	if (method == SWAPFILE_BY_SIZE)
+		blocks = num * 1024 * 1024 / blk_size;
+	else if (method == SWAPFILE_BY_BLKS)
+		blocks = num;
+	else
+		tst_brk(TBROK, "Invalid method, please see include/libswap.h");
+
 	/* To guarantee at least one page can be swapped out */
 	if (blk_size * blocks < pg_size)
 		bs = pg_size;
@@ -175,13 +183,13 @@  int make_swapfile(const char *swapfile, int blocks, int safe)
 	argv[2] = NULL;
 
 	return tst_cmd(argv, "/dev/null", "/dev/null", safe ?
-				   TST_CMD_PASS_RETVAL | TST_CMD_TCONF_ON_MISSING : 0);
+			TST_CMD_PASS_RETVAL | TST_CMD_TCONF_ON_MISSING : 0);
 }
 
 bool is_swap_supported(const char *filename)
 {
 	int i, sw_support = 0;
-	int ret = make_swapfile(filename, 10, 1);
+	int ret = MAKE_SWAPFILE_BLKS(filename, 10, 1);
 	int fi_contiguous = file_is_contiguous(filename);
 	long fs_type = tst_fs_type(filename);
 	const char *fstype = tst_fs_type_name(fs_type);
diff --git a/testcases/kernel/syscalls/swapoff/swapoff01.c b/testcases/kernel/syscalls/swapoff/swapoff01.c
index 2a0b683c1..d0d7c3c1f 100644
--- a/testcases/kernel/syscalls/swapoff/swapoff01.c
+++ b/testcases/kernel/syscalls/swapoff/swapoff01.c
@@ -44,7 +44,7 @@  static void setup(void)
 {
 	is_swap_supported(TEST_FILE);
 
-	if (make_swapfile(SWAP_FILE, 65536, 1))
+	if (MAKE_SWAPFILE_BLKS(SWAP_FILE, 65536, 1))
 		tst_brk(TBROK, "Failed to create file for swap");
 }
 
diff --git a/testcases/kernel/syscalls/swapoff/swapoff02.c b/testcases/kernel/syscalls/swapoff/swapoff02.c
index 52906848f..b57290386 100644
--- a/testcases/kernel/syscalls/swapoff/swapoff02.c
+++ b/testcases/kernel/syscalls/swapoff/swapoff02.c
@@ -88,7 +88,7 @@  static void setup(void)
 
 	is_swap_supported(TEST_FILE);
 
-	if (make_swapfile(SWAP_FILE, 10, 1))
+	if (MAKE_SWAPFILE_BLKS(SWAP_FILE, 10, 1))
 		tst_brk(TBROK, "Failed to create file for swap");
 }
 
diff --git a/testcases/kernel/syscalls/swapon/swapon01.c b/testcases/kernel/syscalls/swapon/swapon01.c
index d406e4bd9..2e399db61 100644
--- a/testcases/kernel/syscalls/swapon/swapon01.c
+++ b/testcases/kernel/syscalls/swapon/swapon01.c
@@ -38,7 +38,7 @@  static void verify_swapon(void)
 static void setup(void)
 {
 	is_swap_supported(SWAP_FILE);
-	make_swapfile(SWAP_FILE, 10, 0);
+	MAKE_SWAPFILE_BLKS(SWAP_FILE, 10, 0);
 
 	SAFE_CG_PRINTF(tst_cg, "cgroup.procs", "%d", getpid());
 	SAFE_CG_PRINTF(tst_cg, "memory.max", "%lu", TESTMEM);
diff --git a/testcases/kernel/syscalls/swapon/swapon02.c b/testcases/kernel/syscalls/swapon/swapon02.c
index 7e876d26a..f76bb28cf 100644
--- a/testcases/kernel/syscalls/swapon/swapon02.c
+++ b/testcases/kernel/syscalls/swapon/swapon02.c
@@ -50,8 +50,8 @@  static void setup(void)
 	is_swap_supported(TEST_FILE);
 
 	SAFE_TOUCH(NOTSWAP_FILE, 0777, NULL);
-	make_swapfile(SWAP_FILE, 10, 0);
-	make_swapfile(USED_FILE, 10, 0);
+	MAKE_SWAPFILE_BLKS(SWAP_FILE, 10, 0);
+	MAKE_SWAPFILE_BLKS(USED_FILE, 10, 0);
 
 	if (tst_syscall(__NR_swapon, USED_FILE, 0))
 		tst_res(TWARN | TERRNO, "swapon(alreadyused) failed");
diff --git a/testcases/kernel/syscalls/swapon/swapon03.c b/testcases/kernel/syscalls/swapon/swapon03.c
index 6f47fc01f..aaaedfa11 100644
--- a/testcases/kernel/syscalls/swapon/swapon03.c
+++ b/testcases/kernel/syscalls/swapon/swapon03.c
@@ -49,7 +49,7 @@  static int setup_swap(void)
 
 			/* Create the swapfile */
 			snprintf(filename, sizeof(filename), "%s%02d", TEST_FILE, j + 2);
-			make_swapfile(filename, 10, 0);
+			MAKE_SWAPFILE_BLKS(filename, 10, 0);
 
 			/* turn on the swap file */
 			TST_EXP_PASS_SILENT(swapon(filename, 0));
@@ -62,7 +62,7 @@  static int setup_swap(void)
 		tst_brk(TFAIL, "Failed to setup swap files");
 
 	tst_res(TINFO, "Successfully created %d swap files", swapfiles);
-	make_swapfile(TEST_FILE, 10, 0);
+	MAKE_SWAPFILE_BLKS(TEST_FILE, 10, 0);
 
 	return 0;
 }