Message ID | 20171226141002.GA23091@altlinux.org |
---|---|
State | New |
Headers | show |
Series | [v3] tst-ttyname: skip the test if failed to become root | expand |
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.
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)