This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2 5/5] linux ttyname and ttyname_r: Add tests [BZ #22145]
On Wed, 08 Nov 2017 14:43:07 -0500,
Christian Brauner wrote:
> > + if ((fd = open ("/proc/self/uid_map", O_RDWR, 0)) >= 0)
> > + {
> > + char buf;
> > + if (read (fd, &buf, 1) == 0)
> > + {
> > + char *str = xasprintf ("0 %ld 1\n", (long)orig_uid);
> > + if (write (fd, str, strlen (str)) < 0)
> > + FAIL_EXIT1 ("write (uid_map, \"%s\"): %m", str);
>
> I haven't looked at the test code a lot yet but is it generally considered ok to
> not cleanup before exit? That's mostly style but still it'd be good to clarify
> this. I usually do all the cleanup even when exiting.
Clean up the file system, or clean up memory? FS cleanup isn't
necessary because we do everything on a tmpfs in a new mount
namespace. Looking at other tests, it seems to be OK to leave memory
messy when exiting with an error.
> > + free (str);
> > + }
> > + xclose (fd);
> > + }
> > +
> > + /* Oh, but we can't set the gid map until we turn off setgroups. */
>
> That sounds like a discovery you just made. I think - nitpick - a simple:
> /* We need to set setgroups to "deny" otherwise we won't be able to write a gid
> * mapping. */
My intent was for it to read as a postscript to the previous comment,
but I'm happy to change it.
> > + }
> > +
> > + char *actual_name = actual.name
> > + ? xasprintf ("\"%s\"", actual.name)
> > + : strdup ("NULL");
>
> I'd prefer if this would be proper if () branches as this is pretty unreadable.
> Also, I'd prefer - even though newer C standards allow this - if new variables
> wouldn't be declared mid-stack especially if the function is rather small.
Sure.
> > + char *expected_name = expected.name
> > + ? xasprintf ("\"%s\"", expected.name)
> > + : strdup ("NULL");
>
> Likewise.
Ack.
> > + }
> > +
> > + char *actual_name = actual.name
> > + ? xasprintf ("\"%s\"", actual.name)
> > + : strdup ("NULL");
>
> Likewise.
Ack.
> > + char *expected_name = expected.name
> > + ? xasprintf ("\"%s\"", expected.name)
> > + : strdup ("NULL");
>
> Likewise.
Ack.
> > +static bool
> > +doit (int fd, const char *testname, struct result_r expected_r) {
(whoops, that brace needs to be on a new line)
> > + bool ret = true;
> > +
> > + printf ("info: testcase: %s\n", testname);
> > +
> > + ret = eq_ttyname (run_ttyname (fd), expected) && ret;
> > + ret = eq_ttyname_r (run_ttyname_r (fd), expected_r) && ret;
>
> Imho, this is not very nice. It looks clever but is hard to parse semantically.
> I'd prefer if the assignment from eq_ttyname{_r}() would be entangled from the
> following "&& ret" check. This is just easier to maintain.
Ok, see below.
> > + /* enable `wait`ing on grandchildren */
> > + VERIFY (prctl (PR_SET_CHILD_SUBREAPER, 1) == 0);
>
> For future reference in case you want to avoid this clutch you can also use
> clone() with CLONE_PARENT.
I wanted to do that, but unfortunately glibc's clone(2) wrapper
requires passing in the stack explicitly, which requires knowing if
the stack grows up or down; and I didn't want to have to deal with
that in this test.
> > + /* There are 3 groups of tests here. The first group throws a
> > + wrench into the works in a generic way, and verifies that ttyname
>
> That's using a pretty (for a non-native speaker) specific English idiom. Could
> you maybe simply say "creates problems" or something similar.
Ok.
> > + copes correctly. The remaining groups are increasingly
> > + convoluted, as we direct that wrench towards specific parts of
>
> Likewise.
Ok.
> > + {
> > + char *linkname = xasprintf ("/proc/self/fd/%d", slave);
> > + char *target = xreadlink (linkname);
> > + free (linkname);
> > + /* Depeding on how we set up the chroot, the kernel may or may not
> > + * trim the leading path to the target (it may give us "/6",
>
> Uhm? I'm sorry, I don't understand. That sounds like a bug we fixed a while back
> upstream whereby the kernel recorded the wrong vfs mount for devpts. That's one
> scenario where this can happen. In any case the comment should outline when the
> kernel can *legitimately* show you the contents of the symlink as /<n> instead
> of the correct /dev/pts/<n>. Right now it sounds like this code doesn't
> understand how such a scenario can happen but only that it can happen.
You're right, this comment should be clearer. The test has two
chroot-setup routines, and runs this code in each. One of the setup
routines does it in a way that results in /<n> and the other does it
in a way that results in /dev/pts/<n>; this code doesn't have context
to know which of the setup routines was called.
>
> > + * instead of "/dev/pts/6"). This test group relies on the target
> > + * existing, so guarantee that (so create a file at "/6" if
> > + * necessary). */
> > + if (stat (target, &st) < 0)
> > + {
> > + VERIFY (errno == ENOENT);
> > + xtouch (target, 0);
> > + }
> > +
> > + VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
> > + VERIFY (mount ("/console", target, NULL, MS_BIND, NULL) == 0);
> > + ok = doit (slave, "with readlink target",
> > + (struct result_r){.name=target, .ret=0, .err=0}) && ok;
> > + VERIFY (umount (target) == 0);
> > + VERIFY (umount ("/dev/console") == 0);
> > +
> > + VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
> > + VERIFY (mount (slavename, target, NULL, MS_BIND, NULL) == 0);
> > + ok = doit (slave, "with readlink trap; fallback",
> > + (struct result_r){.name="/dev/console", .ret=0, .err=0}) && ok;
> > + VERIFY (umount (target) == 0);
> > + VERIFY (umount ("/dev/console") == 0);
> > +
> > + VERIFY (mount (slavename, target, NULL, MS_BIND, NULL) == 0);
> > + ok = doit (slave, "with readlink trap; no fallback",
> > + (struct result_r){.name=NULL, .ret=ENODEV, .err=ENODEV}) && ok;
>
> Same comment as above: Imho, this is not very nice. It looks clever but is hard
> to parse semantically. I'd prefer if the assignment would be entangled from the
> following "&& ret" check. This is just easier to maintain.
How would you rather it be written?
static void
doit (..., bool *ok)
{
...
if (!eq_ttyname (run_ttyname (fd), expected))
*ok = false;
if (!eq_ttyname_r (run_ttyname_r (fd), expected_r))
*ok = false;
}
doit(..., &ok);
?
> > + VERIFY (umount (target) == 0);
> > + }
> > +
> > + /* This test is extra-convoluted because we need to control the
> > + order of files within the directory. So, just create 3 files,
> > + then use opendir/readdir to see what order they are in, and
> > + assign meaning based on that order, not by name. This assumes
>
> Why do you want to assign meaning based on order not name? If there's a
> legitimate reason please mention it.
Reason: for this test "we need to control the order of files within
the directory."
Conceptually, the test is making sure that an pseudo-match from the
wrong devpts that readdir() finds before it finds the real match
doesn't mess anything up.
To test that, we need to set up the pseudo-match and the actual match
such that readdir finds the pseudo-match first. There's no good way
to control the order that readdir will find the files in (maybe I
should look in to the whitebox testing infrastructure?). So we create
two files, and whichever one readdir finds first, we assign that one
to be the pseudo-match; and whichever one readdir finds second, we
assign that one to be the actual match.
(We also test that readdir finding a pseudo-match after the actual
match doesn't mess anything up either, hence 3 files instead of 2.)
> > + ok = doit (slave, "with search-path trap",
> > + (struct result_r){.name=c[1], .ret=0, .err=0}) && ok;
>
> Same comment as above: Imho, this is not very nice. It looks clever but is hard
> to parse semantically. I'd prefer if the assignment would be entangled from the
> following "&& ret" check. This is just easier to maintain.
See above.
--
Happy hacking,
~ Luke Shumaker