RE: bzr commit into mysql-6.0-backup branch (cbell:2674) Bug#35230

[Chuck Bell]

Hi,

Thanks for a great review. I have a short reply below. 

> kernel.cc @@141
> ---------------
> Should the comment read "needed since we cannot trust that 
> the path has
> *not* become invalid"? Or remove the double negation :)

Statement corrected.

> stream.cc
> ---------
> BACKUP ... TO '../backup1.bak'; seems to ignore the "../". I 
> *think* this is because in Stream::make_relative_path, j is set to 
> backupdir->length(), and it therefore points outside the 
> string, not to
> '/'. "if (base[j] == FN_LIBCHAR)" is therefore not true the 
> first iteration. Because of this, the first ../ in orig_loc 
> does not have any effect.

Bug fixed. Bug showed up IFF you have a trailing / for backupdir.

> mysqld.cc
> ---------
> Are the warnings supposed to be localized?

I cannot set it to localized at this point in the code because the ER()
facility is not initialized.

> set_var.cc
> ----------
> Documentation for sys_check_backupdir - I don't think this 
> function can return anything but 0.

Documentation corrected.

> backup_backupdir.test:
> ----------------------
> It would be somewhat safer to use another backup image name 
> on the ../ tests, something like '../bup_backupdir2.bak'. 
> This would more likely result in an error if "../" handling 
> was not correct.

Good idea. I changed the test.

> Surprisingly, the test with path ../../../../../../ completes 
> successfully on my Linux system. The number of ../ is much 
> higher than the path depth. When I try to do the same 
> manually, the command fails as expected.

Weird. I checked it on Windows and Ubuntu.

On Windows, the test runs fine. On Ubuntu, the test runs fine if run by
itself but if run as --do-test=backup it fails. I discovered when the
relative path doesn't work and I return an empty string that it isn't really
empty. So I initialize it first now. That solves the problem.

New patch coming soon.

Chuck