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

[Petr Vorel <pvorel@suse.cz>]

> 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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp