Re: [PATCH v2] selftests/vm: fix errno handling in mrelease_test

[Adam Sindelar <adam@wowsignal.io>]

On Tue, Jul 05, 2022 at 10:02:08AM -0700, Suren Baghdasaryan wrote:
> On Tue, Jul 5, 2022 at 9:41 AM David Vernet <void@manifault.com> wrote:
> >
> > On Mon, Jul 04, 2022 at 07:33:51PM +0200, Adam Sindelar wrote:
> >
> > Thanks for fixing this, Adam.
> >
> > > mrelease_test should return KSFT_SKIP when process_mrelease is not
> > > defined, but due to a perror call consuming the errno, it returns
> > > KSFT_FAIL.
> > >
> > > This patch decides the exit code before calling perror.
> > >
> >
> > We should probably also include a "Fixes" line here (see [0]):
> >
> > Fixes: 33776141b812 ("selftests: vm: add process_mrelease tests")
> >
> > [0]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
> >
> > > Signed-off-by: Adam Sindelar <adam@wowsignal.io>
> > > ---
> > > v1->v2: Fixed second instance in the same file
> > >
> > >  tools/testing/selftests/vm/mrelease_test.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/vm/mrelease_test.c b/tools/testing/selftests/vm/mrelease_test.c
> > > index 96671c2f7d48..e8b17258579b 100644
> > > --- a/tools/testing/selftests/vm/mrelease_test.c
> > > +++ b/tools/testing/selftests/vm/mrelease_test.c
> > > @@ -100,8 +100,10 @@ int main(void)
> > >
> > >       /* Test a wrong pidfd */
> > >       if (!syscall(__NR_process_mrelease, -1, 0) || errno != EBADF) {
> > > +             /* perror overwrites errno, so this line must be first */
> > > +             res = (errno == ENOSYS ? KSFT_SKIP : KSFT_FAIL);
> > >               perror("process_mrelease with wrong pidfd");
> > > -             exit(errno == ENOSYS ? KSFT_SKIP : KSFT_FAIL);
> > > +             exit(res);
> > >       }
> > >
> > >       /* Start the test with 1MB child memory allocation */
> > > @@ -156,8 +158,9 @@ int main(void)
> > >       run_negative_tests(pidfd);
> > >
> > >       if (kill(pid, SIGKILL)) {
> > > +             res = (errno == ENOSYS ? KSFT_SKIP : KSFT_FAIL);
> > >               perror("kill");
> > > -             exit(errno == ENOSYS ? KSFT_SKIP : KSFT_FAIL);
> > > +             exit(res);
> > >       }
> > >
> > >       success = (syscall(__NR_process_mrelease, pidfd, 0) == 0);
> > > --
> > > 2.35.1
> > >
> >
> > This looks good to me, but it looks like there are a couple of other places
> > where we're doing the wrong thing, i.e. in run_negative_tests() and after
> > calling waitpid(). Could you please fix those as well?
> >
> > Also adding Suren to cc.
> 
> Thanks for adding me, David!
> 
> The fixes look good but there are 5 places in all this fix is needed:
> https://elixir.bootlin.com/linux/v5.19-rc5/source/tools/testing/selftests/vm/mrelease_test.c#L68
> https://elixir.bootlin.com/linux/v5.19-rc5/source/tools/testing/selftests/vm/mrelease_test.c#L76
> https://elixir.bootlin.com/linux/v5.19-rc5/source/tools/testing/selftests/vm/mrelease_test.c#L103
> https://elixir.bootlin.com/linux/v5.19-rc5/source/tools/testing/selftests/vm/mrelease_test.c#L159
> https://elixir.bootlin.com/linux/v5.19-rc5/source/tools/testing/selftests/vm/mrelease_test.c#L175
> Thanks for catching this, Adam!
> Suren.
> 
> >
> > - David

Ah, well spotted. I've fixed the remaining instances in v3.

-Adam