From: Carlos O'Donell <carlos@redhat.com>
To: Adhemerval Zanella <adhemerval.zanella@linaro.org>,
libc-alpha@sourceware.org
Cc: John David Anglin <danglin@gcc.gnu.org>
Subject: Re: [PATCH] posix: Fix tst-spawn6 terminal handling (BZ #28853)
Date: Wed, 2 Feb 2022 17:23:49 -0500 [thread overview]
Message-ID: <c6accd25-c3ed-9bea-5251-9da7cbd63c37@redhat.com> (raw)
In-Reply-To: <20220202215911.3153490-1-adhemerval.zanella@linaro.org>
On 2/2/22 16:59, Adhemerval Zanella wrote:
> The test changes the current foreground process group, which might
> break testing depending of how the make check is issued. For instance:
>
> nohup make -j1 test t=posix/tst-spawn6 | less
>
> Will set 'make' and 'less' to be in the foreground process group in
> the current session. When tst-spawn6 new child takes over it becomes
> the foreground process and 'less' is stopped and backgrounded which
> interrupts the 'make check' command.
>
> To fix it a pseudo-terminal is allocated, the test starts in new
> session (so there is no controlling terminal associated), and the
> pseudo-terminal is set as the controlling one (similar to what
> login_tty does).
>
> Checked on x86_64-linux-gnu.
OK for glibc 2.35, and this fixes the test issues reported in bug 28853.
Tested on x86_64.
Tested-by: Carlos O'Donell <carlos@redhat.com>
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> ---
> posix/tst-spawn6.c | 62 ++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 51 insertions(+), 11 deletions(-)
>
> diff --git a/posix/tst-spawn6.c b/posix/tst-spawn6.c
> index 911e90a461..044abd8535 100644
> --- a/posix/tst-spawn6.c
> +++ b/posix/tst-spawn6.c
> @@ -29,7 +29,14 @@
> #include <support/check.h>
> #include <support/xunistd.h>
> #include <sys/wait.h>
> +#include <sys/ioctl.h>
> #include <stdlib.h>
> +#include <termios.h>
> +
> +#ifndef PATH_MAX
> +# define PATH_MAX 1024
> +#endif
> +static char ptmxpath[PATH_MAX];
OK.
>
> static int
> handle_restart (const char *argv1, const char *argv2)
> @@ -115,7 +122,7 @@ run_subprogram (int argc, char *argv[], const posix_spawnattr_t *attr,
> }
>
> static int
> -do_test (int argc, char *argv[])
> +run_test (int argc, char *argv[])
> {
> /* We must have either:
> - four parameters left if called initially:
> @@ -127,16 +134,7 @@ do_test (int argc, char *argv[])
> + --setgrpr optional
> */
>
> - if (restart)
> - return handle_restart (argv[1], argv[2]);
> -
> - int tcfd = open64 (_PATH_TTY, O_RDONLY, 0600);
> - if (tcfd == -1)
> - {
> - if (errno == ENXIO)
> - FAIL_UNSUPPORTED ("terminal not available, skipping test");
> - FAIL_EXIT1 ("open64 (\"%s\", 0x%x, 0600): %m", _PATH_TTY, O_RDONLY);
> - }
> + int tcfd = xopen (ptmxpath, O_RDONLY, 0600);
>
> /* Check setting the controlling terminal without changing the group. */
> {
> @@ -198,5 +196,47 @@ do_test (int argc, char *argv[])
> return 0;
> }
>
> +static int
> +do_test (int argc, char *argv[])
> +{
> + if (restart)
> + return handle_restart (argv[1], argv[2]);
> +
OK. In hindsight I think this can all be simplified using forkpty().
> + pid_t pid = xfork ();
> + if (pid == 0)
> + {
> + /* Create a pseudo-terminal to avoid interfering with the one using by
> + test itself, creates a new session (so there is no controlling
> + terminal), and set the pseudo-terminal as the controlling one. */
> + int ptmx = posix_openpt (0);
> + if (ptmx == -1)
> + {
> + if (errno == ENXIO)
> + FAIL_UNSUPPORTED ("terminal not available, skipping test");
> + FAIL_EXIT1 ("posix_openpt (0): %m");
> + }
> + TEST_VERIFY_EXIT (grantpt (ptmx) == 0);
> + TEST_VERIFY_EXIT (unlockpt (ptmx) == 0);
> +
> + TEST_VERIFY_EXIT (setsid () != -1);
> + TEST_VERIFY_EXIT (ioctl (ptmx, TIOCSCTTY, NULL) == 0);
> + while (dup2 (ptmx, STDIN_FILENO) == -1 && errno == EBUSY)
> + ;
> + while (dup2 (ptmx, STDOUT_FILENO) == -1 && errno == EBUSY)
> + ;
> + while (dup2 (ptmx, STDERR_FILENO) == -1 && errno == EBUSY)
> + ;
OK. This connects to the ptm side of the terminal.
> + TEST_VERIFY_EXIT (ptsname_r (ptmx, ptmxpath, sizeof ptmxpath) == 0);
OK. This is OK as-is, but it fills ptmxpath with the pts side of the ptm/pts pair. So we end
up operating on the pts side, but I think that's fine because it's the terminal that matters.
> + xclose (ptmx);
> +
> + run_test (argc, argv);
> + _exit (0);
> + }
> + int status;
> + xwaitpid (pid, &status, 0);
> + TEST_VERIFY (WIFEXITED (status));
> + exit (0);
> +}
> +
> #define TEST_FUNCTION_ARGV do_test
> #include <support/test-driver.c>
--
Cheers,
Carlos.
prev parent reply other threads:[~2022-02-02 22:23 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-02 21:59 Adhemerval Zanella
2022-02-02 22:23 ` Carlos O'Donell [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c6accd25-c3ed-9bea-5251-9da7cbd63c37@redhat.com \
--to=carlos@redhat.com \
--cc=adhemerval.zanella@linaro.org \
--cc=danglin@gcc.gnu.org \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).