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

[Petr Vorel <pvorel@suse.cz>]

...
> > -       /*create and turn on remaining swapfiles */
> > -       for (j = 0; j < swapfiles; j++) {
> > +       min_swapfiles = MIN_SWAP_FILES - used_swapfiles;


> I can assume a potential issue here is: if a test system already
> contains swapfiles more than MIN_SWAP_FILES, here min_swapfile
> will be a negative value.

> It sounds weird to mount a negative number of file for test.

> What about:

> min_swapfiles = MIN_SWAP_FILES > used_swapfiles ? \
>               (MIN_SWAP_FILES - used_swapfiles) : 0;

min_swapfiles is used only in comparison

if (errno == EPERM && swapfiles > min_swapfiles)

Therefore it's ok:

Due current limitation of requiring at least single swap, this later call will
always contain 1:

tst_res(TINFO, "Successfully created %d swap files", swapfiles);

I'll try to remove this limitation, therefore I'll print this only if
meaningful:

if (swapfiles > 0)
	tst_res(TINFO, "Successfully created %d swap files", swapfiles);
else
	tst_res(TINFO, "No swap file created");

> > +       while (true) {


> There is another issue in the infinite loop, if a kernel bug makes more
> swapfile does not return EPERM but any others, here not report failure
> and only keep looping forever.

Or it can endup like:
swapon03.c:58: TFAIL: swapon(mntpoint/testswap116, 0): EPERM (1)
swapon03.c:51: TINFO: create a swapfile size of 1 megabytes (MB)
swapon03.c:51: TCONF: Insufficient disk space to create swap file
swapon03.c:79: TWARN: Failed to swapoff mntpoint/testswap01
swapon03.c:79: TWARN: Failed to swapoff mntpoint/testswap02
swapon03.c:79: TWARN: Failed to swapoff mntpoint/testswap03
swapon03.c:79: TWARN: Failed to swapoff mntpoint/testswap04
swapon03.c:79: TWARN: Failed to swapoff mntpoint/testswap05

(again bugs in cleanup).

Good point. I was thinking that Cyril's suggestion does not have break and
intend to add tst_brk(), but in the end I forget on it.

> Maybe we should set a uplimit (e.g MAX_TRIES) to avoid that happening.

tst_brk() is simpler than MAX_TRIES therefore I'd prefer it. But it skips
testing for following filesystems.


>                 /* Create the swapfile */
> > -               snprintf(filename, sizeof(filename), "%s%02d", TEST_FILE,
> > j + 2);
> > -               SAFE_MAKE_SMALL_SWAPFILE(filename);
> > +               snprintf(filename, sizeof(filename), "%s%02d", TEST_FILE,
> > swapfiles);
> > +               MAKE_SMALL_SWAPFILE(filename);
> > +
> > +               /* Quit on a first swap file over max, check for EPERM */
> > +               if (swapon(filename, 0) == -1) {
> > +                       if (errno == EPERM && swapfiles > min_swapfiles)
> > +                               break;

...
> >  static int check_and_swapoff(const char *filename)
> >  {
> > -       char cmd_buffer[256];
> > +       char cmd_buffer[FILENAME_MAX+28];
> >         int rc = -1;


> Here we'd better initialize 'rc = 0' though the return value is not used
> anywhere.

I'd prefer to postpone cleanup like this to later. Otherwise we did not manage
to get this to LTP release :(.

Kind regards,
Petr

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