Code review comment for lp:~longbow/percona-xtrabackup/fix_687544

On 24.11.11 12:51, Valentine Gostev wrote:
> - there's no need to get tmpdir explicitly...
>
> No. We need this, since by default new temp file is created in $cwd
> Please check this example:
>
> my $temp_dir=File::Spec->tmpdir();
> print "Temp dir is $temp_dir \n";
> my $tempfile=File::Temp->new( TEMPLATE => 'tempXXXXX',
> SUFFIX => '.pxb',
> UNLINK => 0);
> my $tempfile2=File::Temp->new( TEMPLATE => 'tempXXXXX',
> DIR => $temp_dir,
> SUFFIX => '.pxb',
> UNLINK => 0);
> print "File name is $tempfile \n";
> print "File name is $tempfile2 \n";
>
> [longbow@mil-x percona]$ ./test.pl
> Temp dir is /tmp
> File name is tempHklkd.pxb
> File name is /tmp/tempReYtO.pxb
> [longbow@mil-x percona]$ ls tempHklkd.pxb
> tempHklkd.pxb
> [longbow@mil-x percona]$ ls /tmp/tempR*
> /tmp/tempReYtO.pxb
> [longbow@mil-x percona]$
>
> tmpdir() allows to find first writeable temp directory, and it is cross-platform.
>

Right, and it is used by File::Temp. The reason it is created in the
current directory in the first case is that you specify template without
specifying a directory explicitly. Try removing TEMPLATE from your examples.

> - there's no need to specify the template explicitly...
> - we don't care about the suffix either
>
> 1. looks nice

Come on, how much nicer is tempHtwm_.pxb as compared to NARg17D6OP.pxb
or just yjOxKMXEGD?

> 2. suffix allows to trace leftovers (just in case)
>

There can be no leftovers, those files are removed automatically unless
UNLINK => 0 is used. Yes, even if innobackupex fails with an error or
crashes. Yes, on Windows too.

> - File::Temp->new() returns a file handle...
> That is why I use it, since it is safe to work with fh.
> Keeping filename separately is a possible source of race conditions,
> for example when object is being destroyed, but variable keeps old
> filename.
> Since object acts both as filename and fh - it does not make sense to keep filename separately.
>

So you've RTFMed which is good, but you didn't understand it which is bad.

File::Temp returns both a filename and an open file handle to guarantee
the temporary file is not replaced between getting the filename,
checking that it doesn't exist and opening it.

Our case is different. If you take a look at how $mysql_stdout and
$mysql_stderr are actually used, you'll see that we don't use (and thus,
don't need) the filehandle at all. We just the filename in shell
commands. So those security considerations from the manual are simply
not applicable: all we need is a temporary file name, and there's always
a chance it will be overwritten/replaced between different mysql client
invocations.

Which actually means all we need is tmpnam(), File::Temp->new() is an
overkill for our purposes.

> - note that $mysql_stderr and $mysql_stdout...
> Yes, they are reassigned but they are responsible for another leftovers (/tmp/mysql_std[err,out] after stream backup)
> I think it is ok to use the same fh.
>

It will _not_ use the same fh, as no fh is actually being used. It will
assign "$backup_dir/mysql-stderr" or "$option_tmpdir/mysql-stderr" and
use those _file_names_.

> - how about a test case?
> testing that stderr and stdout are not there would not reveal above issue, since those are different leftovers kept in different location. But test will require to add debug option to verify that files are created correctly and removed after backup finished.
> So I guess right now it is not worth to do it

So it turns out it would, as your changes would simply not work for
local backups (see above).

And what's so complex in verifying that specific files are not left
behind after an innobackupex run in the current directory? All you have
to do is to read the bug description and make sure your test case fails
before your fix, but succeeds after. What debug option are you referring to?