On Thu, Mar 12, 2026 at 02:09:30PM -0700, Yosry Ahmed wrote:
> > > 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:
Yes, this looks more tidy than open it. I'll go this helper in v3.
>
> static void check_zswap_enabled(void)
> {
> char value;
read_text() requires string but not char.
>
> 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");
> }
>
--
Regards,
Li Wang
