Re: [PATCH v2 1/7] selftests/cgroup: skip test_zswap if zswap is globally disabled

[Yosry Ahmed <yosry@kernel.org>]

> > diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testing/selftests/cgroup/test_zswap.c
> > index 64ebc3f3f203..ec64daaa2f5a 100644
> > --- a/tools/testing/selftests/cgroup/test_zswap.c
> > +++ b/tools/testing/selftests/cgroup/test_zswap.c
> > @@ -589,23 +589,41 @@ struct zswap_test {
> >  };
> >  #undef T
> >
> > -static bool zswap_configured(void)
> > +static int zswap_enabled(void)
> >  {
> > -       return access("/sys/module/zswap", F_OK) == 0;
> > +       char buf[16];
> > +       ssize_t n;
> > +
> > +       if (access("/sys/module/zswap", F_OK))
> > +               ksft_exit_skip("zswap isn't configured\n");
> > +
> > +       n = read_text("/sys/module/zswap/parameters/enabled", buf, sizeof(buf));
> > +       if (n <= 0)
> > +               return -1;
> > +
> > +       if (buf[0] == 'Y')
> > +               return 1;
> > +       else if (buf[0] == 'N')
> > +               return 0;
> > +
> > +       return -1;
> >  }
> >
> >  int main(int argc, char **argv)
> >  {
> >         char root[PATH_MAX];
> > -       int i;
> > +       int i, state;
> >
> >         ksft_print_header();
> >         ksft_set_plan(ARRAY_SIZE(tests));
> >         if (cg_find_unified_root(root, sizeof(root), NULL))
> >                 ksft_exit_skip("cgroup v2 isn't mounted\n");
> >
> > -       if (!zswap_configured())
> > -               ksft_exit_skip("zswap isn't configured\n");
> > +       state = zswap_enabled();
> > +       if (state == 0)
> > +               ksft_exit_skip("zswap is disabled (hint: echo 1 > /sys/module/zswap/parameters/enabled)\n");
> > +       else if (state < 0)
> > +               ksft_exit_fail_msg("Failed to read zswap state\n");
> >
> >         /*
> >          * Check that memory controller is available:
> > --
> > 2.53.0
> >
>
> Seems a bit convoluted. You ksft_exit_skip() for one case in
> zswap_enabled(), but return some value in other cases. This value is
> checked in the caller (main()) and then used to decide whether to skip
> or fail as well?
>
> Why don't you just consolidate them in one place. If zswap_enabled()
> has no other callers, let's just open code it, yeah?

Hmm I actually like having the helper. What if we move all the
ksft_exit_skip() and ksft_exit_fail_msg() calls inside and rename it
to check_zswap_enabled()? We can also just read one character:

static void check_zswap_enabled(void)
{
       char value;

       if (access("/sys/module/zswap", F_OK))
               ksft_exit_skip("zswap isn't configured\n");

       if (read_text("/sys/module/zswap/parameters/enabled", value,
sizeof(value)) <= 0)
               ksft_exit_fail_msg("Failed to read
/sys/module/zswap/parameters/enabled\n");

       if (value == 'N')
               ksft_exit_skip("zswap is disabled (hint: echo 1 >
/sys/module/zswap/parameters/enabled)\n");
}