[LTP] [PATCH v2 1/2] libswap: add two methods to create swapfile

[Li Wang]

On Wed, Mar 20, 2024 at 3:31 PM Petr Vorel <pvorel@suse.cz> wrote:

> Hi Li,
>
> Generally LGTM.
>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>
> ...
> >  /*
> > - * Make a swap file
> > + * Create a swapfile of a specified size or number of blocks.
> >   */
> > -int make_swapfile(const char *swapfile, int blocks, int safe);
> > +int make_swapfile(const char *swapfile, unsigned int num,
> > +                     int safe, enum swapfile_method method);
> I wonder if it would help to add const char *file, const int lineno here.
>
> > +
> > +#define MAKE_SWAPFILE_SIZE(swapfile, size, safe) \
> > +    make_swapfile(swapfile, size, safe, SWAPFILE_BY_SIZE)
> nit: I like the name but one have to search which units (kB vs. MB vs. GB)
> are used.
>

Maybe we can add code comments like:

+/**
+ * Macro to create swapfile size in megabytes (MB)
+ */
 #define MAKE_SWAPFILE_SIZE(swapfile, size, safe) \
    ...

+/**
+ * Macro to create swapfile size in block numbers
+ */
 #define MAKE_SWAPFILE_BLKS(swapfile, blocks, safe) \
      ...


> > +
> > +#define MAKE_SWAPFILE_BLKS(swapfile, blocks, safe) \
> > +    make_swapfile(swapfile, blocks, safe, SWAPFILE_BY_BLKS)
>
> And we could also have SAFE_ variants.
>
> Therefore maybe rename make_swapfile() to make_swapfile_()
> (approach in LTP for functions to be wrapped) and define macros:
>
> int make_swapfile_(const char *file, const int lineno,
>         const char *swapfile, unsigned int num,
>         int safe, enum swapfile_method method);
>
> #define MAKE_SWAPFILE_SIZE(swapfile, size, safe) \
>     make_swapfile_(__FILE__, __LINE__, swapfile, size, 0, SWAPFILE_BY_SIZE)
>
> #define MAKE_SWAPFILE_BLKS(swapfile, blocks, safe) \
>     make_swapfile_(__FILE__, __LINE__, swapfile, blocks, 0,
> SWAPFILE_BY_BLKS)
>
> #define SAFE_MAKE_SWAPFILE_SIZE(swapfile, size, safe) \
>     make_swapfile_(__FILE__, __LINE__, swapfile, size, 1, SWAPFILE_BY_SIZE)
>
> #define SAFE_MAKE_SWAPFILE_BLKS(swapfile, blocks, safe) \
>     make_swapfile_(__FILE__, __LINE__, swapfile, blocks, 1,
> SWAPFILE_BY_BLKS)
>


Good suggestions.


>
> >  /*
> >   * Check swapon/swapoff support status of filesystems or files
> > diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c
> > index a26ea25e4..0e2476ec2 100644
> > --- a/libs/libltpswap/libswap.c
> > +++ b/libs/libltpswap/libswap.c
> > @@ -133,18 +133,26 @@ out:
> >       return contiguous;
> >  }
>
> > -int make_swapfile(const char *swapfile, int blocks, int safe)
> > +int make_swapfile(const char *swapfile, unsigned int num, int safe,
> enum swapfile_method method)
> >  {
> >       struct statvfs fs_info;
> >       unsigned long blk_size, bs;
> >       size_t pg_size = sysconf(_SC_PAGESIZE);
> >       char mnt_path[100];
> > +     unsigned int blocks = 0;
>
> >       if (statvfs(".", &fs_info) == -1)
> >               return -1;
>
> >       blk_size = fs_info.f_bsize;
>
> > +     if (method == SWAPFILE_BY_SIZE)
> > +             blocks = num * 1024 * 1024 / blk_size;
> > +     else if (method == SWAPFILE_BY_BLKS)
> > +             blocks = num;
> > +     else
> > +             tst_brk(TBROK, "Invalid method, please see
> include/libswap.h");
>
> nit: I would print the method.
>

+1


> Using const char *file, const int lineno and tst_brk_() would help
> later to point out which file actually contains wrong method.
>

Yes, that should be better. Thanks!


-- 
Regards,
Li Wang