V6 test-in-container patch

Now that 2.28 is out, a young programmer's fancy turns to testing... ;-)

No changes since last submittal (V5), I bumped to V6 to drop the RFQ
tag.  bash, echo, and true added to buildroot.  Parallel/cross/native
testing supported.  I'm pretty sure I've covered everyone's comments
and suggestions, and added or changed all the things that need to be
added or changed.

May I check it in yet?  Joseph, Carlos, Florian - you've reviewed it
in the past, do you wish to review it again?  Or either join consensus
or abstain (if time doesn't permit)?

BTW I'm not adding a mail program for the testroot ;-)


* Makefile (testroot.pristine): New rules to initialize the
test-in-container "testroot".
* Makerules (all-testsuite): Add tests-container.
* Rules (tests): Add tests-container.
(binaries-all-tests): Likewise.
(tests-container): New, run these tests in the testroot container.
* support/Makefile (others): Add *-container, support_paths.c,
xmkdirp, and links-dso-program.
* support/links-dso-program-c.c: New.
* support/links-dso-program.cc: New.
* support/test-container.c: New.
* support/shell-container.c: New.
* support/echo-container.c: New.
* support/true-container.c: New.
* support/mkdirp.c: New.
* support/support_paths.c: New.
* support/support.h: Add mkdirp() and support paths prototypes.

* nss/tst-nss-test3.c: Convert to test-in-container.
* nss/tst-nss-test3.root/: New.

On 08/03/2018 05:32 PM, DJ Delorie wrote:
> Now that 2.28 is out, a young programmer's fancy turns to testing... ;-)
> 
> No changes since last submittal (V5), I bumped to V6 to drop the RFQ
> tag.  bash, echo, and true added to buildroot.  Parallel/cross/native
> testing supported.  I'm pretty sure I've covered everyone's comments
> and suggestions, and added or changed all the things that need to be
> added or changed.
> 
> May I check it in yet?  Joseph, Carlos, Florian - you've reviewed it
> in the past, do you wish to review it again?  Or either join consensus
> or abstain (if time doesn't permit)?
> 
> BTW I'm not adding a mail program for the testroot ;-)

OK, I reviewed this with a careful eye towards it being really nicely
polished. This is going to be a piece of infrastructure that is with us
for a long time and we need to make sure we check all error conditions
and that the code is clean and well commented. To that end I've found
some things that need fixing, please go thorugh them, and post v7, there
is enough change to warrant a v7, rather than a "OK if you fix X." A lot
of it is mechanical, and not technical, there are just a few technical
nits which include mostly error checking and the creationof more xFOO
support functions for use with the container program.

Please review all the changes below and post v7.

IMO with v7 I think we'll be ready to commit. I don't see much elase that
needs fixing to deploy this in day-to-day development.

> * Makefile (testroot.pristine): New rules to initialize the
> test-in-container "testroot".
> * Makerules (all-testsuite): Add tests-container.
> * Rules (tests): Add tests-container.
> (binaries-all-tests): Likewise.
> (tests-container): New, run these tests in the testroot container.
> * support/Makefile (others): Add *-container, support_paths.c,
> xmkdirp, and links-dso-program.
> * support/links-dso-program-c.c: New.
> * support/links-dso-program.cc: New.
> * support/test-container.c: New.
> * support/shell-container.c: New.
> * support/echo-container.c: New.
> * support/true-container.c: New.
> * support/mkdirp.c: New.
> * support/support_paths.c: New.
> * support/support.h: Add mkdirp() and support paths prototypes.
> 
> * nss/tst-nss-test3.c: Convert to test-in-container.
> * nss/tst-nss-test3.root/: New.
> 
> diff --git a/Makefile b/Makefile
> index d3f25a525a..8d07375fba 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -340,6 +340,48 @@ define summarize-tests
>  @! egrep -q -v '^(X?PASS|XFAIL|UNSUPPORTED):' $(objpfx)$1
>  endef
>  
> +# The intention here is to do ONE install of our build into the
> +# testroot.pristine/ directory, then rsync (internal to
> +# support/test-container) that to testroot.root/ at the start of each
> +# test.  That way we can promise each test a "clean" install, without
> +# having to do the install for each test.
> +#

OK.

> +# In addition, we have to copy some files into this root in addition
> +# to what glibc installs.  For example, many tests require /bin/sh be
> +# present, and any shared objects that /bin/sh depends on.  We also
> +# build a "test" program in either C or (if available) C++ just so we
> +# can copy in any shared objects that GCC-compiled programs depend on.
> +

This comment in inaccurate because we build a shell-container program and
so we no longer copy in /bin/sh and the objects it depends upon. Please
cleanup comment.

> +$(tests-container) $(addsuffix /tests,$(subdirs)) : $(objpfx)testroot.pristine/install.stamp
> +$(objpfx)testroot.pristine/install.stamp :
> +	test -d $(objpfx)testroot.pristine || \
> +	  mkdir $(objpfx)testroot.pristine
> +	# We need a working /bin/sh for some of the tests.
> +	test -d $(objpfx)testroot.pristine/bin || \
> +	  mkdir $(objpfx)testroot.pristine/bin
> +	cp $(objpfx)support/shell-container $(objpfx)testroot.pristine/bin/sh
> +	cp $(objpfx)support/echo-container $(objpfx)testroot.pristine/bin/echo
> +	cp $(objpfx)support/true-container $(objpfx)testroot.pristine/bin/true
> +	# Copy these DSOs first so we can overwrite them with our own.
> +	for dso in `$(test-wrapper-env) LD_TRACE_LOADED_OBJECTS=1  $(objpfx)elf/$(rtld-installed-name) \
> +			        $(objpfx)testroot.pristine/bin/sh \
> +	                      | grep / | sed 's/^[^/]*//' | sed 's/ .*//'` ;\
> +	  do \
> +	    test -d `dirname $(objpfx)testroot.pristine$$dso` || \
> +	      mkdir -p `dirname $(objpfx)testroot.pristine$$dso` ;\
> +	    $(test-wrapper) cp $$dso $(objpfx)testroot.pristine$$dso ;\
> +	  done
> +	for dso in `$(test-wrapper-env) LD_TRACE_LOADED_OBJECTS=1  $(objpfx)elf/$(rtld-installed-name) \
> +			        $(objpfx)support/links-dso-program \
> +	                      | grep / | sed 's/^[^/]*//' | sed 's/ .*//'` ;\
> +	  do \
> +	    test -d `dirname $(objpfx)testroot.pristine$$dso` || \
> +	      mkdir -p `dirname $(objpfx)testroot.pristine$$dso` ;\
> +	    $(test-wrapper) cp $$dso $(objpfx)testroot.pristine$$dso ;\
> +	  done
> +	$(MAKE) install DESTDIR=$(objpfx)testroot.pristine
> +	touch $(objpfx)testroot.pristine/install.stamp
> +

OK.

>  tests-special-notdir = $(patsubst $(objpfx)%, %, $(tests-special))
>  tests: $(tests-special)
>  	$(..)scripts/merge-test-results.sh -s $(objpfx) "" \
> diff --git a/Makerules b/Makerules
> index a10a0b4d70..64dab056ea 100644
> --- a/Makerules
> +++ b/Makerules
> @@ -1369,7 +1369,7 @@ xcheck: xtests
>  # The only difference between MODULE_NAME=testsuite and MODULE_NAME=nonlib is
>  # that almost all internal declarations from config.h, libc-symbols.h, and
>  # include/*.h are not available to 'testsuite' code, but are to 'nonlib' code.
> -all-testsuite := $(strip $(tests) $(xtests) $(test-srcs) $(test-extras))
> +all-testsuite := $(strip $(tests) $(xtests) $(test-srcs) $(test-extras) $(tests-container))

OK.

>  ifneq (,$(all-testsuite))
>  cpp-srcs-left = $(all-testsuite)
>  lib := testsuite
> diff --git a/Rules b/Rules
> index 706c8a749d..c3fd2bb0cb 100644
> --- a/Rules
> +++ b/Rules
> @@ -130,12 +130,12 @@ others: $(py-const)
>  
>  ifeq ($(run-built-tests),no)
>  tests: $(addprefix $(objpfx),$(filter-out $(tests-unsupported), \
> -                                          $(tests) $(tests-internal)) \
> +                                          $(tests) $(tests-internal) $(tests-container)) \

OK.

>  			     $(test-srcs)) $(tests-special) \
>  			     $(tests-printers-programs)
>  xtests: tests $(xtests-special)
>  else
> -tests: $(tests:%=$(objpfx)%.out) $(tests-internal:%=$(objpfx)%.out) \
> +tests: $(tests:%=$(objpfx)%.out) $(tests-internal:%=$(objpfx)%.out) $(tests-container:%=$(objpfx)%.out) \

OK.

>         $(tests-special) $(tests-printers-out)
>  xtests: tests $(xtests:%=$(objpfx)%.out) $(xtests-special)
>  endif
> @@ -149,7 +149,7 @@ tests-expected = $(tests) $(tests-internal) $(tests-printers)
>  endif
>  tests:
>  	$(..)scripts/merge-test-results.sh -s $(objpfx) $(subdir) \
> -	  $(sort $(tests-expected) $(tests-special-notdir:.out=)) \
> +	  $(sort $(tests-expected) $(tests-special-notdir:.out=) $(tests-container)) \

OK.

>  	  > $(objpfx)subdir-tests.sum
>  xtests:
>  	$(..)scripts/merge-test-results.sh -s $(objpfx) $(subdir) \
> @@ -158,7 +158,7 @@ xtests:
>  
>  ifeq ($(build-programs),yes)
>  binaries-all-notests = $(others) $(sysdep-others)
> -binaries-all-tests = $(tests) $(tests-internal) $(xtests) $(test-srcs)
> +binaries-all-tests = $(tests) $(tests-internal) $(xtests) $(test-srcs) $(tests-container)

OK.

>  binaries-all = $(binaries-all-notests) $(binaries-all-tests)
>  binaries-static-notests = $(others-static)
>  binaries-static-tests = $(tests-static) $(xtests-static)
> @@ -248,6 +248,16 @@ $(objpfx)%.out: /dev/null $(objpfx)%	# Make it 2nd arg for canned sequence.
>  	$(make-test-out) > $@; \
>  	$(evaluate-test)
>  
> +
> +# Any tests that require an isolated container (chroot) in which to
> +# run, should be added to tests-container.
> +$(tests-container:%=$(objpfx)%.out): $(objpfx)%.out : $(if $(wildcard $(objpfx)%.files),$(objpfx)%.files,/dev/null) $(objpfx)%
> +	$(test-wrapper-env) $(run-program-env) $(run-via-rtld-prefix) \
> +	  $(common-objpfx)support/test-container env $(run-program-env) $($*-ENV) \
> +	  $(host-test-program-cmd) $($*-ARGS) > $@; \
> +	$(evaluate-test)

OK.

> +
> +
>  # tests-unsupported lists tests that we will not try to build at all in
>  # this configuration.  Note this runs every time because it does not
>  # actually create its target.  The dependency on Makefile is meant to
> diff --git a/nss/Makefile b/nss/Makefile
> index 66fac7f5b8..1f8601680f 100644
> --- a/nss/Makefile
> +++ b/nss/Makefile
> @@ -55,11 +55,13 @@ tests-internal		= tst-field
>  tests			= test-netdb test-digits-dots tst-nss-getpwent bug17079 \
>  			  tst-nss-test1 \
>  			  tst-nss-test2 \
> -			  tst-nss-test3 \

OK.

>  			  tst-nss-test4 \
>  			  tst-nss-test5
>  xtests			= bug-erange
>  
> +tests-container = \
> +			  tst-nss-test3

OK. Nice to see our first containerized test!

> +
>  # Tests which need libdl
>  ifeq (yes,$(build-shared))
>  tests += tst-nss-files-hosts-erange
> diff --git a/nss/tst-nss-test3.c b/nss/tst-nss-test3.c
> index d9d708ae7b..4112231778 100644
> --- a/nss/tst-nss-test3.c
> +++ b/nss/tst-nss-test3.c
> @@ -107,7 +107,11 @@ do_test (void)
>    int i;
>    struct group *g = NULL;
>  
> -  __nss_configure_lookup ("group", "test1");
> +/* Previously we used __nss_configure_lookup to isolate the test
> +   from the host environment and to get it to lookup from our new
> +   test1 NSS service module, but now this test is run in a different
> +   root filesystem via the test-container support and we directly
> +   configure the use of the test1 NSS service.  */

OK.

>  
>    setgrent ();
>  
> diff --git a/nss/tst-nss-test3.root/etc/nsswitch.conf b/nss/tst-nss-test3.root/etc/nsswitch.conf
> new file mode 100644
> index 0000000000..5e08fe5eea
> --- /dev/null
> +++ b/nss/tst-nss-test3.root/etc/nsswitch.conf
> @@ -0,0 +1 @@
> +group	test1

OK.

> diff --git a/nss/tst-nss-test3.root/files.txt b/nss/tst-nss-test3.root/files.txt
> new file mode 100644
> index 0000000000..a10beb1e6c
> --- /dev/null
> +++ b/nss/tst-nss-test3.root/files.txt

Please rename files.txt to mytest.in or anything other than 'txt' which is
a document that contains unformatted text. The *.in name indicates they are
an input fragment that needs to be processed. Using the name of the test
instead of 'files' makes it consistent between sysroot'd and non-sysroot'd
tests.

Yes, we do have Unicode *.txt files, and I blame the Unicode Consortium for
this, those files are special with a special format, but I imagine they call
them text to imply an ASCII-encoded file and get past browser restrictions.

The files you have here might actually have UTF-8 in them so I'm wary to just
call them *.txt.

> @@ -0,0 +1,2 @@
> +cp $B/nss/libnss_test1.so $L/libnss_test1.so.2
> +cp $B/nss/libnss_test2.so $L/libnss_test2.so.2
> diff --git a/support/Makefile b/support/Makefile
> index 652d2cdf69..a6b52fb6f8 100644
> --- a/support/Makefile
> +++ b/support/Makefile
> @@ -53,6 +53,7 @@ libsupport-routines = \
>    support_format_netent \
>    support_isolate_in_subprocess \
>    support_openpty \
> +  support_paths \

OK.

>    support_quote_blob \
>    support_record_failure \
>    support_run_diff \
> @@ -84,6 +85,7 @@ libsupport-routines = \
>    xmalloc \
>    xmemstream \
>    xmkdir \
> +  xmkdirp \

OK.

>    xmmap \
>    xmprotect \
>    xmunmap \
> @@ -151,6 +153,42 @@ ifeq ($(build-shared),yes)
>  libsupport-inhibit-o += .o
>  endif
>  
> +CFLAGS-support_paths.c = \
> +		-DSRCDIR_PATH=\"`cd .. ; pwd`\" \
> +		-DOBJDIR_PATH=\"`cd $(objpfx)/..; pwd`\" \
> +		-DINSTDIR_PATH=\"$(prefix)\" \
> +		-DLIBDIR_PATH=\"$(libdir)\"

OK.

> +
> +others: \
> +	$(objpfx)test-container \
> +	$(objpfx)links-dso-program \
> +	$(objpfx)shell-container \
> +	$(objpfx)echo-container \
> +	$(objpfx)true-container

OK.

> +
> +LDLIBS-test-container = $(libsupport)
> +
> +others += test-container
> +others-noinstall += test-container
> +
> +others += shell-container echo-container true-container
> +others-noinstall += shell-container echo-container true-container
> +
> +$(objpfx)test-container : $(libsupport)
> +$(objpfx)shell-container : $(libsupport)
> +$(objpfx)echo-container : $(libsupport)
> +$(objpfx)true-container : $(libsupport)

OK.

> +
> +# This exists only so we can guess which OS DSOs we need to copy into
> +# the testing container.
> +ifeq (,$(CXX))
> +$(objpfx)links-dso-program : $(objpfx)links-dso-program-c.o
> +	$(LINK.o) -o $@ $^
> +else
> +$(objpfx)links-dso-program : $(objpfx)links-dso-program.o
> +	$(LINK.o) -o $@ $^ -lstdc++
> +endif

OK.

> +
>  tests = \
>    README-testing \
>    tst-support-namespace \
> diff --git a/support/echo-container.c b/support/echo-container.c
> new file mode 100644
> index 0000000000..9f8d78f510
> --- /dev/null
> +++ b/support/echo-container.c
> @@ -0,0 +1,34 @@
> +/* Minimal /bin/echo for in-container use.

OK.

> +   Copyright (C) 2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +
> +int
> +main (int argc, const char **argv)
> +{
> +  int i;
> +
> +  for (i=1; i<argc; i++)

GNU Formatting:

for (i = 1; i < argc; i++)

and yes, 66/4537 instances of this issues still need fixing in the sources
(mostly IBM math code).

> +    {
> +      if (i > 1)
> +	putchar (' ');
> +      fputs (argv[i], stdout);
> +    }
> +  putchar ('\n');
> +  return 0;
> +}

OK.

> diff --git a/support/links-dso-program-c.c b/support/links-dso-program-c.c
> new file mode 100644
> index 0000000000..398ec675a0
> --- /dev/null
> +++ b/support/links-dso-program-c.c
> @@ -0,0 +1,5 @@
> +int
> +main (void)
> +{

I'm worried that we don't call any libc function here and that in the future
a clever static linker or compiler optimization will reduce this to some kind
of free-standing implementation.

Can we add a call to printf() here that outputs the values of argc, argv, and
env? This way the compiler can't possibly optimize them out, or reduce it to
puts or a direct write syscall. Who knows what a future compiler can do so I'm
happy to make this a little bigger to it works forever.

e.g.
#include <stdio.h>
  
int main (int argc, char *argv[], char *envp[])
{
  printf ("main: %p %p %p\n", (void *)&argc, (void *)&argv[0],
          (void *)&envp[0]);
  return 0;
}

> +  return 0;
> +}
> diff --git a/support/links-dso-program.cc b/support/links-dso-program.cc
> new file mode 100644
> index 0000000000..398ec675a0
> --- /dev/null
> +++ b/support/links-dso-program.cc
> @@ -0,0 +1,5 @@
> +int
> +main (void)
> +{

Likewise, use cout.

> +  return 0;
> +}
> diff --git a/support/shell-container.c b/support/shell-container.c
> new file mode 100644
> index 0000000000..7354655418
> --- /dev/null
> +++ b/support/shell-container.c
> @@ -0,0 +1,359 @@
> +/* Minimal /bin/sh for in-container use.

OK.

> +   Copyright (C) 2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#define __USE_LARGEFILE64
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sched.h>
> +#include <sys/syscall.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <dirent.h>
> +#include <string.h>
> +#include <sys/mount.h>
> +#include <sys/stat.h>
> +#include <sys/fcntl.h>
> +#include <sys/file.h>
> +#include <sys/wait.h>
> +#include <stdarg.h>
> +#include <sys/sysmacros.h>
> +#include <ctype.h>
> +#include <utime.h>
> +#include <errno.h>
> +#include <error.h>
> +
> +#include <support/support.h>
> +
> +/* Design considerations
> +
> + General rule: optimize for developer time, not run time.
> +
> + Specifically:
> +
> + * Don't worry about slow algorithms
> + * Don't worry about free'ing memory
> + * Don't implement anything the testsuite doesn't need.

OK. I like these constraints. Thank you for listing them.

Could you also list limits? Like script line lengths? etc.

Could you then refactor the limits to be macro constants that
are defind *right here* and used later, such that we can increase
them if we need to without much work?

> +
> +*/
> +
> +#define DEBUG 0
> +#define dprintf if (DEBUG) fprintf

Not OK. This support will bitrot away. Please *always* compile
debugging into the test shell and add an opt argument to turn
it on e.g. --debug.

> +

Need's one-line comment about intent.

> +static int
> +true_func (char **argv)
> +{
> +  return 0;
> +}

OK.

> +

Need's comment about intent, and about
argv[i] and what are they, and what is written to
e.g. stdout.

> +static int
> +echo_func (char **argv)
> +{
> +  int i;
> +
> +  for (i=0; argv[i]; i++)

GNU formatting.

i = 0;

> +    {
> +      if (i > 0)
> +	putchar (' ');
> +      fputs(argv[i], stdout);
> +    }
> +  putchar ('\n');
> +
> +  return 0;
> +}

OK.

> +

Need's comment about intent, and describe
inputs e.g. argv[0] and argv[1], what are they and
if they have any restrictions.

> +static int
> +copy_func (char **argv)
> +{
> +  char *sname = argv[0];
> +  char *dname = argv[1];
> +  int sfd, dfd;
> +  struct stat st;
> +
> +  sfd = open (sname, O_RDONLY);
> +  if (sfd < 0)
> +    {
> +      dprintf (stderr, "unable to open %s for reading\n", sname);
> +      return 1;
> +    }
> +
> +  if (fstat (sfd, &st) < 0)
> +    {
> +      dprintf (stderr, "unable to fstat %s\n", sname);
> +      return 1;
> +    }
> +
> +  dfd = open (dname, O_WRONLY | O_TRUNC | O_CREAT, 0600);
> +  if (dfd < 0)
> +    {
> +      dprintf (stderr, "unable to open %s for writing\n", dname);
> +      return 1;
> +    }
> +
> +  if (copy_file_range (sfd, 0, dfd, 0, st.st_size, 0) != st.st_size)
> +    {
> +      dprintf (stderr, "cannot copy file %s to %s\n", sname, dname);

Please prefix the stderr dprintf's with "cp:" or "copy_func:"
so one knows where they came from.

> +      return 1;
> +    }

OK.

> +
> +  close (sfd);
> +  close (dfd);

OK.

> +
> +  chmod (dname, st.st_mode & 0777);
> +
> +  return 0;
> +
> +}
> +

Needs comment describing what this is.

> +static struct {
> +  const char *name;
> +  int (*func)(char **argv);
> +} builtin_funcs[] = {
> +  { "true", true_func },
> +  { "echo", echo_func },
> +  { "cp", copy_func },
> +  { NULL, NULL }
> +};
> +

Needs comment explaining what argv is, that it's a null-terminated
list of commands to run.

> +static void
> +run_command_array (char **argv)
> +{
> +  int i, j;
> +  pid_t pid;
> +  int status;
> +  int (*builtin_func)(char **args);
> +
> +  if (argv[0] == NULL)
> +    return;
> +
> +  builtin_func = NULL;
> +
> +  int new_stdin = 0;
> +  int new_stdout = 1;
> +  int new_stderr = 2;
> +
> +  dprintf (stderr, "dj: run_command_array\n");

Suggest:

dprintf (stderr, "run_command_array: starting\n");

> +  for (i=0; argv[i]; i++)

GNU Formatting.

i = 0;

> +    dprintf (stderr, "%%DJ%% argv [%d] `%s'\n", i, argv[i]);
> +
> +  for (j=i=0; argv[i]; i++)

Likewise.

j = i = 0;

> +    {
> +      if (strcmp (argv[i], "<") == 0 && argv[i+1])

Likewise.

argv[i + 1]

> +	{
> +	  new_stdin = open (argv[i+1], O_WRONLY|O_CREAT|O_TRUNC, 0777);

Likewise.

> +	  ++ i;

Likewise.

++i;

> +	  continue;
> +	}
> +      if (strcmp (argv[i], ">") == 0 && argv[i+1])

Likewise.

> +	{
> +	  new_stdout = open (argv[i+1], O_WRONLY|O_CREAT|O_TRUNC, 0777);

Likewise.

> +	  ++ i;

Likewise.

> +	  continue;
> +	}
> +      if (strcmp (argv[i], ">>") == 0 && argv[i+1])

Likewise.

> +	{
> +	  new_stdout = open (argv[i+1], O_WRONLY|O_CREAT|O_APPEND, 0777);

Likewise.

> +	  ++ i;

Likewise.

> +	  continue;
> +	}
> +      if (strcmp (argv[i], "2>") == 0 && argv[i+1])

Likewise.

> +	{
> +	  new_stderr = open (argv[i+1], O_WRONLY|O_CREAT|O_TRUNC, 0777);

Likewise.

> +	  ++ i;

Likewise.

> +	  continue;
> +	}
> +      argv[j++] = argv[i];
> +    }
> +  argv[j] = NULL;
> +
> +
> +  for (i=0; builtin_funcs[i].name; i++)

Likewise.

> +    if (strcmp (argv[0], builtin_funcs[i].name) == 0)
> +       builtin_func = builtin_funcs[i].func;
> +
> +  dprintf (stderr, "builtin %p argv0 `%s'\n", builtin_func, argv[0]);
> +
> +  pid = fork();
> +  if (pid < 0)
> +    {
> +      fprintf (stderr, "Can't fork");
> +      perror("The error was");

Suggest just:

perror ("sh: fork failed");

This mimics normal shell errors, and single line failures are eaisier to grep.

> +      exit(1);

OK.

> +    }
> +
> +  if (pid == 0)
> +    {
> +      if (new_stdin != 0)
> +	{
> +	  dup2 (new_stdin, 0);
> +	  close (new_stdin);
> +	}
> +      if (new_stdout != 1)
> +	{
> +	  dup2 (new_stdout, 1);
> +	  close (new_stdout);
> +	}
> +      if (new_stderr != 2)
> +	{
> +	  dup2 (new_stderr, 2);
> +	  close (new_stdout);
> +	}

OK.

> +
> +      if (builtin_func != NULL)
> +	exit (builtin_func(argv+1));

GNU Formatting.

exit (builtin_func(argv + 1));

> +
> +      execvp (argv[0], argv);
> +
> +      fprintf (stderr, "Can't exec %s", argv[0]);
> +      perror("The error was");

Suggest:

fprintf (stderr, "sh: execing '%s' failed: %s", argv[0], strerror (errno));

More concise single line failures easier for grepping.

> +      exit(1);
> +    }
> +
> +  waitpid (pid, &status, 0);
> +
> +  dprintf (stderr, "exiting run_command_array\n");

Suggest:

dprintf (stderr, "run_command_array: exiting\n");

My preference is for function name on the first column of output to
make it easier to grep, just like function names.

> +
> +  if (WIFEXITED (status))
> +    {
> +      int rv = WEXITSTATUS (status);
> +      if (rv)
> +	exit (rv);
> +    }
> +  else
> +    exit (1);

OK.

> +}
> +

Needs a comment explaining what the inputs to the function are
and what it does. Is args[100] a limitation, if so, it should
be described (see comment at top of file).

> +static void
> +run_command_string (const char *cmdline, const char **iargs)
> +{
> +  char *args[100];
> +  int ap = 0;
> +  const char *start, *end;
> +  int nargs;
> +
> +  for (nargs=0; iargs[nargs] != NULL; nargs++)

GNU Coding style:

nargs = 0;

> +    ;
> +
> +  dprintf (stderr, "%%DJ%%: run_command_string '%s'\n", cmdline);

Suggest:

dprintf (stderr, "run_command_string: '%s'\n", cmdline);

> +
> +  while (ap < 99)
> +    {
> +      int in_quote = 0;
> +
> +      while (*cmdline && isspace (*cmdline))
> +	cmdline ++;
> +      if (*cmdline == 0)
> +	break;
> +
> +      start = cmdline;
> +      in_quote = (*cmdline == '\'' || *cmdline == '"') ? *cmdline : 0;
> +
> +      dprintf (stderr, "in_quote %d\n", in_quote);
> +      while (*cmdline
> +	     && (!isspace (*cmdline) || in_quote))
> +	{
> +	  if (*cmdline == in_quote
> +	      && cmdline != start)
> +	    in_quote = 0;
> +	  dprintf (stderr, "[%c]%d ", *cmdline, in_quote);
> +	  cmdline ++;
> +	}
> +      dprintf (stderr, "\n");
> +
> +      end = cmdline;
> +      dprintf (stderr, "start<%s> end<%s>\n", start, end);
> +      args[ap] = (char *) malloc (end - start + 1);
> +      memcpy (args[ap], start, end - start);
> +      args[ap][end - start] = 0;
> +
> +      dprintf (stderr, "args[%d] = <%s>\n", ap, args[ap]);
> +      if (args[ap][0] == '\''
> +	  && args[ap][strlen(args[ap])-1] == '\'')
> +	{
> +	  args[ap][strlen(args[ap])-1] = 0;
> +	  args[ap] ++;
> +	}
> +
> +      else if (args[ap][0] == '"'
> +	  && args[ap][strlen(args[ap])-1] == '"')
> +	{
> +	  args[ap][strlen(args[ap])-1] = 0;
> +	  args[ap] ++;
> +	}
> +
> +      else if (args[ap][0] == '$'
> +	       && isdigit (args[ap][1])
> +	       && args[ap][2] == 0)
> +	{
> +	  int a = args[ap][1] - '1';
> +	  if (0 <= a && a < nargs)
> +	    args[ap] = strdup (iargs[a]);
> +	}
> +
> +      ap ++;
> +
> +      if (*cmdline == 0)
> +	break;

This whole while statement needs comments explaining the
logic of the algorithm.


> +    }
> +
> +  args[ap] = NULL;
> +  run_command_array (args);

OK.

> +}
> +

Needs comment explaining the function and what it expects as input,
and if line[1000] is a limit it should be explained (see comment at
top of file).

> +static void
> +run_script (const char *filename, const char **args)
> +{
> +  char line[1000];
> +  dprintf (stderr, "%%DJ%%: run_script '%s'\n", filename);

Suggest:

dprintf (stderr, "run_script: '%s'\n", filename);

> +  FILE *f = fopen (filename, "r");
> +  if (f == NULL)
> +    {

The dprintf and perror should be changed to...

> +      dprintf (stderr, "can't open %s for reading\n", filename);
> +      perror("The error was");

... this:

fprintf (stderr, "sh: %s: %s", argv[0], strerror (errno));

Which is the canonical error for running a script that doesn't exist.

e.g. sh: foobar: No such file or directory

> +      exit (1);
> +    }
> +  while (fgets (line, sizeof(line), f) != NULL)
> +    {
> +      if (line[0] == '#')
> +	{
> +	  dprintf (stderr, "comment: %s\n", line);
> +	  continue;
> +	}
> +      run_command_string (line, args);
> +    }
> +  fclose (f);

OK.

> +}
> +
> +int
> +main (int argc, const char **argv)
> +{
> +  int i;
> +
> +  for (i=0; i<argc; i++)

GNU Formatting.

for (i = 0; i < argc; i++)

> +    dprintf (stderr, "%%DJ%% sh [%d] `%s'\n", i, argv[i]);

Suggest:

dprintf (stderr, "sh: [%d] `%s'\n", i, argv[i]);

> +
> +
> +  if (strcmp (argv[1], "-c") == 0)
> +    run_command_string (argv[2], argv+3);
> +  else
> +    run_script (argv[1], argv+2);

Please adda a -d for debugging, but feel free to define it in
such a way that it makes the implementation easy.

It may make things a little messier here, you have to look for
-c, find it's index, then look from argv[1] to argv[<index of -c>]
for a -d that enables debugging (otherwise it's a program argument).

> +
> +  dprintf (stderr, "normal exit 0\n");
> +  return 0;
> +}
> diff --git a/support/support.h b/support/support.h
> index b61fe0735c..192b9e1424 100644
> --- a/support/support.h
> +++ b/support/support.h
> @@ -76,6 +76,19 @@ char *xasprintf (const char *format, ...)
>  char *xstrdup (const char *);
>  char *xstrndup (const char *, size_t);
>  
> +/* Equivalent of "mkdir -p".  */
> +void xmkdirp (const char *, int);

Not OK, shouldn't int be mode_t?

> +
> +/* These point to the TOP of the source/build tree, not your (or
> +   support's) subdirectory.  */
> +extern const char support_srcdir_root[];
> +extern const char support_objdir_root[];

OK.

> +
> +/* Corresponds to the --prefix= passed to configure.  */
> +extern const char support_install_prefix[];
> +/* Corresponds to the install's lib/ or lib64/ directory.  */
> +extern const char support_libdir_prefix[];


OK.

> +
>  __END_DECLS
>  
>  #endif /* SUPPORT_H */
> diff --git a/support/support_paths.c b/support/support_paths.c
> new file mode 100644
> index 0000000000..a1c22315bd
> --- /dev/null
> +++ b/support/support_paths.c
> @@ -0,0 +1,51 @@
> +/* Various paths that might be needed.

OK.

> +   Copyright (C) 2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <support/support.h>
> +#include <support/check.h>
> +
> +/* The idea here is to make various makefile-level paths available to
> +   support programs, as canonicalized absolute paths.  */
> +
> +/* These point to the TOP of the source/build tree, not your (or
> +   support's) subdirectory.  */
> +#ifdef SRCDIR_PATH
> +const char support_srcdir_root[] = SRCDIR_PATH;
> +#else
> +# error please -DSRCDIR_PATH=something in the Makefile
> +#endif
> +
> +#ifdef OBJDIR_PATH
> +const char support_objdir_root[] = OBJDIR_PATH;
> +#else
> +# error please -DOBJDIR_PATH=something in the Makefile
> +#endif
> +
> +#ifdef INSTDIR_PATH
> +/* Corresponds to the --prefix= passed to configure.  */
> +const char support_install_prefix[] = INSTDIR_PATH;
> +#else
> +# error please -DINSTDIR_PATH=something in the Makefile
> +#endif
> +
> +#ifdef LIBDIR_PATH
> +/* Corresponds to the install's lib/ or lib64/ directory.  */
> +const char support_libdir_prefix[] = LIBDIR_PATH;
> +#else
> +# error please -DLIBDIR_PATH=something in the Makefile
> +#endif

OK.

> diff --git a/support/test-container.c b/support/test-container.c
> new file mode 100644
> index 0000000000..cb232a1baa
> --- /dev/null
> +++ b/support/test-container.c
> @@ -0,0 +1,940 @@
> +/* Run a test case in an isolated namespace.
> +   Copyright (C) 2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#define __USE_LARGEFILE64
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sched.h>
> +#include <sys/syscall.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <dirent.h>
> +#include <string.h>
> +#include <sys/mount.h>
> +#include <sys/stat.h>
> +#include <sys/fcntl.h>
> +#include <sys/file.h>
> +#include <sys/wait.h>
> +#include <stdarg.h>
> +#include <sys/sysmacros.h>
> +#include <ctype.h>
> +#include <utime.h>
> +#include <errno.h>
> +#include <error.h>
> +
> +#include <support/support.h>
> +#include "check.h"
> +#include "test-driver.h"

OK.

> +
> +int verbose = 0;
> +
> +/* Running a test in a container is tricky.  There are two main
> +   categories of things to do:
> +
> +   1. "Once" actions, like setting up the container and doing an
> +      install into it.
> +
> +   2. "Per-test" actions, like copying in support files and
> +      configuring the container.

OK.

> +
> +
> +   "Once" actions:
> +
> +   * mkdir $buildroot/testroot.pristine/
> +   * install into it
> +   * rsync to $buildroot/testroot.root/

Ok.

> +
> +   "Per-test" actions:
> +   * maybe rsync to $buildroot/testroot.root/
> +   * copy support files and test binary
> +   * chroot/unshare
> +   * set up any mounts (like /proc)

OK.

> +
> +   Magic files:
> +
> +   For test $srcdir/foo/mytest.c we look for $srcdir/foo/mytest.root and, if found...
> +
> +   * mytest.root/ is rsync'd into container
> +   * mytest.root/preclean.txt causes fresh rsync (with delete) before test if present

Call this preclean.req, since it's not a text file (see comments above), but is
a stamp file that requests a preclean of the sysroot. We can in the future add
more of these *.req files and having a consistent name for them is useful.

> +   * mytest.root/files.txt has a list of files to copy - TBD

Call this mytest.in, since it's a fragment of shell to run. Remove "TBD".

> +     ($B/ or $S/ for build and source directories)
> +       syntax:
> +         # comment
> +         mv FILE FILE
> +	 cp FILE FILE
> +	 rm FILE
> +	 FILE must start with $B/, $S/, $I/, $L/, or /
> +	  (expands to build dir, source dir, install dir, library dir
> +	   (in container), or container's root)
> +   * mytest.root/postclean.txt causes fresh rsync (with delete) after test if present

Likewise: postclean.req.

> +
> +   Note that $srcdir/foo/mytest.files may be used instead of a
> +   files.txt in the sysroot, if there is no other reason for a sysroot.

s/mytest.files/mytest.in/g
s/files.txt/files.in/g

> +
> +   Design goals:
> +
> +   * independent of other packages which may not be installed (like
> +     rsync or Docker, or even "cp")
> +

OK. Absolutely.

> +   * Simple, easy to review code (i.e. prefer simple naive code over
> +     complex efficient code)

OK. Yup.

> +
> +   TBD:
> +
> +   * The current implementation is not parallel-make-safe, as one test
> +     could be modifying the chroot while another is running against
> +     it.

I don't believe this is true anymore. I see you are using flock LOCK_EX
to get an exclusive lock on the sysroot, which means that the tests
will serialize against the container. We can in the future perhaps allow
like N containers to be instantiated at a time and iterate through them.
For now this is perfect.

> +
> +*/

GNU Formatting, bring hte '*/' up a line e.g. '... it.  */'

> +
> +/*--------------------------------------------------*/

Remove line.

> +/* Utility Functions */
> +
> +/* Temporarily concatenate multiple strings into one.  Allows up to 10
> +   temporary results; use strdup () if you need them to be
> +   permanent.  */
> +

Remove newline. Comment should be up gainst the function it defines.

> +static char *
> +concat (const char *str, ...)
> +{
> +  /* Assume initialized to NULL/zero.  */
> +  static char *bufs[10];
> +  static size_t buflens[10];
> +  static int bufn = 0;
> +  int n;
> +  size_t len;
> +  va_list ap, ap2;
> +  char *cp;
> +  char *next;
> +
> +  va_start (ap, str);
> +  va_copy (ap2, ap);
> +
> +  n = bufn;
> +  bufn = (bufn + 1) % 10;
> +  len = strlen (str);
> +
> +  while ((next = va_arg (ap, char *)) != NULL)
> +    len = len + strlen (next);
> +
> +  va_end (ap);
> +
> +  if (bufs[n] == NULL)
> +    {
> +      bufs[n] = malloc (len + 1); /* NUL */
> +      buflens[n] = len + 1;
> +    }
> +  else if (buflens[n] < len + 1)
> +    {
> +      bufs[n] = realloc (bufs[n], len + 1); /* NUL */
> +      buflens[n] = len + 1;
> +    }
> +
> +  strcpy (bufs[n], str);
> +  cp = strchr (bufs[n], '\0');
> +  while ((next = va_arg (ap2, char *)) != NULL)
> +    {
> +      strcpy (cp, next);
> +      cp = strchr (cp, '\0');
> +    }
> +  *cp = 0;
> +  va_end (ap2);
> +
> +  return bufs[n];
> +}

OK.

> +
> +/* Try to mount SRC onto DEST.  */
> +

Remove newline.

> +static void
> +trymount (const char *src, const char *dest)
> +{
> +  if (mount (src, dest, "", MS_BIND, NULL) < 0)
> +    FAIL_UNSUPPORTED ("can't mount %s onto %s\n", src, dest);

OK.

> +}
> +
> +/* Special case of above for devices like /dev/zero where we have to
> +   mount a device over a device, not a directory over a directory.  */
> +

Remove newline.

> +static void
> +devmount (const char *new_root_path, const char *which)
> +{
> +  int fd;
> +  fd = open (concat (new_root_path, "/dev/", which, NULL),
> +	     O_CREAT | O_TRUNC | O_RDWR, 0777);
> +  close (fd);
> +
> +  trymount (concat ("/dev/", which, NULL),
> +	    concat (new_root_path, "/dev/", which, NULL));
> +}

OK.

> +
> +/* Returns true if the string "looks like" an environement variable
> +   being set.  */
> +

Remove newline.

> +static int
> +is_env_setting (const char *a)
> +{
> +  int count_name = 0;
> +
> +  while (*a)
> +    {
> +      if (isalnum (*a) || *a == '_')
> +	++count_name;
> +      else if (*a == '=' && count_name > 0)
> +	return 1;
> +      else
> +	return 0;
> +      ++a;
> +    }
> +  return 0;
> +}

OK.

> +
> +/* Break the_line into words and store in the_words.  Max nwords,
> +   returns actual count.  */
> +static int
> +tokenize (char *the_line, char **the_words, int nwords)
> +{
> +  int rv = 0;
> +
> +  while (nwords > 0)
> +    {
> +      /* Skip leading whitespace, if any.  */
> +      while (*the_line && isspace (*the_line))
> +	++the_line;
> +
> +      /* End of line?  */
> +      if (*the_line == 0)
> +	return rv;
> +
> +      /* THE_LINE points to a non-whitespace character, so we have a
> +	 word.  */
> +      *the_words = the_line;
> +      ++the_words;
> +      nwords--;
> +      ++rv;
> +
> +      /* Skip leading whitespace, if any.  */
> +      while (*the_line && ! isspace (*the_line))
> +	++the_line;
> +
> +      /* We now point at the trailing NUL *or* some whitespace.  */
> +      if (*the_line == 0)
> +	return rv;
> +
> +      /* It was whitespace, skip and keep tokenizing.  */
> +      *the_line++ = 0;
> +    }
> +
> +  /* We get here if we filled the words buffer.  */
> +  return rv;
> +}

OK.

> +
> +/*--------------------------------------------------*/

Remove line.

> +/* mini-RSYNC implementation.  Optimize later.      */
> +
> +/* Set this to 1 if you need to debug the rsync function.  */
> +#define RTRACE 0

Not OK. Should always be on to avoid bitrot. Add an argument to the
container to turn on debug tracing.

> +
> +/* A few routines for an "rsync buffer" which stores the paths we're
> +   working on.  We continuously grow and shrink the paths in each
> +   buffer so there's lot of re-use.  */
> +
> +/* We rely on "initialized to zero" to set these up.  */
> +typedef struct
> +{
> +  char *buf;
> +  size_t len;
> +  size_t size;
> +} path_buf;
> +
> +static path_buf spath, dpath;
> +
> +static void
> +r_setup (char *path, path_buf * pb)
> +{
> +  size_t len = strlen (path);
> +  if (pb->buf == NULL || pb->size < len + 1)
> +    {
> +      /* Round up */
> +      size_t sz = ALIGN_UP (len + 1, 512);
> +      if (pb->buf == NULL)
> +	pb->buf = (char *) malloc (sz);
> +      else
> +	pb->buf = (char *) realloc (pb->buf, sz);
> +      if (pb->buf == NULL)
> +	FAIL_UNSUPPORTED ("Out of memory while rsyncing\n");
> +
> +      pb->size = sz;
> +    }
> +  strcpy (pb->buf, path);
> +  pb->len = len;
> +}

OK.

> +
> +static void
> +r_append (const char *path, path_buf * pb)
> +{
> +  size_t len = strlen (path) + pb->len;
> +  if (pb->size < len + 1)
> +    {
> +      /* Round up */
> +      size_t sz = ALIGN_UP (len + 1, 512);

Why 512?

> +      pb->buf = (char *) realloc (pb->buf, sz);
> +      if (pb->buf == NULL)
> +	FAIL_UNSUPPORTED ("Out of memory while rsyncing\n");
> +
> +      pb->size = sz;
> +    }
> +  strcpy (pb->buf + pb->len, path);
> +  pb->len = len;
> +}

OK.

> +
> +static int
> +file_exists (char *path)
> +{
> +  struct stat st;
> +  if (lstat (path, &st) == 0)
> +    return 1;
> +  return 0;
> +}

OK.

> +
> +static void
> +recursive_remove (char *path)
> +{
> +  pid_t child;
> +  int status;
> +  /* FIXME: re-implement without external dependencies at some point.
> +     Fortunately, this runs outside the container.  */

Recomve comment. I don't think this needs fixing.

> +
> +  child = fork ();
> +
> +  switch (child) {
> +  case -1:
> +    FAIL_UNSUPPORTED ("Unable to fork");
> +  case 0:
> +    /* Child.  */
> +    execlp ("rm", "rm", "-rf", path, NULL);
> +  default:
> +    /* Parent.  */
> +    waitpid (child, &status, 0);
> +    break;

No. You need to wait to get the status of the child and this function
should return an error that you check. This way if it's used inside
a container it fails and you detect that and fail the test.

> +  }
> +}

OK. Dependency on coreutils when outside of the container is OK.

> +
> +/* Used for both rsync and the files.txt "cp" command.  */

s/files.txt/mytest.in/g

> +
> +static void
> +copy_one_file (const char *sname, const char *dname)
> +{
> +  int sfd, dfd;
> +  struct stat st;
> +  struct utimbuf times;
> +
> +  sfd = open (sname, O_RDONLY);
> +  if (sfd < 0)
> +    FAIL_EXIT1 ("unable to open %s for reading\n", sname);
> +
> +  if (fstat (sfd, &st) < 0)
> +    FAIL_EXIT1 ("unable to fstat %s\n", sname);
> +
> +  dfd = open (dname, O_WRONLY | O_TRUNC | O_CREAT, 0600);
> +  if (dfd < 0)
> +    FAIL_EXIT1 ("unable to open %s for writing\n", dname);
> +
> +  if (copy_file_range (sfd, 0, dfd, 0, st.st_size, 0) != st.st_size)
> +    FAIL_EXIT1 ("cannot copy file %s to %s\n", sname, dname);
> +
> +  close (sfd);
> +  close (dfd);

Use xclose.

> +
> +  chmod (dname, st.st_mode & 0777);

Check for chmod failure. Or add xchmod.

> +
> +  times.actime = st.st_atime;
> +  times.modtime = st.st_mtime;
> +  utime (dname, &times);

Check for utime failure. Or add xutime.

> +}

OK.

> +
> +/* We don't check *everything* about the two files to see if a copy is
> +   needed, just the minimum to make sure we get the latest copy.  */
> +static int
> +need_sync (char *ap, char *bp, struct stat *a, struct stat *b)
> +{
> +  if ((a->st_mode & S_IFMT) != (b->st_mode & S_IFMT))
> +    return 1;
> +
> +  if (S_ISLNK (a->st_mode))
> +    {
> +      int rv;
> +      char *al, *bl;
> +
> +      if (a->st_size != b->st_size)
> +	return 1;
> +
> +      al = (char *) malloc (a->st_size + 1);
> +      bl = (char *) malloc (b->st_size + 1);

Use xmalloc.

> +      readlink (ap, al, a->st_size + 1);
> +      readlink (bp, bl, b->st_size + 1);

Use xreadlink.

> +      al[a->st_size] = 0;
> +      bl[b->st_size] = 0;
> +      rv = strcmp (al, bl);
> +      free (al);
> +      free (bl);

Check for free failure. Or add xfree. Just kidding ;-)

> +      if (rv == 0)
> +	return 0; /* links are same */
> +      return 1; /* links differ */
> +    }
> +
> +#if RTRACE
> +  if (a->st_size != b->st_size)
> +    printf ("SIZE\n");
> +  if ((a->st_mode & 0777) != (b->st_mode & 0777))
> +    printf ("MODE\n");
> +  if (a->st_mtime != b->st_mtime)
> +    printf ("TIME\n");
> +#endif
> +
> +  if (a->st_size == b->st_size
> +      && ((a->st_mode & 0777) == (b->st_mode & 0777))
> +      && a->st_mtime == b->st_mtime)
> +    return 0;
> +
> +  return 1;

OK.

> +}
> +
> +static void
> +rsync_1 (path_buf * src, path_buf * dest, int and_delete)
> +{
> +  DIR *dir;
> +  struct dirent *de;
> +  struct stat s, d;
> +
> +  r_append ("/", src);
> +  r_append ("/", dest);
> +#if RTRACE
> +  printf ("sync %s to %s %s\n", src->buf, dest->buf,
> +	  and_delete ? "and delete" : "");
> +#endif
> +
> +  size_t staillen = src->len;
> +
> +  size_t dtaillen = dest->len;
> +
> +  dir = opendir (src->buf);
> +
> +  while ((de = readdir (dir)) != NULL)
> +    {
> +      if (strcmp (de->d_name, ".") == 0
> +	  || strcmp (de->d_name, "..") == 0)
> +	continue;
> +
> +      src->len = staillen;
> +      r_append (de->d_name, src);
> +      dest->len = dtaillen;
> +      r_append (de->d_name, dest);
> +
> +      s.st_mode = ~0;
> +      d.st_mode = ~0;
> +
> +      if (lstat (src->buf, &s) != 0)
> +	FAIL_EXIT1 ("%s obtained by readdir, but stat failed.\n", src->buf);
> +
> +      // It's OK if this one fails, since we know the file might be missing.

Use C comment. /* */.

> +      lstat (dest->buf, &d);
> +
> +      if (! need_sync (src->buf, dest->buf, &s, &d))
> +	{
> +	  if (S_ISDIR (s.st_mode))
> +	    rsync_1 (src, dest, and_delete);
> +	  continue;
> +	}

OK.

> +
> +      if (d.st_mode != ~0)
> +	switch (d.st_mode & S_IFMT)
> +	  {
> +	  case S_IFDIR:
> +	    if (!S_ISDIR (s.st_mode))
> +	      {
> +#if RTRACE
> +		printf ("-D %s\n", dest->buf);
> +#endif
> +		recursive_remove (dest->buf);

Check for failure to remove.

> +	      }
> +	    break;
> +
> +	  default:
> +#if RTRACE
> +	    printf ("-F %s\n", dest->buf);
> +#endif
> +	    unlink (dest->buf);

Use xunlink.

> +	    break;
> +	  }
> +
> +      switch (s.st_mode & S_IFMT)
> +	{
> +	case S_IFREG:
> +#if RTRACE
> +	  printf ("+F %s\n", dest->buf);
> +#endif
> +	  copy_one_file (src->buf, dest->buf);
> +	  break;
> +
> +	case S_IFDIR:
> +#if RTRACE
> +	  printf ("+D %s\n", dest->buf);
> +#endif
> +	  mkdir (dest->buf, (s.st_mode & 0777) | 0700);

Use xmkdir.

> +	  rsync_1 (src, dest, and_delete);
> +	  break;
> +
> +	case S_IFLNK:
> +	  {
> +	    char *lp = (char *) malloc (s.st_size + 1);
> +#if RTRACE
> +	    printf ("+L %s\n", dest->buf);
> +#endif
> +	    readlink (src->buf, lp, s.st_size + 1);

Use xreadlink.

> +	    lp[s.st_size] = 0;
> +	    symlink (lp, dest->buf);

Add xsymlink.

> +	    free (lp);
> +	    break;
> +	  }
> +
> +	default:
> +	  break;
> +	}
> +    }
> +
> +  closedir (dir);
> +  src->len = staillen;
> +  src->buf[staillen] = 0;
> +  dest->len = dtaillen;
> +  dest->buf[dtaillen] = 0;

OK.

> +
> +  if (!and_delete)
> +    return;
> +
> +  /* The rest of this function removes any files/directories in DEST
> +     that do not exist in SRC.  This is triggered as part of a
> +     preclean or postsclean step.  */
> +
> +  dir = opendir (dest->buf);
> +
> +  while ((de = readdir (dir)) != NULL)
> +    {
> +      if (strcmp (de->d_name, ".") == 0
> +	  || strcmp (de->d_name, "..") == 0)
> +	continue;
> +
> +      src->len = staillen;
> +      r_append (de->d_name, src);
> +      dest->len = dtaillen;
> +      r_append (de->d_name, dest);
> +
> +      s.st_mode = ~0;
> +      d.st_mode = ~0;
> +
> +      lstat (src->buf, &s);
> +      
> +      if (lstat (dest->buf, &d) != 0)
> +	FAIL_EXIT1 ("%s obtained by readdir, but stat failed.\n", dest->buf);
> +
> +      if (s.st_mode == ~0)
> +	{
> +	  /* dest exists and src doesn't, clean it.  */
> +	  switch (d.st_mode & S_IFMT)
> +	    {
> +	    case S_IFDIR:
> +	      if (!S_ISDIR (s.st_mode))
> +		{
> +#if RTRACE
> +		  printf ("-D %s\n", dest->buf);
> +#endif
> +		  recursive_remove (dest->buf);

Check for failure to remove.

> +		}
> +	      break;
> +
> +	    default:
> +#if RTRACE
> +	      printf ("-F %s\n", dest->buf);
> +#endif
> +	      unlink (dest->buf);

Use xunlink.

> +	      break;
> +	    }
> +	}
> +    }
> +
> +  closedir (dir);
> +}
> +
> +static void
> +rsync (char *src, char *dest, int and_delete)
> +{
> +  r_setup (src, &spath);
> +  r_setup (dest, &dpath);
> +
> +  rsync_1 (&spath, &dpath, and_delete);

OK.

> +}
> +
> +/*--------------------------------------------------*/

Remove line.

> +/* Main */

Remove comment.

> +
> +int
> +main (int argc, char **argv)
> +{
> +  pid_t child;
> +  char *pristine_root_path;
> +  char *new_root_path;
> +  char *new_cwd_path;
> +  char *new_objdir_path;
> +  char *new_srcdir_path;
> +  char **new_child_proc;
> +  char *command_root;
> +  char *command_base;
> +  char *so_base;
> +  int do_postclean = 0;
> +
> +  uid_t original_uid;
> +  gid_t original_gid;
> +  int UMAP;
> +  int GMAP;
> +  char tmp[100];

Is this a limit that needs to be documented?

> +  struct stat st;
> +  int lock_fd;
> +
> +  setbuf (stdout, NULL);
> +
> +  // The command line we're expecting looks like this:
> +  // env <set some vars> ld.so <library path> test-binary
> +
> +  // We need to peel off any "env" or "ld.so" portion of the command
> +  // line, and keep track of which env vars we should preserve and
> +  // which we drop.

Use C comment. /* */.

> +
> +  if (argc < 2)
> +    {
> +      fprintf (stderr, "Usage: containerize <program to run> <args...>\n");
> +      exit (1);
> +    }
> +
> +  if (strcmp (argv[1], "-v") == 0)
> +    {
> +      verbose = 1;
> +      ++argv;
> +      --argc;
> +    }
> +
> +  if (strcmp (argv[1], "env") == 0)
> +    {
> +      ++argv;
> +      --argc;
> +      while (is_env_setting (argv[1]))
> +	{
> +	  // List variables we do NOT want to propogate.

Use C comment. /* */.


> +#if 0
> +	  // until we discover why locale/iconv tests don't
> +	  // work against an installed tree...

Use C comment. /* */.


> +	  if (memcmp (argv[1], "GCONV_PATH=", 11)
> +	      && memcmp (argv[1], "LOCPATH=", 8))
> +#endif
> +	    {
> +	      // Need to keep these.  Note that putenv stores a
> +	      // pointer to our argv.

Use C comment. /* */.

> +	      putenv (argv[1]);
> +	    }
> +	  ++argv;
> +	  --argc;
> +	}
> +    }
> +
> +  if (strncmp (argv[1], concat (support_objdir_root, "/elf/ld-linux-", NULL),
> +	       strlen (support_objdir_root) + 14) == 0)
> +    {
> +      ++argv;
> +      --argc;
> +      while (argv[1][0] == '-')
> +	{
> +	  if (strcmp (argv[1], "--library-path") == 0)
> +	    {
> +	      ++argv;
> +	      --argc;
> +	    }
> +	  ++argv;
> +	  --argc;
> +	}
> +    }

OK.

> +
> +  pristine_root_path = strdup (concat (support_objdir_root, "/testroot.pristine", NULL));
> +  new_root_path = strdup (concat (support_objdir_root, "/testroot.root", NULL));
> +  new_cwd_path = get_current_dir_name ();
> +  new_child_proc = argv + 1;
> +
> +  lock_fd = open (concat (pristine_root_path, "/lock.fd", NULL),
> +		 O_CREAT | O_TRUNC | O_RDWR, 0666);
> +  if (lock_fd < 0)
> +    FAIL_UNSUPPORTED ("Cannot create testroot lock.\n");
> +
> +  while (flock (lock_fd, LOCK_EX) != 0)
> +    {
> +      if (errno != EINTR)
> +	FAIL_UNSUPPORTED ("Cannot lock testroot.\n");
> +    }

OK. this make it parallel safe by serializing.

> +
> +  xmkdirp (new_root_path, 0755);
> +
> +  /* We look for extra setup info in a subdir in the same spot as the
> +     test, with the same name but a ".root" extension.  This is that
> +     directory.  We try to look in the source tree if the path we're
> +     given refers to the build tree, but we rely on the path to be
> +     absolute.  This is what the glibc makefiles do.  */
> +  command_root = concat (argv[1], ".root", NULL);
> +  if (strncmp (command_root, support_objdir_root, strlen (support_objdir_root)) == 0
> +      && command_root[strlen (support_objdir_root)] == '/')
> +    command_root = concat (support_srcdir_root, argv[1] + strlen (support_objdir_root), ".root", NULL);
> +  command_root = strdup (command_root);
> +
> +  /* This cuts off the ".root" we appended above.  */
> +  command_base = strdup (command_root);
> +  command_base[strlen (command_base) - 5] = 0;
> +
> +  /* Shared object base directory.  */
> +  so_base = strdup (argv[1]);
> +  if (strrchr (so_base, '/') != NULL)
> +    strrchr (so_base, '/')[1] = 0;
> +
> +  if (file_exists (concat (command_root, "/postclean.txt", NULL)))
> +    do_postclean = 1;
> +
> +  rsync (pristine_root_path, new_root_path,
> +	 1 || file_exists (concat (command_root, "/preclean.txt", NULL)));
> +
> +  if (stat (command_root, &st) >= 0
> +      && S_ISDIR (st.st_mode))
> +    rsync (command_root, new_root_path, 0);
> +
> +  new_objdir_path = strdup (concat (new_root_path, support_objdir_root, NULL));
> +  new_srcdir_path = strdup (concat (new_root_path, support_srcdir_root, NULL));
> +
> +  /* new_cwd_path starts with '/' so no "/" needed between the two.  */
> +  xmkdirp (concat (new_root_path, new_cwd_path, NULL), 0755);
> +  xmkdirp (new_srcdir_path, 0755);
> +  xmkdirp (new_objdir_path, 0755);
> +
> +  original_uid = getuid ();
> +  original_gid = getgid ();
> +
> +  /* Handle the cp/mv/rm "script" here.  */
> +  {
> +    char *the_line = NULL;
> +    size_t line_len = 0;
> +    char *fname = concat (command_root, "/files.txt", NULL);

s/files.txt/files.in/g

> +    char *the_words[3];
> +    FILE *f = fopen (fname, "r");
> +
> +    if (verbose && f)
> +      fprintf (stderr, "reading %s\n", fname);
> +
> +    if (f == NULL)
> +      {
> +	/* Try foo.files instead of foo.root/files.txt, as a shortcut.  */

s/files.txt/files.in/g

> +	fname = concat (command_base, ".files", NULL);
> +	f = fopen (fname, "r");
> +	if (verbose && f)
> +	  fprintf (stderr, "reading %s\n", fname);
> +      }
> +
> +#if 0
> +    /* I don't want to add this until we know we need it, but here it
> +       is...  */
> +    if (f == NULL)
> +      {
> +	/* Look for a Makefile-generated one also.  */
> +	fname = concat (argv[1], ".files", NULL);
> +	f = fopen (fname, "r");
> +      }
> +#endif

I'd rather see a bigger comment explaining what might be missing, and remove
the #if 0 / #endif code.

> +
> +    /* This is where we "interpret" the mini-script which is <test>.files.  */

OK.

> +    if (f != NULL)
> +      {
> +	while (getline (&the_line, &line_len, f) > 0)
> +	  {
> +	    int nt = tokenize (the_line, the_words, 3);
> +	    int i;
> +
> +	    for (i = 1; i < nt; ++i)
> +	      {
> +		if (memcmp (the_words[i], "$B/", 3) == 0)
> +		  the_words[i] = concat (support_objdir_root, the_words[i] + 2, NULL);
> +		else if (memcmp (the_words[i], "$S/", 3) == 0)
> +		  the_words[i] = concat (support_srcdir_root, the_words[i] + 2, NULL);
> +		else if (memcmp (the_words[i], "$I/", 3) == 0)
> +		  the_words[i] = concat (new_root_path, support_install_prefix, the_words[i] + 2, NULL);
> +		else if (memcmp (the_words[i], "$L/", 3) == 0)
> +		  the_words[i] = concat (new_root_path, support_libdir_prefix, the_words[i] + 2, NULL);
> +		else if (the_words[i][0] == '/')
> +		  the_words[i] = concat (new_root_path, the_words[i], NULL);
> +	      }
> +
> +	    if (nt == 3 && the_words[2][strlen (the_words[2]) - 1] == '/')
> +	      {
> +		char *r = strrchr (the_words[1], '/');
> +		if (r)
> +		  the_words[2] = concat (the_words[2], r + 1, NULL);
> +		else
> +		  the_words[2] = concat (the_words[2], the_words[1], NULL);
> +	      }
> +
> +	    if (nt == 2 && strcmp (the_words[0], "so") == 0)
> +	      {
> +		the_words[2] = concat (new_root_path, support_libdir_prefix, "/", the_words[1], NULL);
> +		the_words[1] = concat (so_base, the_words[1], NULL);
> +		copy_one_file (the_words[1], the_words[2]);
> +	      }
> +	    else if (nt == 3 && strcmp (the_words[0], "cp") == 0)
> +	      {
> +		copy_one_file (the_words[1], the_words[2]);
> +	      }
> +	    else if (nt == 3 && strcmp (the_words[0], "mv") == 0)
> +	      {
> +		rename (the_words[1], the_words[2]);

Check for rename failure or add xrename.

> +	      }
> +	    else if (nt == 3 && strcmp (the_words[0], "chmod") == 0)
> +	      {
> +		long int m;
> +		m = strtol (the_words[1], NULL, 0);
> +		chmod (the_words[2], m);

Check for chmod failure or add xchmod.

> +	      }
> +	    else if (nt == 2 && strcmp (the_words[0], "rm") == 0)
> +	      {
> +		unlink (the_words[1]);

Use xunlink.

> +	      }
> +	    else if (nt > 0 && the_words[0][0] != '#')
> +	      {
> +		printf ("\033[31minvalid [%s]\033[0m\n", the_words[0]);
> +	      }
> +	  }
> +	fclose (f);
> +      }

OK.

> +  }
> +
> +  // The unshare here gives us our own spaces and capabilities.

Use C comment. /* */.

> +  if (unshare (CLONE_NEWUSER | CLONE_NEWPID | CLONE_NEWNS) < 0)
> +    FAIL_UNSUPPORTED ("unable to unshare user/fs, ");
> +
> +  /* Some systems, by default, all mounts leak out of the namespace.  */
> +  if (mount ("none", "/", NULL, MS_REC | MS_PRIVATE, NULL) != 0)
> +    FAIL_UNSUPPORTED ("could not create a private mount namespace\n");
> +
> +  trymount (support_srcdir_root, new_srcdir_path);
> +  trymount (support_objdir_root, new_objdir_path);
> +
> +  xmkdirp (concat (new_root_path, "/dev", NULL), 0755);
> +  devmount (new_root_path, "null");
> +  devmount (new_root_path, "zero");
> +  devmount (new_root_path, "urandom");
> +
> +  // We're done with the "old" root, switch to the new one.

Use C comment. /* */.

> +  if (chroot (new_root_path) < 0)
> +    FAIL_UNSUPPORTED ("Can't chroot to %s - ", new_root_path);
> +
> +  if (chdir (new_cwd_path) < 0)
> +    FAIL_UNSUPPORTED ("Can't cd to new %s - ", new_cwd_path);

OK.

> +
> +  /* To complete the containerization, we need to fork () at least
> +     once.  We can't exec, nor can we somehow link the new child to
> +     our parent.  So we run the child and propogate it's exit status
> +     up.  */
> +  child = fork ();
> +  if (child < 0)
> +    FAIL_UNSUPPORTED ("Unable to fork");
> +  else if (child > 0)
> +    {
> +      /* Parent.  */
> +      int status;
> +      waitpid (child, &status, 0);
> +
> +      /* There's a bit of magic here, since the buildroot is mounted
> +	 in our space, the paths are still valid, and since the mounts
> +	 aren't recursive, it sees *only* the built root, not anything
> +	 we would normally se if we rsync'd to "/" like mounted /dev
> +	 files.  */
> +      if (do_postclean)
> +	  rsync (pristine_root_path, new_root_path, 1);
> +
> +      if (WIFEXITED (status))
> +	exit (WEXITSTATUS (status));
> +
> +      if (WIFSIGNALED (status))
> +	{
> +	  printf ("%%SIGNALLED%%\n");
> +	  exit (77);
> +	}
> +
> +      printf ("%%EXITERROR%%\n");
> +      exit (78);
> +    }
> +
> +  /* The rest is the child process, which is now PID 1 and "in" the
> +     new root.  */
> +
> +  mkdir ("/tmp", 0755);

Use xmkdir.

> +
> +  // Now that we're pid 1 (effectively "root") we can mount /proc

Use C comment. /* */. Full sentences if please.

> +  mkdir ("/proc", 0777);

Use xmkdir.

> +  if (mount ("proc", "/proc", "proc", 0, NULL) < 0)
> +    FAIL_UNSUPPORTED ("Unable to mount /proc: ");
> +
> +  // We map our original UID to the same UID in the container so we
> +  // can own our own files normally

Use C comment. /* */. Full sentences if please.

> +  UMAP = open ("/proc/self/uid_map", O_WRONLY);
> +  if (UMAP < 0)
> +    FAIL_UNSUPPORTED ("can't write to /proc/self/uid_map\n");
> +
> +  sprintf (tmp, "%lld %lld 1\n", (long long) original_uid, (long long) original_uid);
> +  write (UMAP, tmp, strlen (tmp));
> +  close (UMAP);
> +
> +  // We must disable setgroups () before we can map our groups, else we
> +  // get EPERM.

Use C comment. /* */.

> +  GMAP = open ("/proc/self/setgroups", O_WRONLY);
> +  if (GMAP >= 0)
> +    {
> +      /* We support kernels old enough to not have this.  */
> +      write (GMAP, "deny\n", 5);
> +      close (GMAP);
> +    }
> +
> +  // We map our original GID to the same GID in the container so we
> +  // can own our own files normally

Use C comment. /* */. Full sentences if please.

> +  GMAP = open ("/proc/self/gid_map", O_WRONLY);
> +  if (GMAP < 0)
> +    FAIL_UNSUPPORTED ("can't write to /proc/self/gid_map\n");
> +
> +  sprintf (tmp, "%lld %lld 1\n", (long long) original_gid, (long long) original_gid);
> +  write (GMAP, tmp, strlen (tmp));
> +  close (GMAP);
> +
> +  // Now run the child

Use C comment. /* */.

> +  execvp (new_child_proc[0], new_child_proc);
> +
> +  // Or don't run the child?

Use C comment. /* */.

> +  FAIL_EXIT1 ("Unable to exec %s\n", new_child_proc[0]);
> +
> +  // Because gcc won't know error () never returns...

Use C comment. /* */.

> +  exit (EXIT_UNSUPPORTED);
> +}

OK.

> diff --git a/support/true-container.c b/support/true-container.c
> new file mode 100644
> index 0000000000..157d9c999a
> --- /dev/null
> +++ b/support/true-container.c
> @@ -0,0 +1,23 @@
> +/* Minimal /bin/true for in-container use.
> +   Copyright (C) 2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +

Add a comment explaining this implements the true command which always
returns true (0).

> +int
> +main(void)
> +{
> +  return 0;
> +}

OK.

> diff --git a/support/xmkdirp.c b/support/xmkdirp.c
> new file mode 100644
> index 0000000000..a15fddea48
> --- /dev/null
> +++ b/support/xmkdirp.c
> @@ -0,0 +1,67 @@
> +/* Error-checking replacement for "mkdir -p".
> +   Copyright (C) 2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <support/support.h>
> +#include <support/check.h>
> +
> +#include <stdarg.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +
> +/* Equivalent of "mkdir -p".  Any failures cause FAIL_EXIT1 so no
> +   return code is needed.  */

OK.

> +
> +void
> +xmkdirp (const char *path, int mode)
> +{
> +  struct stat s;
> +  const char *slash_p;
> +  int rv;
> +
> +  if (path[0] == 0)
> +    return;
> +
> +  if (stat (path, &s) == 0)
> +    {
> +      if (S_ISDIR (s.st_mode))
> +	return;
> +      errno = EEXIST;
> +      FAIL_EXIT1 ("mkdir_p (\"%s\", 0%o): %m", path, mode);
> +    }
> +
> +  slash_p = strrchr (path, '/');
> +  if (slash_p != NULL)
> +    {
> +      while (slash_p > path && slash_p[-1] == '/')
> +	--slash_p;
> +      if (slash_p > path)
> +	{
> +	  char *parent = xstrndup (path, slash_p - path);
> +	  xmkdirp (parent, mode);
> +	  free (parent);
> +	}
> +    }
> +
> +  rv = mkdir (path, mode);

OK. Fine to use mkdir here because you want a custom error code.

> +  if (rv != 0)
> +    FAIL_EXIT1 ("mkdir_p (\"%s\", 0%o): %m", path, mode);

OK.

> +
> +  return;
> +}
>