Re: [LTP] [PATCH 2/3] swapon03: Try to swapon() as many files until it fails

[Petr Vorel <pvorel@suse.cz>]

> Hi Petr,
> some comments inline
+1

...
> > -	/* Determine how many more files are to be created */
> > -	max_swapfiles = tst_max_swapfiles();
> >  	used_swapfiles = tst_count_swaps();
> > -	swapfiles = max_swapfiles - used_swapfiles;
> > -	if (swapfiles > max_swapfiles)
> > -		swapfiles = max_swapfiles;
> > +	expected_swapfiles = NUM_SWAP_FILES - used_swapfiles;
> > +
> > +	if (expected_swapfiles < 0) {
> do we want to proceed when expected_swapfiles is 0 here?

Yes, because NUM_SWAP_FILES is 15, which is safe for the changes.
I.e. on my Tumbleweed VM it can be 23 swaps. One is used as filesystem
partition, therefore it would work on even 22.

> > +		tst_brk(TCONF, "Warning: too many used swap files (%d)",
> > +			expected_swapfiles);
> I think this should be used_swapfiles ?

+1.

Also I should remove "Warning: " (left over from previous not-yet-sent
version which used tst_res(TWARN)).

I'd also prefer either to improve grep or replace it with C based search
(better as it does not require grep).

> > +	}

> >  	pid = SAFE_FORK();
> >  	if (pid == 0) {
> > -		/*create and turn on remaining swapfiles */
> > -		for (j = 0; j < swapfiles; j++) {
> > -
> > +		while (true) {
> >  			/* Create the swapfile */
> > -			snprintf(filename, sizeof(filename), "%s%02d", TEST_FILE, j + 2);
> > +			snprintf(filename, sizeof(filename), "%s%02d", TEST_FILE, *swapfiles);
> >  			MAKE_SMALL_SWAPFILE(filename);

> > -			/* turn on the swap file */
> > -			TST_EXP_PASS_SILENT(swapon(filename, 0));
> > +			/* Quit on a first swap file over max */
> > +			if (swapon(filename, 0) == -1)
> so now first swapon() fail is here and then we check for errno==EPERM by
> making another swapon() call in verify_swapon function.
> Is it possible to combine setup_swap() and verify_swapon() and check for
> EPERM on first swapon() fail itself?

I was thinking about it as well. To support -i, we still need to have setup run
only once. Moving code from setup to test function would require to run "setup"
part only once (guard with static variable).

Other option would be to keep setup, but test errno also here (i.e. test it twice).
I'm not sure which one is less awkward.

Kind regards,
Petr

> > +				break;
> > +			(*swapfiles)++;
> >  		}
> >  		exit(0);
> >  	} else {
> >  		waitpid(pid, &status, 0);
> >  	}

> > -	if (WEXITSTATUS(status))
> > +	if (WEXITSTATUS(status) || *swapfiles == 0)
> >  		tst_brk(TBROK, "Failed to setup swap files");
...

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