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

  * sysdeps/unix/sysv/linux/tst-ttyname.c (do_test): Skip the test
if support_become_root failed.
---
 ChangeLog                             | 5 +++++
 sysdeps/unix/sysv/linux/tst-ttyname.c | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)
  

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.
  

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.
  

On Sat, 30 Dec 2017 19:11:52 -0500,
Dmitry V. Levin wrote:
> On Sat, Dec 30, 2017 at 06:09:22PM -0500, Luke Shumaker wrote:
> > 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.
>
> Also, one cannot declare these environments unsupported in master branch
> without reaching a consensus.

On Fri, 29 Dec 2017 08:55:17 -0500,
Florian Weimer wrote:
> * Dmitry V. Levin:
> > It used to work perfectly for at least 15 years, and it still works
> > when tst-ttyname is fixed.
> 
> This just reflects that we did not have sufficient test coverage for
> the PTY code.

Obviously, I'm the outsider here; and can't authoritatively comment on
whether glibc supports environments without ptmx / UNIX 98 PTYs, but
from looking over the codebase:

 1. There is insufficient test coverate of posix_openpt and friends to
    conclude that Linux environments without ptmx were previously
    supported.

    The only other tests that call posix_openpt or other PTY-opening
    functions are

     - debug/tst-chk1.c
     - login/tst-grantpt.c
     - login/tst-ptsname.c
    
    I don't have a good idea of what tst-chk1 does, or why it doesn't
    error if posix_openpt fails; but the other 2 ignore an error
    because "maybe the system does not have SUS pseudo terminals", and
    the posix_openpt bit is just one subtest case.

    Which is to say that posix_openpt is previously untested by the
    test suite.

    But, Linux does support SUS / UNIX 98 pseudo terminals; and
    tst-ttyname lives in sysdeps/unix/sysv/linux.  Linux has no config
    option to disable ptmx.  The only variable there is whatever sets
    up /dev.

 2. The Linux implementation of getpt (a GNU extension) first tries
    posix_openpt to get a UNIX 98 PTY, but if that fails, it falls
    back to trying to get a BSD PTY.  Which suggests that glibc
    supports Linux environments without ptmx.

I find #2 fairly compelling.  That is to say, I've changed my mind,
and do support applying and backporting a patch to EXIT_UNSUPPORTED if
posix_openpt returns ENOENT.