Rafal Somla wrote:
> 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.
>
The test code I added does not generate any output in the result file, I
did this on purpose, the 'diff_files' line checks weather this part of
the test passed or not.
> 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.
>
I am thinking about another way now, moving the load_defaults out of
parse_args() to main(), so that we do not need to make defaults_argv
global.
> 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
