Magnus, hej.
Thanks for your notes!
Newer patch will account them.
> Andrei Elkin skrev:
>> Below is the list of changes that have just been committed into a local
>> 6.0 repository of aelkin. When aelkin does a push these changes
>> will be propagated to the main repository and, within 24 hours after the
>> push, to the public repository.
>> For information on how to access the public repository
>> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>>
>> ChangeSet@stripped, 2008-05-02 21:32:15+03:00, aelkin@stripped +3 -0
>> Bug #35546 mysqlbinlog.cc: Load_log_processor::process_first_event() fails
> creating unique
>> Bug #35543 mysqlbinlog.cc does not properly work with tmp files
>> There were failures on pb executions in that mysqlbinlog could
>> not compose a new unique file name of its purpose.
>> The reason of the bug has been that mysqlbinlog used the system wide temprorary
> directory instead
>> of the specified by mtr so that mysqlbinlog's temporary files happened to be
> out of reach of mtr.
>> Logics of the mysqlbinlog program correctly allows leaving new
>> produced files such as ones that hold LOAD DATA data.
>> This is fine as long as a caller of mysqlbinlog takes care of the new files
> later.
>> The problem fixed with specifying explicitly the temp directory
>> for mysqlbinlog when it's called
>> from mtr. New files mysqlbinlog can produce now reside in the mtr temporary
> directory.
>> Also there were memory leaks reported (bug#35543) which are
>> fixed with finding out the sources and deploying
>> freeing.
>>
>>
>> client/mysqlbinlog.cc@stripped, 2008-05-02 21:32:12+03:00,
> aelkin@stripped +5 -1
>> Bug #35543 memory leak issue.
>> Fixed with freeing at error branches of
> Load_log_processor::process_first_event()
>>
>> mysql-test/mysql-test-run.pl@stripped, 2008-05-02 21:32:12+03:00,
> aelkin@stripped +17 -1
>> adding --local-load= $opt_tmpdir argument to mysqlbinlog.
>>
>> mysql-test/suite/binlog/t/binlog_killed_simulate.test@stripped, 2008-05-02
> 21:32:12+03:00, aelkin@stripped +1 -1
>> changing to portable macro.
>>
>> diff -Nrup a/client/mysqlbinlog.cc b/client/mysqlbinlog.cc
>> --- a/client/mysqlbinlog.cc 2008-03-05 12:19:29 +02:00
>> +++ b/client/mysqlbinlog.cc 2008-05-02 21:32:12 +03:00
>> @@ -438,6 +438,7 @@ Exit_status Load_log_processor::process_
>> {
>> error("Could not construct local filename %s%s.",
>> target_dir_name,bname);
>> + my_free(fname, MYF(0));
>> delete ce;
>> DBUG_RETURN(ERROR_STOP);
>> }
>> @@ -448,6 +449,7 @@ Exit_status Load_log_processor::process_
>> if (set_dynamic(&file_names, (uchar*)&rec, file_id))
>> {
>> error("Out of memory.");
>> + my_free(fname, MYF(0));
>
> magnus: ok, fname is a variable allocated with my_alloc in the
> begining of function.
>
>> delete ce;
>> DBUG_RETURN(ERROR_STOP);
>> }
>> @@ -458,11 +460,13 @@ Exit_status Load_log_processor::process_
>> if (my_write(file, (uchar*)block, block_len, MYF(MY_WME|MY_NABP)))
>> {
>> error("Failed writing to file.");
>> + my_free(fname, MYF(0));
>> retval= ERROR_STOP;
>> }
>> if (my_close(file, MYF(MY_WME)))
>> {
>> error("Failed closing file.");
>> + my_free(fname, MYF(0));
>
> magnus not ok! potential double free of "fname" if first "my_write"
> fails and then "my_close" also fails. Maybe need to set "fname= 0" at
> first close and then use MYF(MY_ALLOW_ZERO_PTR) flag?
You set of eyes is exactly what's needed!
I corrected this double freeing with doing
if (retval != OK_CONTINUE)
my_free(fname, MYF(0));
after the 2nd if and before to leave from the function
>
>> retval= ERROR_STOP;
>> }
>
>> DBUG_RETURN(retval);
>> @@ -823,7 +827,7 @@ Exit_status process_event(PRINT_EVENT_IN
>> print_event_info->common_header_len=
>> glob_description_event->common_header_len;
>> ev->print(result_file, print_event_info);
>> - ev->temp_buf= 0; // as the event ref is zeroed
>> + ev->free_temp_buf(); // as the event ref is zeroed
>
> magnus: This one is impossible for me to review.
This is understandable.
There was a valgrind complaint in that non-zero temp_buf for Format
Desc event is possible only with dynamic allocation. So usage of the
method is necessary.
Needless to say valgrind is quite after this hunk.
>
>> /*
>> We don't want this event to be deleted now, so let's hide it (I
>> (Guilhem) should later see if this triggers a non-serious Valgrind
>> diff -Nrup a/mysql-test/mysql-test-run.pl b/mysql-test/mysql-test-run.pl
>> --- a/mysql-test/mysql-test-run.pl 2008-04-28 11:19:40 +03:00
>> +++ b/mysql-test/mysql-test-run.pl 2008-05-02 21:32:12 +03:00
>> @@ -987,6 +987,22 @@ sub client_arguments ($) {
>> }
>> +sub mysqlbinlog_arguments ($) {
> magnus: remove ^
>> + my $tmp_dir= shift;
> magnus: remove^
2 ok
>
>> + my $exe= mtr_exe_exists("$path_client_bindir/mysqlbinlog");
>> +
>> + my $args;
>> + mtr_init_args(\$args);
>> + mtr_add_arg($args, "--defaults-file=%s", $path_config_file);
>> + if (!($tmp_dir eq ""))
>> + {
>
> magnus: it will always be set, drop the "if"
ok
>
>> + mtr_add_arg($args, "--local-load=%s", $tmp_dir);
> magnus: Just use $opt_tmpdir
>> + }
>> + client_debug_arg($args, "mysqlbinlog");
>> + return mtr_args2str($exe, @$args);
>> +}
>> +
>> +
>> sub mysqlslap_arguments () {
>> my $exe= mtr_exe_maybe_exists("$path_client_bindir/mysqlslap");
>> if ( $exe eq "" ) {
>> @@ -1218,7 +1234,7 @@ sub environment_setup {
>> $ENV{'MYSQL_SLAP'}= mysqlslap_arguments();
>> $ENV{'MYSQL_IMPORT'}= client_arguments("mysqlimport");
>> $ENV{'MYSQL_SHOW'}= client_arguments("mysqlshow");
>> - $ENV{'MYSQL_BINLOG'}= client_arguments("mysqlbinlog");
>> + $ENV{'MYSQL_BINLOG'}= mysqlbinlog_arguments($opt_tmpdir);
> magnus: No need to pass $opt_tmpdir, it's a global variable and always set
ok
>
>> $ENV{'MYSQL'}= client_arguments("mysql");
>> $ENV{'MYSQL_UPGRADE'}= client_arguments("mysql_upgrade");
>> $ENV{'MYSQLADMIN'}= native_path($exe_mysqladmin);
>> diff -Nrup a/mysql-test/suite/binlog/t/binlog_killed_simulate.test
> b/mysql-test/suite/binlog/t/binlog_killed_simulate.test
>> --- a/mysql-test/suite/binlog/t/binlog_killed_simulate.test 2008-04-08 09:34:29
> +03:00
>> +++ b/mysql-test/suite/binlog/t/binlog_killed_simulate.test 2008-05-02 21:32:12
> +03:00
>> @@ -63,7 +63,7 @@ let $error_code= `select @a like "%#%err
>> eval select $error_code /* must return 0 to mean the killed query is in */;
>> # cleanup for the sub-case
>> -system rm $MYSQLTEST_VARDIR/tmp/binlog_killed_bug27571.binlog;
>> +remove_file $MYSQLTEST_VARDIR/tmp/binlog_killed_bug27571.binlog;
>> drop table t1,t2;
>>
>
cheers,
Andrei
