Hi Jason,
Here is my review in the format we use in backup team. We consider requests
compulsory - you have to address them somehow (by e.g., discussing if they are
reasonable). The suggestions are optional for you - it is fine to ignore them
completely.
STATUS
------
Requested missing update for test result file.
REQUESTS
--------
1. You extend mysqlbinlog.test but I don't see the corresponding changes in the
result file (forgot to --record new results?). This should be fixed.
SUGGESTIONS
-----------
2. Declare **defaults_argv as static variable to discourage its use from other
modules.
3. I don't like that main() is using defaults_argv variable which is not
assigned within it. This obscures the logic and makes code harder to
understand/maintain. I suggest to go around this by declaring a new function
with the following specification:
/**
@function free_args()
@short Frees memory allocated by parse_args(). Should be always called after
a call to parse_args().
*/
Then this function would be called from within main(). Of course you still need
a global variable to communicate between free_args() and parse_args(), but the
use of that variable would be limited to these two functions and this will be
easier to understand.
Rafal
He Zhenxing wrote:
> #At file:///media/sdb2/hezx/work/mysql/bzrwork/b38468/6.0-rpl/ based on
> revid:alfranio.correia@stripped
>
> 2841 He Zhenxing 2009-04-14
> BUG#38468 Memory leak detected when using mysqlbinlog utility
>
> There were two memory leaks in mysqlbinlog command, one was already
> fixed by previous patches, another one was that defaults_argv was
> set to the value of argv after parse_args, in which called
> handle_options after calling load_defaults and changed the value
> of argv, and caused the memory allocated for defaults arguments
> not freed.
>
> Fixed the problem by setting defaults_argv right after calling
> load_defaults.
> @ client/mysqlbinlog.cc
> change defaults_argv to global, and set its value right after load_defaults
>
> M client/mysqlbinlog.cc
> M mysql-test/t/mysqlbinlog.test
