On Wed, Apr 15, 2015 at 02:59:55PM +0900, Michael Paquier wrote:
> On Wed, Apr 15, 2015 at 2:38 PM, Noah Misch wrote:
> > Solaris 10 ships Perl 5.8.4, and RHEL 5.11 ships Perl 5.8.8. Therefore, Perl
> > installations lacking this File::Path feature will receive vendor support for
> > years to come. Replacing the use of keep_root with rmtree+mkdir will add 2-10
> > lines of code, right? Let's do that and not raise the system requirements.
>
> Good point. Let's use mkdir in combination then. Attached is an updated patch.
> --- a/src/bin/initdb/t/001_initdb.pl
> +++ b/src/bin/initdb/t/001_initdb.pl
> @@ -1,6 +1,7 @@
> use strict;
> use warnings;
> use TestLib;
> +use File::Path qw(rmtree);
> use Test::More tests => 19;
>
> my $tempdir = TestLib::tempdir;
> @@ -18,27 +19,30 @@ command_fails([ 'initdb', '-S', "$tempdir/data3" ],
> mkdir "$tempdir/data4" or BAIL_OUT($!);
> command_ok([ 'initdb', "$tempdir/data4" ], 'existing empty data directory');
>
> -system_or_bail "rm -rf '$tempdir'/*";
> -
> +rmtree($tempdir);
> +mkdir $tempdir;
It's usually wrong to remove and recreate the very directory made by
File::Temp. Doing so permits a race condition: an attacker can replace the
directory between the rmdir() and the mkdir(). However, TestLib::tempdir
returns a subdirectory of the build directory, and the build directory is
presumed secure. That's good enough.
> -system_or_bail "rm -rf '$tempdir'/*";
> +rmtree($tempdir);
> command_ok([ 'initdb', '-T', 'german', "$tempdir/data" ],
> 'select default dictionary');
You omitted the mkdir() on that last one. It works, since initdb does the
equivalent of "mkdir -p", but it looks like an oversight.
As I pondered this, I felt it would do better to solve a different problem.
The "rm -rf" invocations presumably crept in to reduce peak disk usage.
Considering the relatively-high duration of a successful initdb run, I doubt
we get good value from so many positive tests. I squashed those positive
tests into one, as attached. (I changed the order of negative tests but kept
them all.) This removed the need for intra-script cleaning, and it
accelerated script runtime from 22s to 9s. Does this seem good to you?
Thanks,
nm