Andrei Elkin wrote:
> Alfranio, hello.
>
>
>> #At
> file:///home/acorreia/workspace.sun/repository.mysql/bzrwork/bug-43949/mysql-5.1-bugteam/
> based on revid:v.narayanan@stripped
>>
>> 2862 Alfranio Correia 2009-04-09
>> BUG#43949 Initialization of slave produces a warning message in Valgrind
>>
>> In order to define the --slave-load-tmpdir, the init_relay_log_file()
>> was calling fn_format(MY_PACK_FILENAME) which internally was calling
>> strmov_overlapp() and the following warning message was being printed
>> out while running in Valgrind: "source and destination overlap in strcpy".
>>
>> We fixed the issue by removing the flag MY_PACK_FILENAME as it was not
>> necessary. In a nutshell, with this flag the function fn_format() tried
>> to replace a directory by either "~", "." or "..". However, we wanted
>> exactly the opposite which is achieved by the flag MY_UNPACK_FILENAME.
>>
>> In this patch, we also refactored the functions init_relay_log_file()
>> and check_temp_dir(). The former was refactored to call the fn_format()
>> with the flag MY_SAFE_PATH along with two more flags, MY_UNPACK_FILENAME
>> and MY_RETURN_REAL_PATH, in order to avoid issues with long directories,
>> get rid off "~", "." and ".." and return an absolute path, respectively.
>> This result is stored in an rli variable, which is then processed by the
>> other function in order to verify if the directory exists and if we are
>> able to create files in it.
>> modified:
>> sql/rpl_rli.cc
>> sql/slave.cc
>>
>> === modified file 'sql/rpl_rli.cc'
>> --- a/sql/rpl_rli.cc 2009-02-22 13:40:52 +0000
>> +++ b/sql/rpl_rli.cc 2009-04-09 21:52:44 +0000
>> @@ -105,7 +105,7 @@ int init_relay_log_info(Relay_log_info*
>> rli->tables_to_lock_count= 0;
>>
>> fn_format(rli->slave_patternload_file, PREFIX_SQL_LOAD, slave_load_tmpdir,
> "",
>> - MY_PACK_FILENAME | MY_UNPACK_FILENAME |
>> + MY_SAFE_PATH | MY_UNPACK_FILENAME |
>> MY_RETURN_REAL_PATH);
>>
>
> Good. the "annihilating" line got corrected.
>
> Now fn_format() is capable to return NULL due to MY_SAFE_PATH.
> I think we are better to check the return value.
> Maybe it's unnecessary to have a dedicate test for the error
> propagation, but I strongly recommend to simulate failures in all use
> cases calling init_relay_log_info().
>
Sure. I will check if null is returned.
If you want I can create a test that will force returning null but
I think it is not necessary.
>
>> rli->slave_patternload_file_size= strlen(rli->slave_patternload_file);
>>
>>
>> === modified file 'sql/slave.cc'
>> --- a/sql/slave.cc 2009-04-08 10:07:24 +0000
>> +++ b/sql/slave.cc 2009-04-09 21:52:44 +0000
>> @@ -2638,14 +2638,21 @@ err:
>> LOAD DATA INFILE.
>> */
>> static
>> -int check_temp_dir(char* tmp_dir, char *tmp_file)
>> +int check_temp_dir(char* tmp_file)
>> {
>> int fd;
>> MY_DIR *dirp;
>> + char tmp_dir[FN_REFLEN];
>> + size_t tmp_dir_size;
>>
>> DBUG_ENTER("check_temp_dir");
>>
>> /*
>> + Get the directory from the temporary file.
>> + */
>> + dirname_part(tmp_dir, tmp_file, &tmp_dir_size);
>> +
>> + /*
>> Check if the directory exists.
>> */
>> if (!(dirp=my_dir(tmp_dir,MYF(MY_WME))))
>> @@ -2800,7 +2807,7 @@ log '%s' at position %s, relay log '%s'
>>
> llstr(rli->group_master_log_pos,llbuff),rli->group_relay_log_name,
>> llstr(rli->group_relay_log_pos,llbuff1));
>>
>> - if (check_temp_dir(slave_load_tmpdir, rli->slave_patternload_file))
>> + if (check_temp_dir(rli->slave_patternload_file))
>> {
>> rli->report(ERROR_LEVEL, thd->main_da.sql_errno(),
>> "Unable to use slave's temporary directory %s - %s",
>>
>>
>
> I agree, it looks much better now.
>
>
> cheers,
>
> Andrei
>
Cheers.
