Luís Soares wrote:
> Hi Zhenxing,
>
> Good work. I only have two minor comment/questions inline regarding
> the test case (I think these are not a show blocker, but I will wait for
> your reply before approving).
>
> Regards,
> Luís
>
> On Tue, 2009-04-14 at 09:57 +0000, 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
> > === modified file 'client/mysqlbinlog.cc'
> > --- a/client/mysqlbinlog.cc 2008-12-24 10:48:24 +0000
> > +++ b/client/mysqlbinlog.cc 2009-04-14 09:57:06 +0000
> > @@ -43,6 +43,7 @@
> >
> > #define CLIENT_CAPABILITIES (CLIENT_LONG_PASSWORD | CLIENT_LONG_FLAG |
> CLIENT_LOCAL_FILES)
> >
> > +char **defaults_argv;
> > char server_version[SERVER_VERSION_LENGTH];
> > ulong server_id = 0;
> >
> > @@ -1303,6 +1304,7 @@ static int parse_args(int *argc, char***
> >
> > result_file = stdout;
> > load_defaults("my",load_default_groups,argc,argv);
> > + defaults_argv=*argv;
> > if ((ho_error=handle_options(argc, argv, my_long_options, get_one_option)))
> > exit(ho_error);
> > if (debug_info_flag)
> > @@ -1947,7 +1949,6 @@ end:
> >
> > int main(int argc, char** argv)
> > {
> > - char **defaults_argv;
> > Exit_status retval= OK_CONTINUE;
> > ulonglong save_stop_position;
> > MY_INIT(argv[0]);
> > @@ -1957,7 +1958,6 @@ int main(int argc, char** argv)
> > my_init_time(); // for time functions
> >
> > parse_args(&argc, (char***)&argv);
> > - defaults_argv=argv;
> >
> > if (!argc)
> > {
> >
> > === modified file 'mysql-test/t/mysqlbinlog.test'
> > --- a/mysql-test/t/mysqlbinlog.test 2009-03-06 20:33:52 +0000
> > +++ b/mysql-test/t/mysqlbinlog.test 2009-04-14 09:57:06 +0000
> > @@ -368,3 +368,27 @@ eval SET @@global.server_id= $save_serve
> > --remove_file $binlog_file
> >
> > --echo End of 5.1 tests
> > +
> > +#
> > +# BUG#38468 Memory leak detected when using mysqlbinlog utility;
> > +#
> > +disable_query_log;
> > +RESET MASTER;
> > +CREATE TABLE t1 SELECT 1;
> > +FLUSH LOGS;
> > +DROP TABLE t1;
> > +enable_query_log;
> > +
> > +# Write an empty file for comparison
> > +write_file $MYSQLTEST_VARDIR/tmp/mysqlbinlog.warn.empty;
> > +EOF
> > +
> > +# Before fix of BUG#38468, this would generate some warnings
> > +--exec $MYSQL_BINLOG $MYSQLTEST_VARDIR/mysqld.1/data/master-bin.000001
> >/dev/null 2> $MYSQLTEST_VARDIR/tmp/mysqlbinlog.warn
>
> Comment: I was wondering if /dev/null here could cause the test to fail
> under different platforms (like windows). PB runs the test under cygwin
> (if I am not mistaken) so this is ok for PB, but probably not for native
> windows. In any case, I can see that other test cases in the same file
> use the same pattern, so lets not make a fuss out of this. ;)
>
I think /dev/null should not be a problem, I found lots of usage of
this, there are usages of it in this test case (mysqlbinlog.test)
already.
> Question: Why not use $MYSQLTEST_DATADIR= `SELECT @@datadir;`; instead
> of $MYSQLTEST_VARDIR? Can '$MYSQLTEST_VARDIR/mysqld.1/data/...' cause
> test failure if mtr is usign parallel execution and happens to schedule
> this test to run on say instance 3 of mysqld (wouldn't then the path
> change to $MYSQLTEST_VARDIR/mysqld.3/... ?).
>
You're right, I'll change it to use $MYSQLTEST_DATADIR, thx
> > +
> > +# Make sure the command above does not generate any error or warnings
> > +diff_files $MYSQLTEST_VARDIR/tmp/mysqlbinlog.warn
> $MYSQLTEST_VARDIR/tmp/mysqlbinlog.warn.empty;
> > +
> > +# Cleanup for this part of test
> > +remove_file $MYSQLTEST_VARDIR/tmp/mysqlbinlog.warn.empty;
> > +remove_file $MYSQLTEST_VARDIR/tmp/mysqlbinlog.warn;
> >
> --
> Luís
>
>
