Code review comment for lp:~akopytov/percona-xtrabackup/use-dbd-mysql-in-innobackupex

I don't know if the script declares this in its main package, but if not:

use strict;
use warnings FATAL => 'all';
use English qw(-no_match_vars);

No production Perl program should run without those. Then:

s/$@/$EVAL_ERROR/g
s/$!/$CHILD_ERROR/g

etc. See http://perldoc.perl.org/perlvar.html

---

This

20 +# check existence of DBD::mysql module
21 +my $dbd_mysql_installed;
22 +
23 +BEGIN {
24 + eval "use DBD::mysql";
25 + $dbd_mysql_installed=$@ ? 0 : 1;
26 +}
27 +

is better written like:

eval {
   require DBD::mysql;
};
my $have_dbd_mysql = $EVAL_ERROR ? 0 : 1;

---

Typo:

322 + Die "Keep-alive process has already been started for this connection."
395 + Die "Keep-alive process has never been started for this connection."
etc.

s/Die/die/

---

With hashes like

45 +my %mysql;
97 + %mysql = mysql_connect({
152 + wait_for_safe_slave(\%mysql);

it's best to return and pass hashrefs (and arrayrefs). So instead of

return %hash

a sub should

my %hash = ( ... );
return \%hash;

or

my $hash = { ... };
return $hash;

I almost never use "regular" arrays (@foo) or hashes (%foo) unless there's a good reason.

And sometimes using a hash like this is a sign that the hash should be turned into a class/object.

---

For sub args like

97 + %mysql = mysql_connect({
98 + abort_on_error => 0,
99 + keepalives => 0
100 + });

the general standard is

hello(arg1 => "foo", arg2 => $bar); # caller

sub hello {
   my (%args) = @_;
   my $args1 = $args{args1};
   my $args2 = $args{args2};
   ...

Then you can do things like check for required args, have optional args (e.g. $optional_arg = $args{whatever} || 42;).

---

For eval like

347 + eval {
348 + $con->{dbh}->selectrow_array("select '$hello_message'");
349 + };
350 +
351 + $rc = 255 if $@;

it's good to adopt this style:

eval {
   ...
};
if ( $EVAL_ERROR ) {
   ...
}

So always put an explicit check of $EVAL_ERROR after and eval {} block. This is core Perl's equivalent to try {} catch {}. (There's a real try-catch syntax with a non-core module, but it has serious limitations imho.)

---

Using 'and' as in

846 -if($var_version =~ m/5\.1\.\d/ and $var_innodb_version =~ m/.*/){

isn't preferred because it has different precedence than '&&', so '&&' is used in most cases. 'or' is the common exception in Perl, used very often like:

open my $fh, '>', $file or die "Error opening $file: $OS_ERROR";

===

Setting review to "Needs Fixing" because of "Die" (see above); that will cause the tool to die with a syntax error rather than the intended error message.