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

[Rafal Somla]

Hi Oystein,

Great that you spotted the merge mistake. I committed a fixed patch: 
http://lists.mysql.com/commits/59364.

Øystein Grøvlen wrote:
> 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;
>  > -}
>  > +{}
> 

Yes, that was merge mistake - good that you caught it!

> 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".

I changed comment to a hopefully more clear:

    /*
      When logging to server's error log, msg will be prefixed with
      "Backup:"/"Restore:" if the operation has been initialized (i.e., after
      Logger::init() call). For other destinations, msg is reported as it is.

      Pointer out points at output string for server's error log, which has the
      prefix added if needed.
     */

>  >
>  > +  /*
>  > +    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.
> 

I added comments to friend declarations.

Rafal