This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: V6 test-in-container patch
- From: DJ Delorie <dj at redhat dot com>
- To: "Carlos O'Donell" <carlos at redhat dot com>
- Cc: libc-alpha at sourceware dot org
- Date: Wed, 15 Aug 2018 01:04:48 -0400
- Subject: 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, ×);
>
> 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?