STATUS
------
Changes requested
REQUESTS
--------
1. Please explain the change in stream.cc whereby mysql_real_data_home is
used in the fn_format() call. See [1]. I think this is confusing and it's
why I didn't use that method for the backupdir work originally.
2. Please don't use {} when only 1 statement is used. See [2] below and
http://forge.mysql.com/wiki/MySQL_Internals_Coding_Guidelines#Indentation_an
d_Spacing
3. Please make distinct error messages for 'can't write to...' and the
--secure-file-priv failure. See [3].
COMMENTARY
----------
Good work!
SUGGESTIONS
-----------
Can we do the hard path test like this (in backup_backupdir test)?
--echo Try a backup to an invalid hard path.
--error ER_OPTION_PREVENTS_STATEMENT
BACKUP DATABASE bup_backupdir TO
'/no/no/no/no/no/master_data/not/there/either/bup_backupdir6.bak';
There in only one other very minor suggestion below.
DETAILS
-------
> === modified file 'mysql-test/r/backup_errors.result'
...
> -ERROR HY000: Can't write to backup location 'bdb/t1.frm'
> (file already exists?)
> +ERROR HY000: Can't write to backup location 'bdb/t1.frm'
> (file already
> +exists or running with --secure-file-priv option?)
[3] Can we not make this message into two separate error messages like
'can't write to...' ' server is running secure-file-priv' (last one already
exists)? It would make for a more accurate report of what is wrong.
> BACKUP DATABASE adb TO "test.bak";
> backup_id
> #
> BACKUP DATABASE adb TO "test.bak";
> -ERROR HY000: Can't write to backup location 'test.bak' (file
> already exists?)
> +ERROR HY000: Can't write to backup location 'test.bak' (file already
> +exists or running with --secure-file-priv option?)
[3]] Same as previous comment.
...
> +INSERT INTO sfp_db.t1 VALUES (1);
> +INSERT INTO sfp_db.t1 VALUES (2);
> +INSERT INTO sfp_db.t1 VALUES (3);
> +INSERT INTO sfp_db.t1 VALUES (4);
> +INSERT INTO sfp_db.t1 VALUES (5);
> +INSERT INTO sfp_db.t1 VALUES (10);
> +INSERT INTO sfp_db.t1 VALUES (15);
> +INSERT INTO sfp_db.t1 VALUES (100);
Did you know this could be written as INSERT...VALUES (1), (2), (3)...?
> === modified file 'sql/backup/stream.cc'
...
> + if (!test_if_hard_path((*backupdir).c_ptr())) {
> + char full_path[FN_LEN];
> + /*
> + MY_UNPACK_FILENAME -> "~/" will be changed to full path
> + MY_RELATIVE_PATH -> path is constructed from the path in both
> + arguments, including that every "../" in backupdir removes
> + deepest dir from mysql_real_data_home
> + */
[1] I don't understand this. Why is mysql_real_data_home involved at all in
this call? It should have nothing to do with backupdir.
> + fn_format(full_path, (*backupdir).c_ptr(),
> mysql_real_data_home, "",
> + MY_UNPACK_FILENAME|MY_RELATIVE_PATH);
> +
> + (*backupdir)= (String)full_path;
> + }
> +
...
> + if (!has_access) {
> + /* Write only allowed to dir or subdir specified by
> secure_file_priv */
> + my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0),
> +"--secure-file-priv");
> + }
[2] Please do not use {} when only 1 statement (and not a macro/case) is
target of condition. See coding guidelines.
> + if (!test_secure_file_priv_access(m_path.c_ptr())) {
> + return FALSE;
> + }
[2] Please do not use {} when only 1 statement (and not a macro/case) is
target of condition. See coding guidelines.
> === modified file 'sql/share/errmsg.txt'
> --- a/sql/share/errmsg.txt 2008-08-20 13:23:10 +0000
> +++ b/sql/share/errmsg.txt 2008-08-21 08:38:12 +0000
> @@ -6177,7 +6177,7 @@ ER_BACKUP_INVALID_LOC ER_BACKUP_READ_LOC
> eng "Can't read backup location '%-.64s'"
> ER_BACKUP_WRITE_LOC
> - eng "Can't write to backup location '%-.64s' (file
> already exists?)"
> + eng "Can't write to backup location '%-.64s' (file
> already exists or running with --secure-file-priv option?)"
> ER_BACKUP_LIST_DBS
> eng "Can't enumerate server databases"
> ER_BACKUP_LIST_TABLES
[3] I think we need separate error messages here.
