This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: V6 test-in-container patch


Thanks for the review; I'll post a V7 in addition to my per-change notes
here.

"Carlos O'Donell" <carlos@redhat.com> writes:
>> +# 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.

It's still technically correct, but misleading.  I replaced the first
line with:

# In addition, we have to copy some files (which we build) into this

>> 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.

I've renamed these to mytest.script to reflect the fact that it contains
commands which we "run" (it's not a shell script, but looks like one).

There's also preclean and postclean triggers, which were named
preclean.txt and postclean.txt.  I've renamed those to preclean.script
and postclean.script although they're not really scripts.

> GNU Formatting:
>
> for (i = 1; i < argc; i++)

Fixed.

>> 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.

Done.

> Likewise, use cout.

Done.

>> +/* 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?

Done.

>> +
>> +*/
>> +
>> +#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.

Done.

> Need's one-line comment about intent.

Done.

>> +      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.

Done, and replaced those with fprintfs - we always want the errors
printed :-)

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

Done.

>> +  dprintf (stderr, "dj: run_command_array\n");
>
> Suggest:
>
> dprintf (stderr, "run_command_array: starting\n");

Done.

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

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

Done throughout.

>> +	  ++ i;
>
> Likewise.
>
> ++i;

The inconsistency boggles the mind...

>> +    {
>> +      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.

Done throughout, using fprintf.

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

Done.

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

Done throughout.

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

Done throughout.

> 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).

Done.

>> +      if (*cmdline == 0)
>> +	break;
>
> This whole while statement needs comments explaining the
> logic of the algorithm.

Done.

> 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).

Done.

>> +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.

done.

> 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]);

Done.

>> +
>> +
>> +  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.

I used --debug as you mentioned before.

> 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).

--debug has to be the first argument, any -c et al follow.

>> 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?

Yup, but that adds a dependency on <sys/stat.h> in support.h, which I
added.  Seems to be 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.

Done.

>> +   * 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".

I called this mytest.script as it contains commands.

>> +   * mytest.root/postclean.txt causes fresh rsync (with delete) after test if present
>
> Likewise: postclean.req.

Yup.

>> +
>> +   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

Yup.

>> +   * 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.

Fixed.

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

Done.

>> +
>> +/*--------------------------------------------------*/
>
> Remove line.

Is there some other convention for separating major portions of a source
file?

>> +/* 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.

Done.

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

Done throughout.

>> +/* 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.

I hooked it into the verbose flag (-v).

>> +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?

Why not?  It seemed like a nice round number :-)

(it's just to avoid calling realloc too often)

>> +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.

Ok.

>> +
>> +  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.

I added this:

    /* "rm" would have already printed a suitable error message.  */
    if (! WIFEXITED (status)
	|| WEXITSTATUS (status) != 0)
      exit (1)

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

Done.

>> +
>> +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.

Done throughout.

>> +
>> +  chmod (dname, st.st_mode & 0777);
>
> Check for chmod failure. Or add xchmod.

Done.

>> +
>> +  times.actime = st.st_atime;
>> +  times.modtime = st.st_mtime;
>> +  utime (dname, &times);
>
> Check for utime failure. Or add xutime.

Done.

>> +      al = (char *) malloc (a->st_size + 1);
>> +      bl = (char *) malloc (b->st_size + 1);
>
> Use xmalloc.

Done.

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

Done throughout.

>> +      free (al);
>> +      free (bl);
>
> Check for free failure. Or add xfree. Just kidding ;-)

Oh sure, joke about this one, don't mention xopen() ;-)

>> +      // It's OK if this one fails, since we know the file might be missing.
>
> Use C comment. /* */.

In C99, // *is* a "C comment" :-)

But changed throughout ;-)

>> +	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.

That no longer returns on failure, but exits.

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

I added a maybe_xunlink:

void
maybe_xunlink (char char *path)
{
  int rv = unlink (path);
  if (rv < 0 && errno != ENOENT)
    FAIL_EXIT1 ("unlink (\"%s\"): %m", path);
}

That way, "rm file" in the script acts like "rm -f file"

>> +	  mkdir (dest->buf, (s.st_mode & 0777) | 0700);
>
> Use xmkdir.

I added a maybe_xmkdir because for most of these, the directory may
already exist, which is OK.

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

Done.

>> +		  recursive_remove (dest->buf);
>
> Check for failure to remove.

No longer returns on failure.

>> +/* Main */
>
> Remove comment.

Done.

>> +  char tmp[100];
>
> Is this a limit that needs to be documented?

It's only used to sprintf("%lld %lld 1") which works for up to 160-bit
integers.  I added a comment.


>> +	/* Try foo.files instead of foo.root/files.txt, as a shortcut.  */
>
> s/files.txt/files.in/g

s//foo.script/g ;-)

>> +#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.

Done.

>> +		rename (the_words[1], the_words[2]);
>
> Check for rename failure or add xrename.

Done.

>> +		chmod (the_words[2], m);
>
> Check for chmod failure or add xchmod.

Done.

>> +		unlink (the_words[1]);
>
> Use xunlink.

This is the one where it's OK to ENOENT.

>> +  mkdir ("/tmp", 0755);
>
> Use xmkdir.

Done.

>> +  mkdir ("/proc", 0777);
>
> Use xmkdir.

Done.

>> +/* Minimal /bin/true for in-container use.
>> +   Copyright (C) 2018 Free Software Foundation, Inc.
>> . . .
>> +   <http://www.gnu.org/licenses/>.  */
>> +
>
> Add a comment explaining this implements the true command which always
> returns true (0).

Done.  The line 1 comment doesn't count as that?


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]