STATUS
------
Not approved - changes in test script requested.
REQUESTS
--------
1. Use symbolic error names in --error directive (see [1]).
SUGGESTIONS
-----------
1. Do not introduce virtual get_file() method to Stream class (see [2]).
DETAILS
-------
Oystein.Grovlen@stripped wrote:
> #At file:///home/oysteing/mysql/mysql-6.0-backup-1/
>
> 2737 oystein.grovlen@stripped 2008-12-03
> BUG#34754 Backup: Invalid error message for incorrect path name while
> performing backup
>
> Will now use error messages generated by my_create (BACKUP) and my_open
> (RESTORE)
> when failing to create/open a file since these message are more specific as to
> the cause of the failure.
>
> Note that the backup-specific errors will still be pushed on the error stack,
> and it is still these errors that are recorded in the backup history log and
> in the error file.
> modified:
> mysql-test/suite/backup/r/backup_backupdir.result
> mysql-test/suite/backup/r/backup_errors.result
> mysql-test/suite/backup/t/backup_backupdir.test
> mysql-test/suite/backup/t/backup_errors.test
> sql/backup/stream.cc
> sql/backup/stream.h
> sql/share/errmsg.txt
>
(...)
> === modified file 'mysql-test/suite/backup/t/backup_errors.test'
> --- a/mysql-test/suite/backup/t/backup_errors.test 2008-11-25 17:44:19 +0000
> +++ b/mysql-test/suite/backup/t/backup_errors.test 2008-12-03 14:51:44 +0000
> @@ -15,7 +15,9 @@ DROP DATABASE IF EXISTS bdb;
> --enable_warnings
>
> # non-existent backup archive
> ---error ER_BACKUP_READ_LOC
> +--replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
> +--replace_regex /Errcode: [0-9]+/Errcode: #/
> +--error 29
[1] Why symbolic error name is replaced by numeric value? There was a bug and a
patch doing exactly reverse of this.
> RESTORE FROM 'test.bak';
>
> CREATE DATABASE adb;
> @@ -29,9 +31,13 @@ BACKUP DATABASE adb TO '';
> SHOW WARNINGS;
>
> # don't overwrite existing files
> ---error ER_BACKUP_WRITE_LOC
> +--replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
> +--replace_regex /Errcode: [0-9]+/Errcode: #/
> +--error 1
> BACKUP DATABASE adb TO "bdb/t1.frm";
> --replace_column 2 #
> +--replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
> +--replace_regex /Errcode: [0-9]+/Errcode: #/
> SHOW WARNINGS;
>
> --replace_column 1 #
> @@ -40,9 +46,13 @@ BACKUP DATABASE adb TO "test.bak";
> SHOW WARNINGS;
>
> # don't overwrite existing backup image
> ---error ER_BACKUP_WRITE_LOC
> +--replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
> +--replace_regex /Errcode: [0-9]+/Errcode: #/
> +--error 1
> BACKUP DATABASE adb TO "test.bak";
> --replace_column 2 #
> +--replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
> +--replace_regex /Errcode: [0-9]+/Errcode: #/
> SHOW WARNINGS;
>
> --remove_file $MYSQLTEST_VARDIR/master-data/test.bak
>
> === modified file 'sql/backup/stream.cc'
> --- a/sql/backup/stream.cc 2008-10-15 20:00:48 +0000
> +++ b/sql/backup/stream.cc 2008-12-03 14:51:44 +0000
(...)
> @@ -472,6 +472,15 @@ bool Output_stream::rewind()
> return init();
> }
>
> +/**
> + Create file to be written to
> +
> + @return File descriptor
> +*/
> +File Output_stream::get_file()
> +{
> + return my_create(m_path->c_ptr(), 0, m_flags, MYF(MY_WME)); // reports errors
Note: when I once proposed to mark functions which report errors to distinguish
them from these which do not, I meant reporting to backup logs (using
backup::Logger). If my_create() reports errors then it is inside the core server
module, which is outside of the scope of the backup code. From the perspective
of backup code, this function *does not* report errors (meaning: it does not log
anything with backup::Logger).
(...)
> === modified file 'sql/backup/stream.h'
> --- a/sql/backup/stream.h 2008-11-13 13:02:36 +0000
> +++ b/sql/backup/stream.h 2008-12-03 14:51:44 +0000
> @@ -100,6 +100,9 @@ class Stream: public fd_stream
> size_t m_block_size;
> Logger& m_log;
>
> + virtual File get_file() = 0; // Create or open file
> +
> +
[2] I think it will go better with the original intention of the backup::Stream
class if instead of delegating the task of opening a file descriptor to derived
classes, the open() method will be extended with a parameter telling if the
underlying file should be created or not. Like this:
/**
Open a stream.
@param[IN] create Should we create underlying file if does not exist?
The stream is opened giving access to the underlying file (as specified
when Stream instance was created). If file does not exists and create is
TRUE, the file is created first. Otherwise (file does not exist but create is
FALSE) method fails and reports error.
@retval 0 if stream was successfully opened
@retval ER_OPTION_PREVENTS_STATEMENT if secure-file-priv option
prevented stream open from this path
@retval -1 if open failed for another reason
*/
int Stream::open(bool create);
Rafal
