Re: [ptest-runner][PATCH v2 4/4] utils: ensure child can be session leader

[Randy MacLeod <randy.macleod@windriver.com>]

On 6/25/19 9:51 PM, Anibal Limon wrote:
> 
> 
> On Wed, 19 Jun 2019 at 12:50, Randy MacLeod <randy.macleod@windriver.com 
> <mailto:randy.macleod@windriver.com>> wrote:
> 
>     On 6/14/19 10:48 AM, Randy MacLeod wrote:
>      > When running the run-execscript bash ptest as a user rather than
>     root, a warning:
>      >    bash: cannot set terminal process group (16036): Inappropriate
>     ioctl for device
>      >    bash: no job control in this shell
>      > contaminates the bash log files causing the test to fail. This
>     happens only
>      > when run under ptest-runner and not when interactively testing!
>      >
>      > The changes made to fix this include:
>      > 1. Get the process group id (pgid) before forking,
>      > 2. Set the pgid in both the parent and child to avoid a race,
>      > 3. Find, open and set permission on the child tty, and
>      > 4. Allow the child to attach to controlling tty.
>      >
>      > Also add '-lutil' to Makefile. This lib is from libc and provides
>     openpty.
> 
> 
>     Hmmm, I was making the code compile cleanly under clang using
>         -Weverything
>     when I noticed:
> 
>     1. the 'make check' tests. They still work fine.
>     2. The './ptest-runner -d tests/data -t 1' tests
>          which now generate loads of error like:
>           ERROR: Unable to detach from controlling tty, Inappropriate ioctl
>     for device
> 
>     so while this change fixed the bash-ptest, the ptest-runner self-test
>     it did something wrong.... Ah, I'm calling:
>          ioctl(0, TIOCNOTTY) == -1)
>     repeatedly in the parent so that's what's generating the extra logs.
>     Fixed locally and I'll send a patch but it's not urgent. Phew! :)
> 
>     Anibal,
> 
>     If you could reply to explain your plans for Richard's patches
>     that would help me figure out when to send the clang warning clean-ups
>     commits and what commit to base my work on.
> 
> 
> Hi,
> 
> I plan to take the Richard patches, He added in the recipe to have real 
> testing and looks like
> there aren't problems related to, Richard can you confirm it?,
> 
> Regarding the openpty include, I see some linkage problem when running 
> make check, proposed fix:

Yes, I had noticed that and fixed it as well.

I'll send my latest patch series once you have merged
Richard's changes into master. Hopefully that will be today... :)

I decided to compile with:
   clang -Weverything
to see if there were any problems and there
were quite a few things to fix. Now, for the most part,
neither clang nor gcc complain about the code.

../Randy

