[LTP] [PATCH v5 2/4] swapon/libswapon: add helper is_swap_supported

[Murphy Zhou]

On Tue, May 28, 2019 at 08:56:06AM +0300, Amir Goldstein wrote:
> On Tue, May 28, 2019 at 7:40 AM Murphy Zhou <xzhou@redhat.com> wrote:
> >
> > To check if the filesystem we are testing on supports swapon/swapoff
> > operations. Keep NFS and TMPFS on the white list. Don't report fail
> > if BTRFS fails with EINVAL.
> 
> Changes look very good, but I don't think you need the whitelist anymore...
> 
> >
> > Signed-off-by: Murphy Zhou <xzhou@redhat.com>
> > ---
> >  testcases/kernel/syscalls/swapon/libswapon.c | 56 ++++++++++++++++++++
> >  testcases/kernel/syscalls/swapon/libswapon.h |  6 +++
> >  2 files changed, 62 insertions(+)
> >
> > diff --git a/testcases/kernel/syscalls/swapon/libswapon.c b/testcases/kernel/syscalls/swapon/libswapon.c
> > index cf6a98891..e02fdd4ad 100644
> > --- a/testcases/kernel/syscalls/swapon/libswapon.c
> > +++ b/testcases/kernel/syscalls/swapon/libswapon.c
> > @@ -19,6 +19,8 @@
> >   *
> >   */
> >
> > +#include <errno.h>
> > +#include "lapi/syscalls.h"
> >  #include "test.h"
> >  #include "libswapon.h"
> >
> > @@ -47,3 +49,57 @@ void make_swapfile(void (cleanup)(void), const char *swapfile)
> >
> >         tst_run_cmd(cleanup, argv, "/dev/null", "/dev/null", 0);
> >  }
> > +
> > +/*
> > + * Check swapon/swapoff support status of filesystems or files
> > + * we are testing on.
> > + */
> > +void is_swap_supported(void (cleanup)(void), const char *ops,
> > +                               const char *filename)
> > +{
> > +       int fibmap = tst_fibmap(filename);
> > +       long fs_type = tst_fs_type(cleanup, filename);
> > +       const char *fstype = tst_fs_type_name(fs_type);
> > +
> > +       /* whitelist legacy fs */
> > +       switch (fs_type) {
> > +       case TST_NFS_MAGIC:
> > +       case TST_TMPFS_MAGIC:
> > +               tst_brkm(TCONF, cleanup,
> > +                        "Cannot do %s on a file on %s filesystem",
> > +                        ops, fstype);
> > +       break;
> > +       }
> 
> If you remove this whitelist, then NFS,tmpfs will reach the fiemap != 0 case
> and result in tst_brkm(TCONF anyway.
> 
> > +
> > +       make_swapfile(NULL, filename);
> > +
> > +       TEST(ltp_syscall(__NR_swapon, filename, 0));
> > +
> > +       if (TEST_RETURN == -1) {
> > +               if (fs_type == TST_BTRFS_MAGIC && errno == EINVAL) {
> 
> If you replace (fs_type == TST_BTRFS_MAGIC) with (fibmap != 0)
> then NFS swapfile support could be tested as well and you do not
> special case any filesystem.

Very good idea!

> 
> 
> > +                       tst_brkm(TCONF, cleanup,
> > +                               "Swapfile on BTRFS not implemented");
> > +               } else {
> > +                       if (fibmap == 0) {
> 
> and then you don't need this extra test.

Yes.

> 
> > +                               tst_brkm(TFAIL | TERRNO, cleanup,
> > +                                        "swapon on %s failed", fstype);
> > +                       } else {
> > +                               tst_brkm(TCONF, cleanup,
> > +                                        "swapon on %s is not supported",
> > +                                               fstype);
> > +                       }
> > +               }
> > +       }
> > +
> > +       TEST(ltp_syscall(__NR_swapoff, filename, 0));
> > +
> > +       if (TEST_RETURN == -1) {
> > +               if (fibmap == 0) {
> > +                       tst_brkm(TFAIL | TERRNO, cleanup,
> > +                               "swapoff on %s failed", fstype);
> > +               } else {
> > +                       tst_brkm(TCONF, cleanup,
> > +                                "swapoff on %s is not supported", fstype);
> > +               }
> 
> I don't think there should be any TCONF here.
> If we reached here then swapon is supported - in that case
> failure to swapoff is a real failure.

Hmm.. make perfect sense. I have felt this test here was a little odd..

Thanks!

I'll tests this more on different filesystems and then post again.

--
Murphy

> 
> Thanks,
> Amir.