STATUS
------
One change requested.
REQUESTS
--------
1. Remove parameter 'path' of Logger::init() which is no longer used.
COMMENTARY
----------
I will accept this patch but I think that the handling of backup paths should be
implemented differently. I express it in form of a suggestion for your
consideration.
SUGGESTIONS
-----------
1. Do not dynamically allocate memory in Backup_log::backup_file(...) since you
know in advance the size of the buffer.
2. Do not do backup file path computations, access checking and related tasks in
the backup::Stream class but outside, in the Backup_restore_ctx class which is
the logical place for doing all necessary preparations for BACKUP/RESTORE
operation.
When a command is issued, parser identifies the location which was given by user
in the TO/FROM clause. This is passed to the instance of context class. Before
the real work begins, the following should be done:
- Take the location given by user and the current backupdir setting and
calculate the absolute path of backup file (or report error if something
wrong).
- Report the resulting location to backup logger.
- Check if user can access the resulting path (e.g --secure-file-priv
option)
- Open input/output stream for the computed location.
(continue other preparations...)
The logical place for all these preparations is inside
Backup_restore_ctx::prepare() method. Doing some of these tasks inside
backup::Stream class makes it harder to follow the logic and at the same time
makes the Stream class less self contained - it needs to be aware of things like
"--backupdir" or "--secure-file-priv".
It would be much better to keep the simple semantics of backup::Stream - it
represents an input or output stream to a given file. It is not the Stream class
but its user who should decide which file to access. That is, the client of this
class, calculates the path of the file he wants to access using its own logic
(in our case taking into account --backupdir setting etc) and then creates a
Stream instance to access it.
Having the simple and straightforward Stream class, the logic of preparations
for BACKUP/RESTORE operation is not spread over the code but rather concentrated
in a single place - the Backup_restore_ctx class and its prepare*() methods.
This makes it easier to understand and maintain this logic.
DETAILS
-------
Chuck Bell wrote:
(cut)
> @@ -68,6 +64,28 @@ void Backup_log::add_driver(const char *
> if (strlen(driver_name) > 0)
> str.append(driver_name);
> m_op_hist.driver_name.copy(str);
> +}
> +
> +/**
> + Report name of a backup image file and path used in backup/restore.
> +
> + This method updates the backup_file and backup_file_path information
> + in the history data.
> +
> + @param[IN] char * full_path The full path and file name of the backup file.
> +*/
> +void Backup_log::backup_file(const char *full_path)
> +{
> + String str;
> +
> + if (strlen(full_path))
> + {
> + size_t i= 0;
> + size_t j= 0;
> + m_op_hist.backup_file_path= (char *)my_malloc(FN_REFLEN, MYF(MY_WME));
Since backup_file_path always has length FN_REFLEN, why to allocate it
dynamically? An char array of that length could be used instead, without a risk
that allocated memory is not freed.
> + j= dirname_part(m_op_hist.backup_file_path, full_path, &i);
> + m_op_hist.backup_file = (char *)(full_path + i);
> + }
> }