> 
> ...
> --- a/Makefile
> +++ b/Makefile
> @@ -22,19 +22,20 @@ TEST_SOURCES=tests/main.c tests/ptest_list.c 
> tests/utils.c $(BASE_SOURCES)
>   TEST_OBJECTS=$(TEST_SOURCES:.c=.o)
>   TEST_EXECUTABLE=ptest-runner-test
>   TEST_LDFLAGS=-lm -lrt -lpthread
> -TEST_LIBSTATIC=-lcheck -lsubunit
> +TEST_LIBSTATIC=-lutil
> +TEST_LIBSTATIC_TEST=$(TEST_LIBSTATIC) -lcheck -lsubunit
> 
>   TEST_DATA=$(shell echo `pwd`/tests/data)
> 
>   all: $(SOURCES) $(EXECUTABLE)
> 
>   $(EXECUTABLE): $(OBJECTS)
> -       $(CC) $(LDFLAGS) $(OBJECTS) -lutil -o $@
> +       $(CC) $(LDFLAGS) $(OBJECTS) -o $@ $(TEST_LIBSTATIC)
> 
>   tests: $(TEST_SOURCES) $(TEST_EXECUTABLE)
> 
>   $(TEST_EXECUTABLE): $(TEST_OBJECTS)
> -       $(CC) $(LDFLAGS) $(TEST_LDFLAGS) $(TEST_OBJECTS) -o $@ 
> $(TEST_LIBSTATIC)
> +       $(CC) $(LDFLAGS) $(TEST_LDFLAGS) $(TEST_OBJECTS) -o $@ 
> $(TEST_LIBSTATIC_TEST)
> 
>   check: $(TEST_EXECUTABLE)
>          ./$(TEST_EXECUTABLE) -d $(TEST_DATA)
> ...
> 
> Best regards,
> Anibal
> 
> 
> 
>     ../Randy
> 
>      >
>      > Signed-off-by: Sakib Sajal <sakib.sajal@windriver.com
>     <mailto:sakib.sajal@windriver.com>>
>      > Signed-off-by: Randy MacLeod <Randy.MacLeod@windriver.com
>     <mailto:Randy.MacLeod@windriver.com>>
>      > ---
>      >   Makefile |   2 +-
>      >   utils.c  | 102
>     +++++++++++++++++++++++++++++++++++++++++++++++++------
>      >   2 files changed, 92 insertions(+), 12 deletions(-)
>      >
>      > diff --git a/Makefile b/Makefile
>      > index 1bde7be..439eb79 100644
>      > --- a/Makefile
>      > +++ b/Makefile
>      > @@ -29,7 +29,7 @@ TEST_DATA=$(shell echo `pwd`/tests/data)
>      >   all: $(SOURCES) $(EXECUTABLE)
>      >
>      >   $(EXECUTABLE): $(OBJECTS)
>      > -     $(CC) $(LDFLAGS) $(OBJECTS) -o $@
>      > +     $(CC) $(LDFLAGS) $(OBJECTS) -lutil -o $@
>      >
>      >   tests: $(TEST_SOURCES) $(TEST_EXECUTABLE)
>      >
>      > diff --git a/utils.c b/utils.c
>      > index ad737c2..f11ce39 100644
>      > --- a/utils.c
>      > +++ b/utils.c
>      > @@ -1,5 +1,6 @@
>      >   /**
>      >    * Copyright (c) 2016 Intel Corporation
>      > + * Copyright (C) 2019 Wind River Systems, Inc.
>      >    *
>      >    * This program is free software; you can redistribute it and/or
>      >    * modify it under the terms of the GNU General Public License
>      > @@ -22,23 +23,27 @@
>      >    */
>      >
>      >   #define _GNU_SOURCE
>      > +
>      >   #include <stdio.h>
>      >
>      > +#include <dirent.h>
>      > +#include <errno.h>
>      > +#include <fcntl.h>
>      > +#include <grp.h>
>      >   #include <libgen.h>
>      > -#include <signal.h>
>      >   #include <poll.h>
>      > -#include <fcntl.h>
>      > +#include <pty.h>
>      > +#include <signal.h>
>      > +#include <stdlib.h>
>      > +#include <string.h>
>      >   #include <time.h>
>      > -#include <dirent.h>
>      > +#include <unistd.h>
>      > +
>      > +#include <sys/ioctl.h>
>      >   #include <sys/resource.h>
>      > +#include <sys/stat.h>
>      >   #include <sys/types.h>
>      >   #include <sys/wait.h>
>      > -#include <sys/stat.h>
>      > -#include <unistd.h>
>      > -#include <string.h>
>      > -#include <stdlib.h>
>      > -
>      > -#include <errno.h>
>      >
>      >   #include "ptest_list.h"
>      >   #include "utils.h"
>      > @@ -346,6 +351,53 @@ wait_child(const char *ptest_dir, const char
>     *run_ptest, pid_t pid,
>      >       return status;
>      >   }
>      >
>      > +/* 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];
>      > +     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 {
>      > +             /* If the tty group does not exist, don't change the
>      > +              * group on the slave pty, only the owner
>      > +              */
>      > +             gid = -1;
>      > +     }
>      > +
>      > +     /* chown/chmod the corresponding pty, if possible.
>      > +      * This will only work if the process has root permissions.
>      > +      */
>      > +     if (chown(pty_name, getuid(), gid) != 0) {
>      > +             fprintf(fp, "ERROR; chown() failed with: %s.\n",
>     strerror(errno));
>      > +     }
>      > +
>      > +     /* Makes the slave read/writeable for the user. */
>      > +     if (chmod(pty_name, S_IRUSR|S_IWUSR) != 0) {
>      > +             fprintf(fp, "ERROR: chmod() failed with: %s.\n",
>     strerror(errno));
>      > +     }
>      > +
>      > +     if ((slave = open(pty_name, O_RDWR)) == -1) {
>      > +             fprintf(fp, "ERROR: open() failed with: %s.\n",
>     strerror(errno));
>      > +     }
>      > +     return (slave);
>      > +}
>      > +
>      > +
>      >   int
>      >   run_ptests(struct ptest_list *head, const struct ptest_options
>     opts,
>      >               const char *progname, FILE *fp, FILE *fp_stderr)
>      > @@ -362,6 +414,8 @@ run_ptests(struct ptest_list *head, const
>     struct ptest_options opts,
>      >       int timeouted;
>      >       time_t sttime, entime;
>      >       int duration;
>      > +     int slave;
>      > +     int pgid = -1;
>      >
>      >       if (opts.xml_filename) {
>      >               xh = xml_create(ptest_list_length(head),
>     opts.xml_filename);
>      > @@ -379,7 +433,6 @@ run_ptests(struct ptest_list *head, const
>     struct ptest_options opts,
>      >                       close(pipefd_stdout[1]);
>      >                       break;
>      >               }
>      > -
>      >               fprintf(fp, "START: %s\n", progname);
>      >               PTEST_LIST_ITERATE_START(head, p);
>      >                       char *ptest_dir = strdup(p->run_ptest);
>      > @@ -388,6 +441,13 @@ run_ptests(struct ptest_list *head, const
>     struct ptest_options opts,
>      >                               break;
>      >                       }
>      >                       dirname(ptest_dir);
>      > +                     if (ioctl(0, TIOCNOTTY) == -1) {
>      > +                             fprintf(fp, "ERROR: Unable to
>     detach from controlling tty, %s\n", strerror(errno));
>      > +                     }
>      > +
>      > +                     if ((pgid = getpgid(0)) == -1) {
>      > +                             fprintf(fp, "ERROR: getpgid()
>     failed, %s\n", strerror(errno));
>      > +                     }
>      >
>      >                       child = fork();
>      >                       if (child == -1) {
>      > @@ -395,13 +455,33 @@ run_ptests(struct ptest_list *head, const
>     struct ptest_options opts,
>      >                               rc = -1;
>      >                               break;
>      >                       } else if (child == 0) {
>      > -                             setsid();
>      > +                             close(0);
>      > +                             if ((slave = setup_slave_pty(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) {
>      > +                                     fprintf(fp, "ERROR: Unable
>     to attach to controlling tty, %s\n", strerror(errno));
>      > +                             }
>      > +
>      >                               run_child(p->run_ptest,
>     pipefd_stdout[1], pipefd_stderr[1]);
>      > +
>      >                       } else {
>      >                               int status;
>      >                               int fds[2]; fds[0] =
>     pipefd_stdout[0]; fds[1] = pipefd_stderr[0];
>      >                               FILE *fps[2]; fps[0] = fp; fps[1] =
>     fp_stderr;
>      >
>      > +                             if (setpgid(child, pgid) == -1) {
>      > +                                     fprintf(fp, "ERROR:
>     setpgid() failed, %s\n", strerror(errno));
>      > +                             }
>      > +
>      >                               sttime = time(NULL);
>      >                               fprintf(fp, "%s\n",
>     get_stime(stime, GET_STIME_BUF_SIZE, sttime));
>      >                               fprintf(fp, "BEGIN: %s\n", ptest_dir);
>      >
> 
> 
>     -- 
>     # Randy MacLeod
>     # Wind River Linux
> 


-- 
# Randy MacLeod
# Wind River Linux