bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] tests/init.sh: new file to be used via most *.sh tests


From: Jim Meyering
Subject: Re: [PATCH] tests/init.sh: new file to be used via most *.sh tests
Date: Fri, 27 Nov 2009 22:03:10 +0100

Ralf Wildenhues wrote:
> * Jim Meyering wrote on Wed, Nov 25, 2009 at 02:49:59PM CET:
>> I've pushed tests/init.sh, as yet unused.
>
> I haven't looked at this in detail, due to time constraints,

Hi Ralf,

Thanks for the feedback.
FYI, the mkdtemp-related code has been in gnulib for some time,
Even if I've been the only user of it via parted's tests, I know
it has gotten some decent exposure.

> so just a couple of notes: Automake has a similar file tests/defs.in
> that is not as elaborate; still, you might be able to profit from it.
> For example, turning on VERBOSE if srcdir is not set but derived from
> $0 is very handy: it typically causes manual
> $ ../../source/tests/foo.test

In tests/defs.in the only use of VERBOSE is to unset it.
I'll look further tomorrow.

> invocations to produce verbose output.
>
> Then, there are a couple of patches pending for that file, that we still
> need to rework to be portable enough, but a proposed run_command
> function to catch output looks quite helpful.
>
>> +# Run the user-overridable cleanup_ function, remove the temporary
>> +# directory and exit with the incoming value of $?.
>> +remove_tmp_()
>> +{
>> +  __st=$?
>
> Have you checked whether using this function in a trap 0 correctly
> catches $? with FreeBSD sh?  I'm thinking of (autoconf.info):
>
>      The shell in FreeBSD 4.0 has the following bug: `$?' is reset to 0
>      by empty lines if the code is inside `trap'.

Thanks, but that code has been in coreutils' test-lib.sh
for a long time without reports of trouble, so I'm not worried.
And besides, since FreeBSD 8.0 was just released, I won't lose
sleep over portability problems in 4.0.

>> +  cleanup_
>> +  # cd out of the directory we're about to remove
>> +  cd "$initial_cwd_" || cd / || cd /tmp
>
> That 'cd /' looks dangerous to me.  Why not simply abort if you can't go
> where you expect to?  I always get nervous with something like this
> before a 'rm -rf'.

Yes, this might be worth adjusting.
The combination of a test failing to return to $initial_cwd_
and a mistake that set $test_dir_ ...  not likely at all,
but not impossible, either.

But then you can argue that a misbehaving test
could simply set $initial_cwd_=/.

Not sure, after all...

>> +  chmod -R u+rwx "$test_dir_"
>> +  # If removal fails and exit status was to be 0, then change it to 1.
>> +  rm -rf "$test_dir_" || { test $__st = 0 && __st=1; }
>> +  exit $__st
>> +}
>
>> +# Create a temporary directory, much like mktemp -d does.
>> +# Written by Jim Meyering.
>
> Why not reuse the code snippet from `info Autoconf --index mktemp'?

Partly because that relies on $RANDOM, which is not portable.
Also because I wanted an XXXX... template of user-selectable length.
I required that this function work safely also when creating a directory
in /tmp.  With just a PID, it's too subject to DoS abuse.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]