Re: [LTP] [PATCH v2 2/5] swapon03: Cleanup

Petr Vorel
2025-11-11T15:48:11Z

> Hi!
> > - Remove unused return code in check_and_swapoff() and setup_swap(). The
> > purpose was to run cleanup() in the end of the setup() if creating
> > swap fails, but return code is always 0. Also cleanup() should be run
> > when test exits with tst_brk() anyway.
> > - Change return code from TFAIL to TBROK.
> > - Add missing brackets.

> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> > The same as in v1.
> > I can split this if you like.

> > testcases/kernel/syscalls/swapon/swapon03.c | 23 +++++++--------------
> > 1 file changed, 7 insertions(+), 16 deletions(-)

> > diff --git a/testcases/kernel/syscalls/swapon/swapon03.c b/testcases/kernel/syscalls/swapon/swapon03.c
> > index 01a3b6d8ac..d9822c01ef 100644
> > --- a/testcases/kernel/syscalls/swapon/swapon03.c
> > +++ b/testcases/kernel/syscalls/swapon/swapon03.c
> > @@ -25,7 +25,7 @@

> > static int swapfiles;

> > -static int setup_swap(void)
> > +static void setup_swap(void)
> > {
> > pid_t pid;
> > int status;
> > @@ -54,33 +54,27 @@ static int setup_swap(void)
> > TST_EXP_PASS_SILENT(swapon(filename, 0));
> > }
> > exit(0);
> > - } else
> > + } else {
> > waitpid(pid, &status, 0);
> > + }

> > if (WEXITSTATUS(status))
> > - tst_brk(TFAIL, "Failed to setup swap files");
> > + tst_brk(TBROK, "Failed to setup swap files");

> I wonder why do we run this code in a child process to begin with? This

Yes, although this is a general recommendation I was thinking about removing it
as well as. and first intended to remove it. But then I thought it was easier
to handle 'mkswap' binary failure (run in make_swapfile() which is called via
MAKE_SMALL_SWAPFILE()).

Other reason was that I did already quite a lot of cleanup, thus extending it
even more means more iterations. But sure, I can fix it (and postpone fix in the
first commit to later).

> is probably some leftover from the conversion to the new library. Among
> other things this masks proper results propagation from the
> MAKE_SMALL_SWAPFILE() because there is at least one tst_brk(TCONF, "")
> in there that will be converted to TBROK here.

Ah, You mean:
tst_brk_(file, lineno, TCONF, "Insufficient disk space to create swap file");
Indeed, I overlooked that one. It's always useful to look into the tests we
already converted some time ago :).

> I guess that we want to remove the fork() here and we want SAFE_SWAPON()
> instead of the TST_EXP_PASS_SILENT() so that we do tst_brk() if we fail

FYI we don't have SAFE_SWAPON() but this part of is_swap_supported() could be
factored out as SAFE_SWAPON():

TEST(tst_syscall(__NR_swapon, filename, 0));
if (TST_RET == -1) {
if (errno == EPERM) {
tst_brk(TCONF, "Permission denied for swapon()");
} else if (errno == EINVAL && fi_contiguous == 0 && sw_support == 0) {
tst_brk(TCONF, "Swapfile on %s not implemented", fstype);
} else {
tst_res(TFAIL | TTERRNO, "swapon() on %s failed", fstype);
return false;
}
}

Also we could use SAFE_MAKE_SMALL_SWAPFILE() instead of MAKE_SMALL_SWAPFILE(),
to catch few errors properly (this is what I did in 3rd commit).

> to setup the swapfile too. The TST_EXP_PASS_SILENT() does not end the
> test on a failure, which is what we want here instead.

Yes, that's the problem now. It could be fixed by calling tst_brk()
instead of TST_EXP_PASS_SILENT() or really create SAFE_SWAPON().
But FYI I'm not sure if there would be use for SAFE_SWAPON(), because I changed
this part in the 3rd commit (this is the main change which expects swapon() to
fail):

- /* turn on the swap file */
- TST_EXP_PASS_SILENT(swapon(filename, 0));
+ /* Quit on a first swap file over max, check for EPERM */
+ if (swapon(filename, 0) == -1) {
+ if (errno != EPERM)
+ tst_res(TFAIL | TERRNO, "swapon(%s, 0)", filename);
+ break;
+ }
+ (*swapfiles)++;

and swapon0[12].c swapoff*.c don't need it (they call *MAKE_SWAPFILE*()
functions.

Kind regards,
Petr