Re: [PATCH v3] tst-ttyname: skip the test if failed to become root

["Dmitry V. Levin" <ldv at altlinux dot org>]

On Sat, Dec 30, 2017 at 06:09:22PM -0500, Luke Shumaker wrote:
> Dmitry V. Levin wrote:
> > It would be quite logical for tst-ttyname to return EXIT_UNSUPPORTED
> > as soon as support_become_root failed.
> 
> Indeed; in the original version of the test, whereit called
> support_become_root, it said:
> 
>       support_become_root ();
>     
>       if (unshare (CLONE_NEWNS) < 0)
>         FAIL_UNSUPPORTED ("could not enter new mount namespace");
> 
> Where, that call to `unshare (CLONE_NEWNS)` is guaranteed to to fail
> if `support_become_root ()` failed; it would have been pointless to
> check for failure twice in the same spot by directly checking
> support_become_root.
> 
> However, when Florian cleaned up the test, the call to
> support_become_root got moved to be much earlier in the test; my above
> assumtion that we would immediately be doing something that would
> implicitly check the success of support_become_root became invalid.
> Of course, I didn't realize that when I said "LGTM" To Florian's
> patch.
> 
> If the only problem with the test is that support_become_root fails,
> then we can count on the later
> 
>       if (!support_enter_mount_namespace ())
> 	FAIL_UNSUPPORTED ("could not enter new mount namespace");
> 
> causing us to exit with EXIT_UNSUPPORTED.  But, checking the result of
> support_become_root allows us to exit earlier; saving pointless work.
> 
> On Tue, 26 Dec 2017 09:10:02 -0500,
> Dmitry V. Levin wrote:
> > diff --git a/sysdeps/unix/sysv/linux/tst-ttyname.c b/sysdeps/unix/sysv/linux/tst-ttyname.c
> > index 0fdf1a8..3943403 100644
> > --- a/sysdeps/unix/sysv/linux/tst-ttyname.c
> > +++ b/sysdeps/unix/sysv/linux/tst-ttyname.c
> > @@ -554,7 +554,8 @@ run_chroot_tests (const char *slavename, int slave)
> >  static int
> >  do_test (void)
> >  {
> > -  support_become_root ();
> > +  if (!support_become_root ())
> > +    return EXIT_UNSUPPORTED;
> >  
> >    int ret1 = do_in_chroot_1 (run_chroot_tests);
> >    if (ret1 == EXIT_UNSUPPORTED)
> 
> So, with the above in mind, I do think this patch should be applied.
> 
> But, I do agree with Florian when he said
> 
> Florian Weimer wrote:
> > I don't think the glibc test suite is supposed to pass in such an
> > environment [without ptmx].  If you don't provide /dev/null, /sys,
> > or /proc to the tests, some of them will fail as well.  I still
> > think that the current test accurately reflects the inadequacy of
> > your test environment.
> 
> So I don't nescessarily think that this should be backported to the
> 2.26 stable branch.

If a backported test introduced a regression in the test suite, then a fix
has to be backported as well, I don't think anybody argues with that.

This is exactly the case - tst-ttyname was backported to the stable branch
and caused a regression in the test suite there.  One cannot say that
environments without ptmx are not supported by 2.26 - the fact is that
they were definitely supported at 2.26 release time.

Also, one cannot declare these environments unsupported in master branch
without reaching a consensus.  tst-ttyname was not purposefully made to
fail when ptmx is not available - it's just an omission that has to be
fixed.


-- 
ldv

Attachment: signature.asc
Description: PGP signature