Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2736) Bug#40303 Bug#40304

[Øystein Grøvlen]

STATUS
======

One issue with latest patch that needs to be fixed.

REQUESTS
========

You have (by accident or by merge?) removed to much from the Logger
destructor:

 >  inline
 >  Logger::~Logger()
 > -{
 > -  clear_saved_errors();
 > -  delete backup_log;
 > -}
 > +{}

I just added 'delete backup_log' in order to prevent a memory leak
reported by valgrind. I do not think this should be removed.

SUGGESTIONS
===========

 > +   /*
 > +     When logging to server's error log, the msg will be prefixed with
 > +     "Backup:"/"Restore:" if the operation has been initialized. In 
that case
 > +     out points at the output string with the prefix added, 
otherwise out
 > +     points at msg (without any prefixes).
 > +    */

This was not quite what I was asking for.  I was thinking of an
explanation to why out is used for some of the reporting (e.g.,
sql_report_error) while msg is used in other places (e.g.,
my_printf_error).

By the way, it took me some time to understand what you meant by "if
the operation has been initialized".
 >
 > +  /*
 > +    Note: constructor is private because instances of this class are 
supposed
 > +    to be created only with 
Backup_restore_ctx::prepare_for_restore() method.
 > +  */

Maybe you should add that this works because Backup_restore_ctx is
declared as friend.

--
Øystein