Hi George,

On Mon, 23 Sep 2013 15:49:15 -0700, George Lorch wrote:
> On 9/23/2013 9:58 AM, Alexey Kopytov wrote:
>> Review: Needs Fixing
>>
>> George,
>>
>>     - I propose we rename ‘mysql_version’ to ‘server_version’, since we
>>       support not only MySQL.
> Makes sense, can do.
>>     - let’s also rename xtrabackup_history to xtrabackup_info. It
>>       contains meta information about the backup where it’s stored, not
>>       the history. I think we can even deprecate xtrabackup_checkpoints
>>       with xtrabackup_info in future.
> Do you want to rename the entire feature and all options (--no-info,
> --info=<name>, --incremental-info-name, --incremental-info-uuid) as well
> or just the table name?

No, sorry I should have been more specific. I was referring to the file 
created in the backup directory. 'xtrabackup_info' reflects its content 
better than 'xtrabackup_history' IMO.

>>     - the table does not define the character set as we discussed
>>       previously. This means the character set will depend on
>>       server/connection settings.
> Very true, forgot about that. Will add utf8 specifier.
>>     - update_history() is vulnerable to SQL injection attacks :) what if
>>       something like this is passed to --history, for example:
>>
>>       --history="foo'; DROP DATABASE customers".
>>
>>       Please use DBI prepared statements. This way you don’t have to roll
>>       your own quoting in other places.
> Yes, I actually had a discussion w/ Vadim on whether or not we are
> concerned w/ SQL injection at that level and the answer was not really
> so I just stuck with the original prototype code I had that used all
> literals. I suppose it wouldn't hurt and would be more proper to use DBD
> prepare and bind values instead of all the literal bits.

Yes, it's certainly not a critical issue in the XtraBackup context, but 
still opens the way for users shooting themselves in the foot with some 
bad argument to --history.

>>
>>     - it also appears that xtrabackup_history will not be created if
>>       --history is not passed to innobackupex. I think it should always
>>       be created. I.e. update_history() should collect all info
>>       unconditionally, but just avoid executing the actual INSERT if
>>       $option_history if false.
> Correct. There were some XB test case failures that would need to be
> fixed up and permissions manipulated if we always created the
> schema/filled the record. At that point I thought too that it may not be
> a great idea to change the default assumption that XB was read only on a
> server database by now having it start to write stuff back into it,
> possibly requiring a user to change their backup user privileges. So, I
> changed it to be entirely optional and any upgrades would be compatible
> with any existing backup user privileges without having to explicitly
> disable it or alter backup user privileges. I should have checked that
> out w/ you first. That being said, if you do still want it on/creating
> schema and filling the table and generating the file by default, I can
> change it back and go fix up whatever failing tests occur as a result
> (just adding --no-history/--no-info) and make sure that I give Hrvoje
> enough info doc wise to try to avoid confusion.

Sorry, I should have been more specific here to. I just meant to say 
that we have to create the xtrabackup_history (or xtrabackup_info) file 
regardless of whether --history is enabled.

The reason we are implementing that file is 
https://bugs.launchpad.net/percona-xtrabackup/2.2/+bug/1133017

[...]

>>      - /proc is only available on Linux, so the following will fail on
>>        everything else. And there are really no reasons to use it,
>>        there are many portable ways to get the cmd line from a Perl
>> script
>>
>> +  $tmp = sprintf("cat /proc/%d/cmdline", $$);
>> +  $tmp = qx($tmp);
>> +  system("cp /proc/$$/cmdline /tmp");
>> +  $tmp =~ s/\0/ /g; # transform embedded nulls between args to spaces
> OK. I originally searched perldocs and didn't see anything useful but
> then, I completely missed a simple and obvious solution...capture them
> to a temp string before they get parsed/shifted.

Yes.

>>      - system("rm -f $file") is equivalent to unlink($file)
> OK, perl noobness
>>
>>      - do you really want to scrub --encrypt and --encrypt-key-file?
> Hmm, yeah well, I guess that is a bit overkill. Removing.
>>
>>      - here’s a more elegant and correct scrub_option() implementation:
>>
>> sub scrub_option {
>>    my $scrub = shift;
>>    my $command = shift;
>>
>>    return $command =~ s/--$scrub=[^ ]+ /--$scrub=... /g
>>
>> }
> Nice.

Actually that won't work (I didn't verify it before posting). The 
following will work:

sub scrub_option {
   my $scrub = shift;
   my $command = shift;

   $command =~ s/--$scrub=[^ ]+/--$scrub=.../g;
   return $command;
}