* [PATCH V2] utils.c: fix pty setup
@ 2024-04-25 1:47 Changqing Li
2024-04-25 1:56 ` Anibal Limon
2024-04-25 21:59 ` Anibal Limon
0 siblings, 2 replies; 5+ messages in thread
From: Changqing Li @ 2024-04-25 1:47 UTC (permalink / raw)
To: yocto-patches; +Cc: anibal, randy.macleod
From: Changqing Li <changqing.li@windriver.com>
subcase test_tevent_trace will fail if run libtevent-ptest with
ptest-runner, but if run directly, the subcase can successfully.
The failure related to how pty is setup:
In child progress, after close(0), the master pty will use 0 as file
descriptors, and then master pty is attched as child stdin, and slave
pty will also be closed by func close_fds. This will make epoll_wait
will get signal EPOLLHUP, then test failed.
Refer:
epoll_wait(3, [{events=EPOLLOUT|EPOLLHUP, data={u32=2625883648, u64=94496201362944}}], 1, 30000) = 1
lrwx------ 1 root root 64 Apr 23 01:08 0 -> /dev/ptmx
l-wx------ 1 root root 64 Apr 23 01:08 1 -> 'pipe:[4261]'
l-wx------ 1 root root 64 Apr 23 01:08 2 -> 'pipe:[4261]'
To fix this:
1. Open master pty in parent process
2. Attach slave pty to child stdin
3. Remove detaching controlling tty from parent process
4. Remove seting process groupid for child process
Refer:
lrwx------ 1 root root 64 Apr 23 07:35 0 -> /dev/pts/0
l-wx------ 1 root root 64 Apr 23 07:35 1 -> 'pipe:[37001]'
l-wx------ 1 root root 64 Apr 23 07:35 2 -> 'pipe:[37001]'
Signed-off-by: Changqing Li <changqing.li@windriver.com>
---
utils.c | 67 ++++++++++++++++++++++++++++++++-------------------------
1 file changed, 38 insertions(+), 29 deletions(-)
diff --git a/utils.c b/utils.c
index 46918f4..6b3a492 100644
--- a/utils.c
+++ b/utils.c
@@ -341,25 +341,31 @@ run_child(char *run_ptest, int fd_stdout, int fd_stderr)
/* exit(1); not needed? */
}
+/* Returns an integer.
+ * If it returns < 0, an error has occurred.
+ * Otherwise, return 0 means successfully
+ * fp should be writable, likely stdout/err.
+ */
+static int
+setup_master_pty(int* pty_master, int* pty_slave, char*pty_name, FILE *fp) {
+ if (openpty(pty_master, pty_slave, pty_name, NULL, NULL) < 0) {
+ fprintf(fp, "ERROR: openpty() failed with: %s.\n", strerror(errno));
+ return -1;
+ }
+ return 0;
+}
+
/* Returns an integer file descriptor.
* If it returns < 0, an error has occurred.
* Otherwise, it has returned the slave pty file descriptor.
* fp should be writable, likely stdout/err.
*/
static int
-setup_slave_pty(FILE *fp) {
- int pty_master = -1;
- int pty_slave = -1;
- char pty_name[256];
+setup_slave_pty(char* pty_name, FILE *fp) {
struct group *gptr;
gid_t gid;
int slave = -1;
- if (openpty(&pty_master, &pty_slave, pty_name, NULL, NULL) < 0) {
- fprintf(fp, "ERROR: openpty() failed with: %s.\n", strerror(errno));
- return -1;
- }
-
if ((gptr = getgrnam(pty_name)) != 0) {
gid = gptr->gr_gid;
} else {
@@ -384,9 +390,9 @@ setup_slave_pty(FILE *fp) {
if ((slave = open(pty_name, O_RDWR)) == -1) {
fprintf(fp, "ERROR: open() failed with: %s.\n", strerror(errno));
}
- return (slave);
-}
+ return slave;
+}
int
run_ptests(struct ptest_list *head, const struct ptest_options opts,
@@ -406,16 +412,13 @@ run_ptests(struct ptest_list *head, const struct ptest_options opts,
do
{
- if (isatty(0) && ioctl(0, TIOCNOTTY) == -1) {
- fprintf(fp, "ERROR: Unable to detach from controlling tty, %s\n", strerror(errno));
- }
-
-
fprintf(fp, "START: %s\n", progname);
PTEST_LIST_ITERATE_START(head, p)
int pipefd_stdout[2] = {-1, -1};
int pipefd_stderr[2] = {-1, -1};
- int pgid = -1;
+ int pty_master = -1;
+ int pty_slave = -1;
+ char pty_name[256] = {0};
if (pipe2(pipefd_stdout, 0) == -1) {
rc = -1;
@@ -435,8 +438,9 @@ run_ptests(struct ptest_list *head, const struct ptest_options opts,
}
dirname(ptest_dir);
- if ((pgid = getpgid(0)) == -1) {
- fprintf(fp, "ERROR: getpgid() failed, %s\n", strerror(errno));
+ if (setup_master_pty(&pty_master, &pty_slave, pty_name, fp) < 0) {
+ fprintf(fp, "ERROR: setup_master_pty failed, %s\n", strerror(errno));
+ break;
}
pid_t child = fork();
@@ -447,25 +451,32 @@ run_ptests(struct ptest_list *head, const struct ptest_options opts,
} else if (child == 0) {
int slave;
- close(0);
/* Close read ends of the pipe */
do_close(&pipefd_stdout[PIPE_READ]);
do_close(&pipefd_stderr[PIPE_READ]);
+ do_close(&pty_master);
- if ((slave = setup_slave_pty(fp)) < 0) {
+ if ((slave = setup_slave_pty(pty_name, fp)) < 0) {
fprintf(fp, "ERROR: could not setup pty (%d).", slave);
}
- if (setpgid(0,pgid) == -1) {
- fprintf(fp, "ERROR: setpgid() failed, %s\n", strerror(errno));
- }
if (setsid() == -1) {
fprintf(fp, "ERROR: setsid() failed, %s\n", strerror(errno));
}
- if (ioctl(0, TIOCSCTTY, NULL) == -1) {
+ if (ioctl(slave, TIOCSCTTY, NULL) == -1) {
fprintf(fp, "ERROR: Unable to attach to controlling tty, %s\n", strerror(errno));
}
+ if (slave > 2) {
+ close(slave);
+ }
+
+ if (dup2(pty_slave, STDIN_FILENO) < 0) {
+ fprintf(fp, "ERROR: Unable to dup slave pty to stdin, %s\n", strerror(errno));
+ }
+ if (pty_slave > 2) {
+ close(pty_slave);
+ }
if (chdir(ptest_dir) == -1) {
fprintf(fp, "ERROR: Unable to chdir(%s), %s\n", ptest_dir, strerror(errno));
@@ -481,10 +492,6 @@ run_ptests(struct ptest_list *head, const struct ptest_options opts,
do_close(&pipefd_stdout[PIPE_WRITE]);
do_close(&pipefd_stderr[PIPE_WRITE]);
- if (setpgid(child, pgid) == -1) {
- fprintf(fp, "ERROR: setpgid() failed, %s\n", strerror(errno));
- }
-
time_t start_time= time(NULL);
fprintf(fp, "%s\n", get_stime(stime, GET_STIME_BUF_SIZE, start_time));
fprintf(fp, "BEGIN: %s\n", ptest_dir);
@@ -608,6 +615,8 @@ run_ptests(struct ptest_list *head, const struct ptest_options opts,
do_close(&pipefd_stdout[PIPE_WRITE]);
do_close(&pipefd_stderr[PIPE_READ]);
do_close(&pipefd_stderr[PIPE_WRITE]);
+ do_close(&pty_master);
+ do_close(&pty_slave);
fflush(fp);
fflush(fp_stderr);
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH V2] utils.c: fix pty setup
2024-04-25 1:47 [PATCH V2] utils.c: fix pty setup Changqing Li
@ 2024-04-25 1:56 ` Anibal Limon
2024-04-25 21:59 ` Anibal Limon
1 sibling, 0 replies; 5+ messages in thread
From: Anibal Limon @ 2024-04-25 1:56 UTC (permalink / raw)
To: changqing.li; +Cc: yocto-patches, randy.macleod
[-- Attachment #1: Type: text/plain, Size: 8522 bytes --]
Hi Changqing,
I will review it, thanks for the patch!,
Regards!.
Anibal
On Wed, Apr 24, 2024 at 7:47 PM <changqing.li@windriver.com> wrote:
> From: Changqing Li <changqing.li@windriver.com>
>
> subcase test_tevent_trace will fail if run libtevent-ptest with
> ptest-runner, but if run directly, the subcase can successfully.
>
> The failure related to how pty is setup:
> In child progress, after close(0), the master pty will use 0 as file
> descriptors, and then master pty is attched as child stdin, and slave
> pty will also be closed by func close_fds. This will make epoll_wait
> will get signal EPOLLHUP, then test failed.
>
> Refer:
> epoll_wait(3, [{events=EPOLLOUT|EPOLLHUP, data={u32=2625883648,
> u64=94496201362944}}], 1, 30000) = 1
>
> lrwx------ 1 root root 64 Apr 23 01:08 0 -> /dev/ptmx
> l-wx------ 1 root root 64 Apr 23 01:08 1 -> 'pipe:[4261]'
> l-wx------ 1 root root 64 Apr 23 01:08 2 -> 'pipe:[4261]'
>
> To fix this:
> 1. Open master pty in parent process
> 2. Attach slave pty to child stdin
> 3. Remove detaching controlling tty from parent process
> 4. Remove seting process groupid for child process
>
> Refer:
> lrwx------ 1 root root 64 Apr 23 07:35 0 -> /dev/pts/0
> l-wx------ 1 root root 64 Apr 23 07:35 1 -> 'pipe:[37001]'
> l-wx------ 1 root root 64 Apr 23 07:35 2 -> 'pipe:[37001]'
>
> Signed-off-by: Changqing Li <changqing.li@windriver.com>
> ---
> utils.c | 67 ++++++++++++++++++++++++++++++++-------------------------
> 1 file changed, 38 insertions(+), 29 deletions(-)
>
> diff --git a/utils.c b/utils.c
> index 46918f4..6b3a492 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -341,25 +341,31 @@ run_child(char *run_ptest, int fd_stdout, int
> fd_stderr)
> /* exit(1); not needed? */
> }
>
> +/* Returns an integer.
> + * If it returns < 0, an error has occurred.
> + * Otherwise, return 0 means successfully
> + * fp should be writable, likely stdout/err.
> + */
> +static int
> +setup_master_pty(int* pty_master, int* pty_slave, char*pty_name, FILE
> *fp) {
> + if (openpty(pty_master, pty_slave, pty_name, NULL, NULL) < 0) {
> + fprintf(fp, "ERROR: openpty() failed with: %s.\n",
> strerror(errno));
> + return -1;
> + }
> + return 0;
> +}
> +
> /* Returns an integer file descriptor.
> * If it returns < 0, an error has occurred.
> * Otherwise, it has returned the slave pty file descriptor.
> * fp should be writable, likely stdout/err.
> */
> static int
> -setup_slave_pty(FILE *fp) {
> - int pty_master = -1;
> - int pty_slave = -1;
> - char pty_name[256];
> +setup_slave_pty(char* pty_name, FILE *fp) {
> struct group *gptr;
> gid_t gid;
> int slave = -1;
>
> - if (openpty(&pty_master, &pty_slave, pty_name, NULL, NULL) < 0) {
> - fprintf(fp, "ERROR: openpty() failed with: %s.\n",
> strerror(errno));
> - return -1;
> - }
> -
> if ((gptr = getgrnam(pty_name)) != 0) {
> gid = gptr->gr_gid;
> } else {
> @@ -384,9 +390,9 @@ setup_slave_pty(FILE *fp) {
> if ((slave = open(pty_name, O_RDWR)) == -1) {
> fprintf(fp, "ERROR: open() failed with: %s.\n",
> strerror(errno));
> }
> - return (slave);
> -}
>
> + return slave;
> +}
>
> int
> run_ptests(struct ptest_list *head, const struct ptest_options opts,
> @@ -406,16 +412,13 @@ run_ptests(struct ptest_list *head, const struct
> ptest_options opts,
>
> do
> {
> - if (isatty(0) && ioctl(0, TIOCNOTTY) == -1) {
> - fprintf(fp, "ERROR: Unable to detach from
> controlling tty, %s\n", strerror(errno));
> - }
> -
> -
> fprintf(fp, "START: %s\n", progname);
> PTEST_LIST_ITERATE_START(head, p)
> int pipefd_stdout[2] = {-1, -1};
> int pipefd_stderr[2] = {-1, -1};
> - int pgid = -1;
> + int pty_master = -1;
> + int pty_slave = -1;
> + char pty_name[256] = {0};
>
> if (pipe2(pipefd_stdout, 0) == -1) {
> rc = -1;
> @@ -435,8 +438,9 @@ run_ptests(struct ptest_list *head, const struct
> ptest_options opts,
> }
> dirname(ptest_dir);
>
> - if ((pgid = getpgid(0)) == -1) {
> - fprintf(fp, "ERROR: getpgid() failed,
> %s\n", strerror(errno));
> + if (setup_master_pty(&pty_master, &pty_slave,
> pty_name, fp) < 0) {
> + fprintf(fp, "ERROR: setup_master_pty
> failed, %s\n", strerror(errno));
> + break;
> }
>
> pid_t child = fork();
> @@ -447,25 +451,32 @@ run_ptests(struct ptest_list *head, const struct
> ptest_options opts,
> } else if (child == 0) {
> int slave;
>
> - close(0);
> /* Close read ends of the pipe */
> do_close(&pipefd_stdout[PIPE_READ]);
> do_close(&pipefd_stderr[PIPE_READ]);
> + do_close(&pty_master);
>
> - if ((slave = setup_slave_pty(fp)) < 0) {
> + if ((slave = setup_slave_pty(pty_name,
> fp)) < 0) {
> fprintf(fp, "ERROR: could not
> setup pty (%d).", slave);
> }
> - if (setpgid(0,pgid) == -1) {
> - fprintf(fp, "ERROR: setpgid()
> failed, %s\n", strerror(errno));
> - }
>
> if (setsid() == -1) {
> fprintf(fp, "ERROR: setsid()
> failed, %s\n", strerror(errno));
> }
>
> - if (ioctl(0, TIOCSCTTY, NULL) == -1) {
> + if (ioctl(slave, TIOCSCTTY, NULL) == -1) {
> fprintf(fp, "ERROR: Unable to
> attach to controlling tty, %s\n", strerror(errno));
> }
> + if (slave > 2) {
> + close(slave);
> + }
> +
> + if (dup2(pty_slave, STDIN_FILENO) < 0) {
> + fprintf(fp, "ERROR: Unable to dup
> slave pty to stdin, %s\n", strerror(errno));
> + }
> + if (pty_slave > 2) {
> + close(pty_slave);
> + }
>
> if (chdir(ptest_dir) == -1) {
> fprintf(fp, "ERROR: Unable to
> chdir(%s), %s\n", ptest_dir, strerror(errno));
> @@ -481,10 +492,6 @@ run_ptests(struct ptest_list *head, const struct
> ptest_options opts,
> do_close(&pipefd_stdout[PIPE_WRITE]);
> do_close(&pipefd_stderr[PIPE_WRITE]);
>
> - if (setpgid(child, pgid) == -1) {
> - fprintf(fp, "ERROR: setpgid()
> failed, %s\n", strerror(errno));
> - }
> -
> time_t start_time= time(NULL);
> fprintf(fp, "%s\n", get_stime(stime,
> GET_STIME_BUF_SIZE, start_time));
> fprintf(fp, "BEGIN: %s\n", ptest_dir);
> @@ -608,6 +615,8 @@ run_ptests(struct ptest_list *head, const struct
> ptest_options opts,
> do_close(&pipefd_stdout[PIPE_WRITE]);
> do_close(&pipefd_stderr[PIPE_READ]);
> do_close(&pipefd_stderr[PIPE_WRITE]);
> + do_close(&pty_master);
> + do_close(&pty_slave);
>
> fflush(fp);
> fflush(fp_stderr);
> --
> 2.25.1
>
>
[-- Attachment #2: Type: text/html, Size: 10784 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2] utils.c: fix pty setup
2024-04-25 1:47 [PATCH V2] utils.c: fix pty setup Changqing Li
2024-04-25 1:56 ` Anibal Limon
@ 2024-04-25 21:59 ` Anibal Limon
2024-04-26 1:21 ` Changqing Li
1 sibling, 1 reply; 5+ messages in thread
From: Anibal Limon @ 2024-04-25 21:59 UTC (permalink / raw)
To: changqing.li; +Cc: yocto-patches, randy.macleod
[-- Attachment #1: Type: text/plain, Size: 8748 bytes --]
Comments below,
Thanks!,
Anibal
On Wed, Apr 24, 2024 at 7:47 PM <changqing.li@windriver.com> wrote:
> From: Changqing Li <changqing.li@windriver.com>
>
> subcase test_tevent_trace will fail if run libtevent-ptest with
> ptest-runner, but if run directly, the subcase can successfully.
>
> The failure related to how pty is setup:
> In child progress, after close(0), the master pty will use 0 as file
> descriptors, and then master pty is attched as child stdin, and slave
> pty will also be closed by func close_fds. This will make epoll_wait
> will get signal EPOLLHUP, then test failed.
>
> Refer:
> epoll_wait(3, [{events=EPOLLOUT|EPOLLHUP, data={u32=2625883648,
> u64=94496201362944}}], 1, 30000) = 1
>
> lrwx------ 1 root root 64 Apr 23 01:08 0 -> /dev/ptmx
> l-wx------ 1 root root 64 Apr 23 01:08 1 -> 'pipe:[4261]'
> l-wx------ 1 root root 64 Apr 23 01:08 2 -> 'pipe:[4261]'
>
> To fix this:
> 1. Open master pty in parent process
> 2. Attach slave pty to child stdin
> 3. Remove detaching controlling tty from parent process
> 4. Remove seting process groupid for child process
>
> Refer:
> lrwx------ 1 root root 64 Apr 23 07:35 0 -> /dev/pts/0
> l-wx------ 1 root root 64 Apr 23 07:35 1 -> 'pipe:[37001]'
> l-wx------ 1 root root 64 Apr 23 07:35 2 -> 'pipe:[37001]'
>
> Signed-off-by: Changqing Li <changqing.li@windriver.com>
> ---
> utils.c | 67 ++++++++++++++++++++++++++++++++-------------------------
> 1 file changed, 38 insertions(+), 29 deletions(-)
>
> diff --git a/utils.c b/utils.c
> index 46918f4..6b3a492 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -341,25 +341,31 @@ run_child(char *run_ptest, int fd_stdout, int
> fd_stderr)
> /* exit(1); not needed? */
> }
>
> +/* Returns an integer.
> + * If it returns < 0, an error has occurred.
> + * Otherwise, return 0 means successfully
> + * fp should be writable, likely stdout/err.
> + */
> +static int
> +setup_master_pty(int* pty_master, int* pty_slave, char*pty_name, FILE
> *fp) {
> + if (openpty(pty_master, pty_slave, pty_name, NULL, NULL) < 0) {
> + fprintf(fp, "ERROR: openpty() failed with: %s.\n",
> strerror(errno));
> + return -1;
> + }
> + return 0;
> +}
> +
> /* Returns an integer file descriptor.
> * If it returns < 0, an error has occurred.
> * Otherwise, it has returned the slave pty file descriptor.
> * fp should be writable, likely stdout/err.
> */
> static int
> -setup_slave_pty(FILE *fp) {
> - int pty_master = -1;
> - int pty_slave = -1;
> - char pty_name[256];
> +setup_slave_pty(char* pty_name, FILE *fp) {
> struct group *gptr;
> gid_t gid;
> int slave = -1;
>
> - if (openpty(&pty_master, &pty_slave, pty_name, NULL, NULL) < 0) {
> - fprintf(fp, "ERROR: openpty() failed with: %s.\n",
> strerror(errno));
> - return -1;
> - }
> -
> if ((gptr = getgrnam(pty_name)) != 0) {
> gid = gptr->gr_gid;
> } else {
> @@ -384,9 +390,9 @@ setup_slave_pty(FILE *fp) {
> if ((slave = open(pty_name, O_RDWR)) == -1) {
> fprintf(fp, "ERROR: open() failed with: %s.\n",
> strerror(errno));
> }
> - return (slave);
> -}
>
> + return slave;
> +}
>
> int
> run_ptests(struct ptest_list *head, const struct ptest_options opts,
> @@ -406,16 +412,13 @@ run_ptests(struct ptest_list *head, const struct
> ptest_options opts,
>
> do
> {
> - if (isatty(0) && ioctl(0, TIOCNOTTY) == -1) {
> - fprintf(fp, "ERROR: Unable to detach from
> controlling tty, %s\n", strerror(errno));
> - }
> -
> -
> fprintf(fp, "START: %s\n", progname);
> PTEST_LIST_ITERATE_START(head, p)
> int pipefd_stdout[2] = {-1, -1};
> int pipefd_stderr[2] = {-1, -1};
> - int pgid = -1;
> + int pty_master = -1;
> + int pty_slave = -1;
> + char pty_name[256] = {0};
>
> if (pipe2(pipefd_stdout, 0) == -1) {
> rc = -1;
> @@ -435,8 +438,9 @@ run_ptests(struct ptest_list *head, const struct
> ptest_options opts,
> }
> dirname(ptest_dir);
>
> - if ((pgid = getpgid(0)) == -1) {
> - fprintf(fp, "ERROR: getpgid() failed,
> %s\n", strerror(errno));
>
This comes from Randy patch to ensure child tests are in the same process
group than the ptest-runner, 59381a64. Any specific reason to remove this?.
> + if (setup_master_pty(&pty_master, &pty_slave,
> pty_name, fp) < 0) {
> + fprintf(fp, "ERROR: setup_master_pty
> failed, %s\n", strerror(errno));
> + break;
> }
>
> pid_t child = fork();
> @@ -447,25 +451,32 @@ run_ptests(struct ptest_list *head, const struct
> ptest_options opts,
> } else if (child == 0) {
> int slave;
>
> - close(0);
> /* Close read ends of the pipe */
> do_close(&pipefd_stdout[PIPE_READ]);
> do_close(&pipefd_stderr[PIPE_READ]);
> + do_close(&pty_master);
>
> - if ((slave = setup_slave_pty(fp)) < 0) {
> + if ((slave = setup_slave_pty(pty_name,
> fp)) < 0) {
> fprintf(fp, "ERROR: could not
> setup pty (%d).", slave);
> }
> - if (setpgid(0,pgid) == -1) {
> - fprintf(fp, "ERROR: setpgid()
> failed, %s\n", strerror(errno));
> - }
>
> if (setsid() == -1) {
> fprintf(fp, "ERROR: setsid()
> failed, %s\n", strerror(errno));
> }
>
> - if (ioctl(0, TIOCSCTTY, NULL) == -1) {
> + if (ioctl(slave, TIOCSCTTY, NULL) == -1) {
> fprintf(fp, "ERROR: Unable to
> attach to controlling tty, %s\n", strerror(errno));
> }
+ if (slave > 2) {
> + close(slave);
> + }
> +
> + if (dup2(pty_slave, STDIN_FILENO) < 0) {
> + fprintf(fp, "ERROR: Unable to dup
> slave pty to stdin, %s\n", strerror(errno));
> + }
> + if (pty_slave > 2) {
> + close(pty_slave);
> + }
>
Which is the reason to make it twice?, and also closing with slave > 2.
>
> if (chdir(ptest_dir) == -1) {
> fprintf(fp, "ERROR: Unable to
> chdir(%s), %s\n", ptest_dir, strerror(errno));
> @@ -481,10 +492,6 @@ run_ptests(struct ptest_list *head, const struct
> ptest_options opts,
> do_close(&pipefd_stdout[PIPE_WRITE]);
> do_close(&pipefd_stderr[PIPE_WRITE]);
>
> - if (setpgid(child, pgid) == -1) {
> - fprintf(fp, "ERROR: setpgid()
> failed, %s\n", strerror(errno));
> - }
> -
>
Removal comment up.
> time_t start_time= time(NULL);
> fprintf(fp, "%s\n", get_stime(stime,
> GET_STIME_BUF_SIZE, start_time));
> fprintf(fp, "BEGIN: %s\n", ptest_dir);
> @@ -608,6 +615,8 @@ run_ptests(struct ptest_list *head, const struct
> ptest_options opts,
> do_close(&pipefd_stdout[PIPE_WRITE]);
> do_close(&pipefd_stderr[PIPE_READ]);
> do_close(&pipefd_stderr[PIPE_WRITE]);
> + do_close(&pty_master);
> + do_close(&pty_slave);
>
> fflush(fp);
> fflush(fp_stderr);
> --
> 2.25.1
>
>
[-- Attachment #2: Type: text/html, Size: 11618 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2] utils.c: fix pty setup
2024-04-25 21:59 ` Anibal Limon
@ 2024-04-26 1:21 ` Changqing Li
2024-05-02 22:42 ` Anibal Limon
0 siblings, 1 reply; 5+ messages in thread
From: Changqing Li @ 2024-04-26 1:21 UTC (permalink / raw)
To: Anibal Limon; +Cc: yocto-patches, randy.macleod
[-- Attachment #1: Type: text/plain, Size: 12206 bytes --]
On 4/26/24 05:59, Anibal Limon wrote:
> **
> *CAUTION: This email comes from a non Wind River email account!*
> Do not click links or open attachments unless you recognize the sender
> and know the content is safe.
> Comments below,
>
> Thanks!,
> Anibal
>
> On Wed, Apr 24, 2024 at 7:47 PM <changqing.li@windriver.com> wrote:
>
> From: Changqing Li <changqing.li@windriver.com>
>
> subcase test_tevent_trace will fail if run libtevent-ptest with
> ptest-runner, but if run directly, the subcase can successfully.
>
> The failure related to how pty is setup:
> In child progress, after close(0), the master pty will use 0 as file
> descriptors, and then master pty is attched as child stdin, and slave
> pty will also be closed by func close_fds. This will make epoll_wait
> will get signal EPOLLHUP, then test failed.
>
> Refer:
> epoll_wait(3, [{events=EPOLLOUT|EPOLLHUP, data={u32=2625883648,
> u64=94496201362944}}], 1, 30000) = 1
>
> lrwx------ 1 root root 64 Apr 23 01:08 0 -> /dev/ptmx
> l-wx------ 1 root root 64 Apr 23 01:08 1 -> 'pipe:[4261]'
> l-wx------ 1 root root 64 Apr 23 01:08 2 -> 'pipe:[4261]'
>
> To fix this:
> 1. Open master pty in parent process
> 2. Attach slave pty to child stdin
> 3. Remove detaching controlling tty from parent process
> 4. Remove seting process groupid for child process
>
> Refer:
> lrwx------ 1 root root 64 Apr 23 07:35 0 -> /dev/pts/0
> l-wx------ 1 root root 64 Apr 23 07:35 1 -> 'pipe:[37001]'
> l-wx------ 1 root root 64 Apr 23 07:35 2 -> 'pipe:[37001]'
>
> Signed-off-by: Changqing Li <changqing.li@windriver.com>
> ---
> utils.c | 67
> ++++++++++++++++++++++++++++++++-------------------------
> 1 file changed, 38 insertions(+), 29 deletions(-)
>
> diff --git a/utils.c b/utils.c
> index 46918f4..6b3a492 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -341,25 +341,31 @@ run_child(char *run_ptest, int fd_stdout,
> int fd_stderr)
> /* exit(1); not needed? */
> }
>
> +/* Returns an integer.
> + * If it returns < 0, an error has occurred.
> + * Otherwise, return 0 means successfully
> + * fp should be writable, likely stdout/err.
> + */
> +static int
> +setup_master_pty(int* pty_master, int* pty_slave, char*pty_name,
> FILE *fp) {
> + if (openpty(pty_master, pty_slave, pty_name, NULL, NULL) <
> 0) {
> + fprintf(fp, "ERROR: openpty() failed with: %s.\n",
> strerror(errno));
> + return -1;
> + }
> + return 0;
> +}
> +
> /* Returns an integer file descriptor.
> * If it returns < 0, an error has occurred.
> * Otherwise, it has returned the slave pty file descriptor.
> * fp should be writable, likely stdout/err.
> */
> static int
> -setup_slave_pty(FILE *fp) {
> - int pty_master = -1;
> - int pty_slave = -1;
> - char pty_name[256];
> +setup_slave_pty(char* pty_name, FILE *fp) {
> struct group *gptr;
> gid_t gid;
> int slave = -1;
>
> - if (openpty(&pty_master, &pty_slave, pty_name, NULL, NULL)
> < 0) {
> - fprintf(fp, "ERROR: openpty() failed with: %s.\n",
> strerror(errno));
> - return -1;
> - }
> -
> if ((gptr = getgrnam(pty_name)) != 0) {
> gid = gptr->gr_gid;
> } else {
> @@ -384,9 +390,9 @@ setup_slave_pty(FILE *fp) {
> if ((slave = open(pty_name, O_RDWR)) == -1) {
> fprintf(fp, "ERROR: open() failed with: %s.\n",
> strerror(errno));
> }
> - return (slave);
> -}
>
> + return slave;
> +}
>
> int
> run_ptests(struct ptest_list *head, const struct ptest_options opts,
> @@ -406,16 +412,13 @@ run_ptests(struct ptest_list *head, const
> struct ptest_options opts,
>
> do
> {
> - if (isatty(0) && ioctl(0, TIOCNOTTY) == -1) {
> - fprintf(fp, "ERROR: Unable to detach from
> controlling tty, %s\n", strerror(errno));
> - }
> -
> -
> fprintf(fp, "START: %s\n", progname);
> PTEST_LIST_ITERATE_START(head, p)
> int pipefd_stdout[2] = {-1, -1};
> int pipefd_stderr[2] = {-1, -1};
> - int pgid = -1;
> + int pty_master = -1;
> + int pty_slave = -1;
> + char pty_name[256] = {0};
>
> if (pipe2(pipefd_stdout, 0) == -1) {
> rc = -1;
> @@ -435,8 +438,9 @@ run_ptests(struct ptest_list *head, const
> struct ptest_options opts,
> }
> dirname(ptest_dir);
>
> - if ((pgid = getpgid(0)) == -1) {
> - fprintf(fp, "ERROR: getpgid()
> failed, %s\n", strerror(errno));
>
>
> This comes from Randy patch to ensure child tests are in the same
> process group than the ptest-runner, 59381a64. Any specific reason to
> remove this?.
See blow comments where pgid is used.
>
> + if (setup_master_pty(&pty_master,
> &pty_slave, pty_name, fp) < 0) {
> + fprintf(fp, "ERROR:
> setup_master_pty failed, %s\n", strerror(errno));
> + break;
> }
>
> pid_t child = fork();
> @@ -447,25 +451,32 @@ run_ptests(struct ptest_list *head, const
> struct ptest_options opts,
> } else if (child == 0) {
> int slave;
>
> - close(0);
> /* Close read ends of the pipe */
> do_close(&pipefd_stdout[PIPE_READ]);
> do_close(&pipefd_stderr[PIPE_READ]);
> + do_close(&pty_master);
>
> - if ((slave = setup_slave_pty(fp))
> < 0) {
> + if ((slave =
> setup_slave_pty(pty_name, fp)) < 0) {
> fprintf(fp, "ERROR: could
> not setup pty (%d).", slave);
> }
> - if (setpgid(0,pgid) == -1) {
> - fprintf(fp, "ERROR:
> setpgid() failed, %s\n", strerror(errno));
> - }
>
The pgid is used to set child process to join the same progress group as
parent.
But next line, we will make child as session leader, then the child
will also be in the new progress group.
So above setpgid actually not need. Here is the process stat with
ptest-runner(without this patch):
root@qemux86-64:~# cat /proc/455/stat
455 (ptest-runner) S 422 *455* 283 0 -1 4194304 109 0 0 0 0 2 0 0 20 0 1
0 62009 2441216 352 18446744073709551615 94258154364928 94258154373585
140722320232848 0 0 0 0 0 0 1 0 0 17 0 0 0 0 0 0 94258154384384
94258154385424 94258154389504 140722320240155 140722320240178
140722320240178 140722320240610 0
root@qemux86-64:~# cat /proc/456/stat
456 (run-ptest) S 455 *456* 456 34817 456 4194560 242 0 0 0 0 0 0 0 20 0
1 0 62065 3813376 640 18446744073709551615 94612400209920 94612400903225
140724672843248 0 0 0 65536 4 65538 1 0 0 17 1 0 0 0 0 0 94612401150672
94612401198372 94612401246208 140724672851446 140724672851491
140724672851491 140724672851923 0
root@qemux86-64:~#
We can see, although setpgid, but finally, we pgid of child will be
the child pid.
>
> if (setsid() == -1) {
> fprintf(fp, "ERROR:
> setsid() failed, %s\n", strerror(errno));
> }
>
> - if (ioctl(0, TIOCSCTTY, NULL) == -1) {
> + if (ioctl(slave, TIOCSCTTY, NULL)
> == -1) {
> fprintf(fp, "ERROR: Unable
> to attach to controlling tty, %s\n", strerror(errno));
> }
>
> + if (slave > 2) {
> + close(slave);
> + }
> +
> + if (dup2(pty_slave, STDIN_FILENO)
> < 0) {
> + fprintf(fp, "ERROR: Unable
> to dup slave pty to stdin, %s\n", strerror(errno));
> + }
> + if (pty_slave > 2) {
> + close(pty_slave);
> + }
>
> Which is the reason to make it twice?, and also closing with slave > 2.
first, according to current code logic, the slave/pty_slave should
always > 2, the if checking just for ensure it is.
pty_slave and slave are point to slave pty, but they have different
fd. So I need to close them both after dup slave pty to stdin.
>
> if (chdir(ptest_dir) == -1) {
> fprintf(fp, "ERROR: Unable
> to chdir(%s), %s\n", ptest_dir, strerror(errno));
> @@ -481,10 +492,6 @@ run_ptests(struct ptest_list *head, const
> struct ptest_options opts,
> do_close(&pipefd_stdout[PIPE_WRITE]);
> do_close(&pipefd_stderr[PIPE_WRITE]);
>
> - if (setpgid(child, pgid) == -1) {
> - fprintf(fp, "ERROR:
> setpgid() failed, %s\n", strerror(errno));
> - }
> -
>
> Removal comment up.
Same as what I comments above, child is in another session, so this is
not needed.
Regards
Changqing
> time_t start_time= time(NULL);
> fprintf(fp, "%s\n",
> get_stime(stime, GET_STIME_BUF_SIZE, start_time));
> fprintf(fp, "BEGIN: %s\n", ptest_dir);
> @@ -608,6 +615,8 @@ run_ptests(struct ptest_list *head, const
> struct ptest_options opts,
> do_close(&pipefd_stdout[PIPE_WRITE]);
> do_close(&pipefd_stderr[PIPE_READ]);
> do_close(&pipefd_stderr[PIPE_WRITE]);
> + do_close(&pty_master);
> + do_close(&pty_slave);
>
> fflush(fp);
> fflush(fp_stderr);
> --
> 2.25.1
>
[-- Attachment #2: Type: text/html, Size: 24520 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2] utils.c: fix pty setup
2024-04-26 1:21 ` Changqing Li
@ 2024-05-02 22:42 ` Anibal Limon
0 siblings, 0 replies; 5+ messages in thread
From: Anibal Limon @ 2024-05-02 22:42 UTC (permalink / raw)
To: Changqing Li; +Cc: yocto-patches, randy.macleod
[-- Attachment #1: Type: text/plain, Size: 11076 bytes --]
Hi Changqing,
I ran the test suite and it looks good so far.
I will run more tests before pushing this + minor changes may be.
Thanks!,
Anibal
On Thu, Apr 25, 2024 at 7:22 PM Changqing Li <changqing.li@windriver.com>
wrote:
>
> On 4/26/24 05:59, Anibal Limon wrote:
>
> *CAUTION: This email comes from a non Wind River email account!*
> Do not click links or open attachments unless you recognize the sender and
> know the content is safe.
> Comments below,
>
> Thanks!,
> Anibal
>
> On Wed, Apr 24, 2024 at 7:47 PM <changqing.li@windriver.com> wrote:
>
>> From: Changqing Li <changqing.li@windriver.com>
>>
>> subcase test_tevent_trace will fail if run libtevent-ptest with
>> ptest-runner, but if run directly, the subcase can successfully.
>>
>> The failure related to how pty is setup:
>> In child progress, after close(0), the master pty will use 0 as file
>> descriptors, and then master pty is attched as child stdin, and slave
>> pty will also be closed by func close_fds. This will make epoll_wait
>> will get signal EPOLLHUP, then test failed.
>>
>> Refer:
>> epoll_wait(3, [{events=EPOLLOUT|EPOLLHUP, data={u32=2625883648,
>> u64=94496201362944}}], 1, 30000) = 1
>>
>> lrwx------ 1 root root 64 Apr 23 01:08 0 -> /dev/ptmx
>> l-wx------ 1 root root 64 Apr 23 01:08 1 -> 'pipe:[4261]'
>> l-wx------ 1 root root 64 Apr 23 01:08 2 -> 'pipe:[4261]'
>>
>> To fix this:
>> 1. Open master pty in parent process
>> 2. Attach slave pty to child stdin
>> 3. Remove detaching controlling tty from parent process
>> 4. Remove seting process groupid for child process
>>
>> Refer:
>> lrwx------ 1 root root 64 Apr 23 07:35 0 -> /dev/pts/0
>> l-wx------ 1 root root 64 Apr 23 07:35 1 -> 'pipe:[37001]'
>> l-wx------ 1 root root 64 Apr 23 07:35 2 -> 'pipe:[37001]'
>>
>> Signed-off-by: Changqing Li <changqing.li@windriver.com>
>> ---
>> utils.c | 67 ++++++++++++++++++++++++++++++++-------------------------
>> 1 file changed, 38 insertions(+), 29 deletions(-)
>>
>> diff --git a/utils.c b/utils.c
>> index 46918f4..6b3a492 100644
>> --- a/utils.c
>> +++ b/utils.c
>> @@ -341,25 +341,31 @@ run_child(char *run_ptest, int fd_stdout, int
>> fd_stderr)
>> /* exit(1); not needed? */
>> }
>>
>> +/* Returns an integer.
>> + * If it returns < 0, an error has occurred.
>> + * Otherwise, return 0 means successfully
>> + * fp should be writable, likely stdout/err.
>> + */
>> +static int
>> +setup_master_pty(int* pty_master, int* pty_slave, char*pty_name, FILE
>> *fp) {
>> + if (openpty(pty_master, pty_slave, pty_name, NULL, NULL) < 0) {
>> + fprintf(fp, "ERROR: openpty() failed with: %s.\n",
>> strerror(errno));
>> + return -1;
>> + }
>> + return 0;
>> +}
>> +
>> /* Returns an integer file descriptor.
>> * If it returns < 0, an error has occurred.
>> * Otherwise, it has returned the slave pty file descriptor.
>> * fp should be writable, likely stdout/err.
>> */
>> static int
>> -setup_slave_pty(FILE *fp) {
>> - int pty_master = -1;
>> - int pty_slave = -1;
>> - char pty_name[256];
>> +setup_slave_pty(char* pty_name, FILE *fp) {
>> struct group *gptr;
>> gid_t gid;
>> int slave = -1;
>>
>> - if (openpty(&pty_master, &pty_slave, pty_name, NULL, NULL) < 0) {
>> - fprintf(fp, "ERROR: openpty() failed with: %s.\n",
>> strerror(errno));
>> - return -1;
>> - }
>> -
>> if ((gptr = getgrnam(pty_name)) != 0) {
>> gid = gptr->gr_gid;
>> } else {
>> @@ -384,9 +390,9 @@ setup_slave_pty(FILE *fp) {
>> if ((slave = open(pty_name, O_RDWR)) == -1) {
>> fprintf(fp, "ERROR: open() failed with: %s.\n",
>> strerror(errno));
>> }
>> - return (slave);
>> -}
>>
>> + return slave;
>> +}
>>
>> int
>> run_ptests(struct ptest_list *head, const struct ptest_options opts,
>> @@ -406,16 +412,13 @@ run_ptests(struct ptest_list *head, const struct
>> ptest_options opts,
>>
>> do
>> {
>> - if (isatty(0) && ioctl(0, TIOCNOTTY) == -1) {
>> - fprintf(fp, "ERROR: Unable to detach from
>> controlling tty, %s\n", strerror(errno));
>> - }
>> -
>> -
>> fprintf(fp, "START: %s\n", progname);
>> PTEST_LIST_ITERATE_START(head, p)
>> int pipefd_stdout[2] = {-1, -1};
>> int pipefd_stderr[2] = {-1, -1};
>> - int pgid = -1;
>> + int pty_master = -1;
>> + int pty_slave = -1;
>> + char pty_name[256] = {0};
>>
>> if (pipe2(pipefd_stdout, 0) == -1) {
>> rc = -1;
>> @@ -435,8 +438,9 @@ run_ptests(struct ptest_list *head, const struct
>> ptest_options opts,
>> }
>> dirname(ptest_dir);
>>
>> - if ((pgid = getpgid(0)) == -1) {
>> - fprintf(fp, "ERROR: getpgid() failed,
>> %s\n", strerror(errno));
>>
>
> This comes from Randy patch to ensure child tests are in the same process
> group than the ptest-runner, 59381a64. Any specific reason to remove this?.
>
> See blow comments where pgid is used.
>
>
>
>> + if (setup_master_pty(&pty_master, &pty_slave,
>> pty_name, fp) < 0) {
>> + fprintf(fp, "ERROR: setup_master_pty
>> failed, %s\n", strerror(errno));
>> + break;
>> }
>>
>> pid_t child = fork();
>> @@ -447,25 +451,32 @@ run_ptests(struct ptest_list *head, const struct
>> ptest_options opts,
>> } else if (child == 0) {
>> int slave;
>>
>> - close(0);
>> /* Close read ends of the pipe */
>> do_close(&pipefd_stdout[PIPE_READ]);
>> do_close(&pipefd_stderr[PIPE_READ]);
>> + do_close(&pty_master);
>>
>> - if ((slave = setup_slave_pty(fp)) < 0) {
>> + if ((slave = setup_slave_pty(pty_name,
>> fp)) < 0) {
>> fprintf(fp, "ERROR: could not
>> setup pty (%d).", slave);
>> }
>> - if (setpgid(0,pgid) == -1) {
>> - fprintf(fp, "ERROR: setpgid()
>> failed, %s\n", strerror(errno));
>> - }
>>
> The pgid is used to set child process to join the same progress group as
> parent.
>
> But next line, we will make child as session leader, then the child will
> also be in the new progress group.
>
> So above setpgid actually not need. Here is the process stat with
> ptest-runner(without this patch):
>
> root@qemux86-64:~# cat /proc/455/stat
> 455 (ptest-runner) S 422 *455* 283 0 -1 4194304 109 0 0 0 0 2 0 0 20 0 1
> 0 62009 2441216 352 18446744073709551615 94258154364928 94258154373585
> 140722320232848 0 0 0 0 0 0 1 0 0 17 0 0 0 0 0 0 94258154384384
> 94258154385424 94258154389504 140722320240155 140722320240178
> 140722320240178 140722320240610 0
> root@qemux86-64:~# cat /proc/456/stat
> 456 (run-ptest) S 455 *456* 456 34817 456 4194560 242 0 0 0 0 0 0 0 20 0
> 1 0 62065 3813376 640 18446744073709551615 94612400209920 94612400903225
> 140724672843248 0 0 0 65536 4 65538 1 0 0 17 1 0 0 0 0 0 94612401150672
> 94612401198372 94612401246208 140724672851446 140724672851491
> 140724672851491 140724672851923 0
> root@qemux86-64:~#
>
> We can see, although setpgid, but finally, we pgid of child will be the
> child pid.
>
>
>> if (setsid() == -1) {
>> fprintf(fp, "ERROR: setsid()
>> failed, %s\n", strerror(errno));
>> }
>>
>> - if (ioctl(0, TIOCSCTTY, NULL) == -1) {
>> + if (ioctl(slave, TIOCSCTTY, NULL) == -1) {
>> fprintf(fp, "ERROR: Unable to
>> attach to controlling tty, %s\n", strerror(errno));
>> }
>
> + if (slave > 2) {
>> + close(slave);
>> + }
>> +
>> + if (dup2(pty_slave, STDIN_FILENO) < 0) {
>> + fprintf(fp, "ERROR: Unable to dup
>> slave pty to stdin, %s\n", strerror(errno));
>> + }
>> + if (pty_slave > 2) {
>> + close(pty_slave);
>> + }
>>
> Which is the reason to make it twice?, and also closing with slave > 2.
>
> first, according to current code logic, the slave/pty_slave should
> always > 2, the if checking just for ensure it is.
>
> pty_slave and slave are point to slave pty, but they have different fd.
> So I need to close them both after dup slave pty to stdin.
>
>
>
>>
>> if (chdir(ptest_dir) == -1) {
>> fprintf(fp, "ERROR: Unable to
>> chdir(%s), %s\n", ptest_dir, strerror(errno));
>> @@ -481,10 +492,6 @@ run_ptests(struct ptest_list *head, const struct
>> ptest_options opts,
>> do_close(&pipefd_stdout[PIPE_WRITE]);
>> do_close(&pipefd_stderr[PIPE_WRITE]);
>>
>> - if (setpgid(child, pgid) == -1) {
>> - fprintf(fp, "ERROR: setpgid()
>> failed, %s\n", strerror(errno));
>> - }
>> -
>>
> Removal comment up.
>
> Same as what I comments above, child is in another session, so this is
> not needed.
>
>
> Regards
>
> Changqing
>
>
>
>> time_t start_time= time(NULL);
>> fprintf(fp, "%s\n", get_stime(stime,
>> GET_STIME_BUF_SIZE, start_time));
>> fprintf(fp, "BEGIN: %s\n", ptest_dir);
>> @@ -608,6 +615,8 @@ run_ptests(struct ptest_list *head, const struct
>> ptest_options opts,
>> do_close(&pipefd_stdout[PIPE_WRITE]);
>> do_close(&pipefd_stderr[PIPE_READ]);
>> do_close(&pipefd_stderr[PIPE_WRITE]);
>> + do_close(&pty_master);
>> + do_close(&pty_slave);
>>
>> fflush(fp);
>> fflush(fp_stderr);
>> --
>> 2.25.1
>>
>>
[-- Attachment #2: Type: text/html, Size: 19703 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-05-02 22:42 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-25 1:47 [PATCH V2] utils.c: fix pty setup Changqing Li
2024-04-25 1:56 ` Anibal Limon
2024-04-25 21:59 ` Anibal Limon
2024-04-26 1:21 ` Changqing Li
2024-05-02 22:42 ` Anibal Limon
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.