Re: V6 test-in-container patch

[Carlos O'Donell <carlos at redhat dot com>]

On 08/15/2018 01:04 AM, DJ Delorie wrote:
> 
> 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

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

Sounds good to me. It's a bit of a bikeshed, but I think *.txt was misleading.

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

Any reason we don't just add a "preclean" or "postclean" command to the script
language used in the *.script file and process it that way?

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

Sounds good to me.
 
>> 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.

Perfect. Simple is fine.

>> Call this mytest.in, since it's a fragment of shell to run. Remove "TBD".
> 
> I called this mytest.script as it contains commands.

Sounds good per comments above.
 
> Is there some other convention for separating major portions of a source
> file?

Yes. A new file. If the file gets too big slipt logical utility code into
other sources files and build them separately or include the C code.
 
>> 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).

Thanks, that's fine too.

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

Add a comment that the number is randomly chosen to avoid calling
realloc too often. We want to guide future readers by stating the
intent of your choice.

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

OK.

>> Check for free failure. Or add xfree. Just kidding ;-)
> 
> Oh sure, joke about this one, don't mention xopen() ;-)

... we have xopen() and you should be using it? :-}

>>> +      // 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 ;-)

You are correct. I meant to say that you should use /* */-style commenting to
follow normal convention in glibc.

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

That makes sense.

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

OK.

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

OK.
 
>>> +		unlink (the_words[1]);
>>
>> Use xunlink.
> 
> This is the one where it's OK to ENOENT.

OK, assume you used maybe_xunlink then.
 
>>> +/* 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?

Oh! Yes, it does. I missed that.

-- 
Cheers,
Carlos.