Re: bzr commit into mysql-6.0-backup branch (jorgen.loland:2679) Bug#34171

[Alexander Nozdrin]

Hi,

thanks for the patch! I concur Chuck's comments. Also I have a few  
minor notes.

Just a question -- did you test the patch with symbolic links?

Please ping me when you send the second patch.

On Aug 21, 2008, at 12:38 , Jorgen Loland wrote:
> === added file 'mysql-test/t/backup_securefilepriv.test'
> --- a/mysql-test/t/backup_securefilepriv.test	1970-01-01 00:00:00  
> +0000
> +++ b/mysql-test/t/backup_securefilepriv.test	2008-08-21 08:38:12  
> +0000
> @@ -0,0 +1,118 @@
> +#
> +# Purpose: Backup images should only be allowed to be written to the
> +# path specified by secure_file_priv or a sub-path of it.
> +#
> +# See backup_securefilepriv-master.opt for secure_file_priv command  
> line option
> +#
> +# @@backupdir        is MYSQLTEST_VARDIR/master-data/
> +# @@secure_file_priv is MYSQLTEST_VARDIR/master-data/sfp_path/
> +
> +--echo Initializing tests
> +
> +--error 0,1,2

I'd suggest using text identifiers (from errmsg.txt), since they are  
more human-readable :)

> +CREATE DATABASE sfp_db;

It's a good rule to ensure that an object does not exist before  
creating it.
Usually it's done as follows:
--disable_warnings
DROP DATABASE IF EXISTS sfp_db;
--enable_warnings

Also, we try to stick to "well-known" object names in the test suite.
In particular, we try to use 'mysqltestN' format for database names.
Try: grep -i 'create database' mysql-test/t/*.test

That would simplify running the test suite on user database.

> +  if (!test_if_hard_path((*backupdir).c_ptr())) {

According to the coding style it should be like this:

if (xxxxx)
{
   ...
}

Also, I believe, "(*backupdir).c_ptr()" == "backupdir->c_ptr()", isn't  
it?

> +    fn_format(full_path, (*backupdir).c_ptr(),  
> mysql_real_data_home, "",
> +              MY_UNPACK_FILENAME|MY_RELATIVE_PATH);
> +
> +    (*backupdir)= (String)full_path;

I suggest using explicit copy operation here. We should try to avoid  
casts in the code.
>
>     fn_format(m_path.c_ptr(), orig_loc.str, backupdir->c_ptr(), "",
> -              MY_UNPACK_FILENAME);
> +              MY_UNPACK_FILENAME|MY_RELATIVE_PATH);

How about (add spaces):
MY_UNPACK_FILENAME | MY_RELATIVE_PATH
?