STATUS
------
Changes requested.
REQUESTS
--------
1. In Logger::write_message() (logger.cc), please fix the error which was
present there. In case of reporting errors using push_warning_printf() the
constant should be MYSQL_ERROR::WARN_LEVEL_ERROR, not MYSQL_ERROR::WARN_LEVEL_WARN.
2. In the same function, use m_push_errors flags also for controlling whether
*warnings* are pushed on the stack.
3. Inside Backup_pump constructor, interrupt construction if errors are detected
(e.g., if bitmap_init() fails).
4. Implementation of Backup_info::Global_iterator class. Either keep the current
design of "construction is initialization", in which case there is no init()
method, or switch to using *::init() method consistently for all iterators. The
second alternative would mean adding init() to the public interface of
Image_info::Iterator and implementing it in derived classes.
5. Use macros TRUE and FALSE instead of C++ constants true and false. This is
for portability.
6. Remove incorrect "never errors" comments:
- at info.get_db_object() call inside find_obj() (image_info.cc) - this method
can error and signal this by returning NULL.
- when Backup_info instance is created inside
Backup_restore_ctx::prepare_for_backup(). The constructor detects and logs
errors.
SUGGESTIONS
-----------
1. Implement Logger::push_errors() as inlined method in logger.h.
2. In this fragment:
> @@ -1646,11 +1658,9 @@ void* bcat_db_iterator_get(st_bstream_im
> }
>
> backup::Image_info::Iterator *it= info->get_db_objects(*db);
> -
> if (!it)
> {
> - info->m_ctx.fatal_error(ER_BACKUP_LIST_DB_TABLES);
> - return NULL;
> + info->m_ctx.fatal_error(ER_OUT_OF_RESOURCES);
> }
retain early return as in the original code. This is safer since the function
will correctly return in case of error, even if someone adds some code after if(
...) statement.
COMMENTARY
----------
It seems that we have a disagreement about how code should be structured. I'm
not going to push my point of view but, for the record, I'll state it here in
form of more suggestions of how to do things differently. There are also few
points here we have not discussed yet.
1. When reporting errors from backup kernel, give only error information which
is known from interface *specifications* not from implementation. This means
less information about many errors (as interface is very simplistic). If more
info is a must, we should extend the interfaces.
2. Call Backup_restore_ctx::fatal_error() only from top-level functions.
Rationale is that low-level function should not decide whether given error is
fatal (in the sense that we abort current operation) or not. The calee should
only report an error and then the caller should decide whether it is ok to
proceed or not. To enforce this, I would turn fatal_error() into a private
method of Backup_restore_ctx and give explicit permissions to use it only to
selected functions using friend declarations.
3. Use a different name for Backup_restore_ctx::log_error(). Current name makes
impression that it is an alternative for Logger::report_error(). But I'd like to
have Logger::report_error() as a central method for logging errors. My
suggestion would be to rename it to something like
Backup_restore_ctx::fatal_error_no_log() and apply for this method the same
restrictions as for Backup_restore_ctx::fatal_error().
4. I think that marking function as one which never errors (and e.g. changing
its return type to void) should be a design decision, not just a reflection of
how it is currently implemented.
Rafal
