[LTP] [PATCH v2 1/2] OVL_MNT: add setup_overlay helper

[Amir Goldstein]

On Fri, May 24, 2019 at 11:54 AM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi Murphy,
>
> > > > + ret = mount("overlay", OVL_MNT, "overlay", 0, "lowerdir="OVL_LOWER
> > > > +             ",upperdir="OVL_UPPER",workdir="OVL_WORK);
> > > > + if (ret < 0) {
> > > > +         if (errno == ENODEV) {
> > > > +                 tst_res(TINFO,
> > > > +                         "overlayfs is not configured in this kernel.");
> > > > +                 return 1;
> > > First I thought we'd implement it as a test flag (member of struct tst_test).
> > > When we have it as separate function I wonder whether we could TCONF on ENODEV
> > > instead of TINFO and return. Maybe there could be here for int strict parameter,
>
> > The return value is referenced in the testcase to determine whether to run
> > tests in overlayfs. It's needed.
>
> > If this strict parameter is only for different wording on NODEV. Is it
> > necessary ?
>
> I'll recap my suggestions:
> 1) I like having macros to help reduce some parameters, this does not block
> functions being flexible (which requires parameters).
> 2) Having helper function create_overlay_dirs() used separately, than parameter
> in single function (Amir [1] suggestion makes sense).
>
> Something like this, just a suggestion:
>
> int create_overlay_dirs()
> {
>         SAFE_MKDIR(OVL_LOWER, 0755);
>         ...
>         return ret;
> }
>
> int mount_overlay(const char *file, const int lineno, int safe)
> {
>         ...
>         if (create_overlay_dirs())
>                 tst_brk(TCONF, "...");
>
>         ret = mount("overlay", OVL_MNT, "overlay", 0, "lowerdir="OVL_LOWER
>                     ",upperdir="OVL_UPPER",workdir="OVL_WORK);
>         if (ret < 0) {
>                 if (errno == ENODEV) {
>                         /*
>                          * TODO: maybe safe is confusing as we use tst_brk(TBROK anyway),
>                          * + sometimes tst_res(TCONF, ..) would be preferred over
>                          * tst_brk(TCONF, ...)
>                          */
>                         if (safe)
>                                 tst_brk(TCONF,
>                                         "overlayfs is not configured in this kernel.");
>                         } else {
>                                 tst_res(TINFO,
>                                         "overlayfs is not configured in this kernel.");
>                                 return 1;
>                         }
>                 }
>                 tst_brk(TBROK | TERRNO, "overlayfs mount failed");
>         }
> }
>
> #define SAFE_MOUNT_OVERLAY() \
>         mount_overlay(__FILE__, __LINE__, 1);
>
> #define TST_MOUNT_OVERLAY() \
>         mount_overlay(__FILE__, __LINE__, 0);
>

I like this version of the helpers/macros.

I would change TST_MOUNT_OVERLAY to
(mount_overlay(__FILE__, __LINE__, 0) == 0)
so that it could be used like this:

ovl_mounted = TST_MOUNT_OVERLAY(...)

Uses of SAFE_MOUNT_OVERLAY() should not check return value
could even place (void) in front of mount_overlay to enforce that.

Test that use SAFE_MOUNT_OVERLAY() should either have no
variable ovl_mounted or set it after SAFE_MOUNT_OVERLAY() without
checking return value.

Thanks,
Amir.