Code review comment for lp://qastaging/~gl-az/percona-xtrabackup/2.1-valgrind

Hm, the comments mention the new --use-valgrind option in innobackupex, but I don't see it in the code?

Which is good, because I would prefer this to be implemented in a different way. Instead of adding a new option to innobackupex we can modify the test suite to pass:

--ibbackup="valgrind xtrabackup*"

to innobackupex. And likewise, call "valgrind xtrabackup*", if the xtrabackup binary is called directly by tests.

This is a bit tricky to implement due to Bash word splitting. But the following proof-of-concept patch works for me:

=== modified file 'test/inc/common.sh'
--- test/inc/common.sh 2013-07-25 15:04:49 +0000
+++ test/inc/common.sh 2013-07-27 10:17:31 +0000
@@ -2,7 +2,7 @@ set -eu

 function innobackupex()
 {
- run_cmd $IB_BIN $IB_ARGS $*
+ run_cmd $IB_BIN "${IB_ARGS[@]}" $*
 }

 function xtrabackup()
@@ -239,7 +239,7 @@ function switch_server()
  MYSQLD_ARGS="$MYSQLD_ARGS --user=root"
     fi

- IB_ARGS="--defaults-file=$MYSQLD_VARDIR/my.cnf --ibbackup=$XB_BIN"
+ IB_ARGS=("--defaults-file=$MYSQLD_VARDIR/my.cnf" "--ibbackup=$XB_BIN")
     XB_ARGS="--defaults-file=$MYSQLD_VARDIR/my.cnf"

     # Some aliases for compatibility, as tests use the following names

=== modified file 'test/run.sh'
--- test/run.sh 2013-07-25 15:04:49 +0000
+++ test/run.sh 2013-07-27 10:17:40 +0000
@@ -435,7 +435,7 @@ function get_version_info()
  vlog "Cannot find '$XB_BIN' in PATH"
  return 1
     fi
- XB_BIN="$XB_PATH"
+ XB_BIN="valgrind $XB_PATH"

     # Set the correct binary for innobackupex
     IB_BIN="`which innobackupex`"

The downside is that IB_ARGS is now an array (this is required to prevent "--ibbackup=valgrind xtrabackup" as 2 different words), so all tests referencing IB_ARGS also have to be modified. But I like it more than another option to innobackupex.