Re: [Maria-developers] [Commits] Rev 3170: After-merge fixes for 5.5 merge. in http://bazaar.launchpad.net/~maria-captains/maria/5.5
Hi Monty, While fixing test failures in the MariaDB 5.5 tree, I encountered problems in sql_string, related to changes you did there in 5.2. I managed to come up with some solution, but please see below patch and see if you can suggest something better. Two issues: 1. You added realloc_with_extra() / realloc_with_extra_if_needed(), and used them to replace calls to realloc(). But realloc() as part of its operation puts a terminating '\0' at the end of the allocation; with your change this is now at a different (and not very useful) position in the buffer. My change tries to put it in the original place, but it is not terribly elegant, and perhaps your original change needs to be rethought. 2. I found a terrible mess (IMHO): We have two distict, subtly differing, sql_string.{h,cc}, one in /sql/ and one in /client/. We even seem to have a requirement for ABI compatibility between the two, since mysql_embedded uses client/sql_string.h with sql/sql_string.cc :-( And in fact your change seems to break such ABI compatibility, so I suspect mysql_embedded might be subtly broken in 5.2. But why do we need to have two sql_string in the first place? Especially given that they are apparently sufficiently close to be ABI compatible ... can't we eliminate on of them to get at least a little sanity into the sql_string story? - Kristian. knielsen@knielsen-hq.org writes:
At http://bazaar.launchpad.net/~maria-captains/maria/5.5
------------------------------------------------------------ revno: 3170 revision-id: knielsen@knielsen-hq.org-20111209144027-yf5as4v9lb6jt3s7 parent: knielsen@knielsen-hq.org-20111208170848-pijq4i8ceqwcl19q committer: knielsen@knielsen-hq.org branch nick: mariadb-5.5 timestamp: Fri 2011-12-09 15:40:27 +0100 message: After-merge fixes for 5.5 merge.
Fix typo causing too low timeout value for wait_for_slave_param.inc. Fix wrong/missing '\0' termination in String:realloc() after Monty's optimisations to reduce reallocation calls. === modified file 'mysql-test/include/wait_for_slave_param.inc' --- a/mysql-test/include/wait_for_slave_param.inc 2010-12-20 14:15:01 +0000 +++ b/mysql-test/include/wait_for_slave_param.inc 2011-12-09 14:40:27 +0000 @@ -79,7 +79,7 @@ if ($_slave_check_configured == 'No such
# mysqltest doesn't provide any better way to multiply by 10 --let $_wait_for_slave_param_zero= 0 ---let $_slave_timeout_counter= $_slave_timeout$zero +--let $_slave_timeout_counter= $_slave_timeout$_wait_for_slave_param_zero --let $_slave_continue= 1 while ($_slave_continue) {
=== modified file 'sql/rpl_handler.cc' --- a/sql/rpl_handler.cc 2011-11-03 18:17:05 +0000 +++ b/sql/rpl_handler.cc 2011-12-09 14:40:27 +0000 @@ -397,7 +397,7 @@ int Binlog_transmit_delegate::before_sen
int ret= 0; FOREACH_OBSERVER(ret, before_send_event, thd, - (¶m, (uchar *)packet->c_ptr(), + (¶m, (uchar *)packet->c_ptr_safe(), packet->length(), log_file+dirname_length(log_file), log_pos)); return ret; @@ -411,7 +411,7 @@ int Binlog_transmit_delegate::after_send
int ret= 0; FOREACH_OBSERVER(ret, after_send_event, thd, - (¶m, packet->c_ptr(), packet->length())); + (¶m, packet->c_ptr_safe(), packet->length())); return ret; }
=== modified file 'sql/sql_string.cc' --- a/sql/sql_string.cc 2011-11-03 18:17:05 +0000 +++ b/sql/sql_string.cc 2011-12-09 14:40:27 +0000 @@ -80,8 +80,12 @@ bool String::real_alloc(uint32 length) no allocation occured.
@retval true An error occured when attempting to allocate memory. + + The _internal() version is just to share code between realloc() and + realloc_with_extra(), so that we can put the NULL termination at the right + position. */ -bool String::realloc(uint32 alloc_length) +bool String::realloc_internal(uint32 alloc_length) { if (Alloced_length <= alloc_length) { @@ -109,6 +113,13 @@ bool String::realloc(uint32 alloc_length Ptr= new_ptr; Alloced_length= len; } + return FALSE; +} + +bool String::realloc(uint32 alloc_length) +{ + if (realloc_internal(alloc_length)) + return TRUE; Ptr[alloc_length]=0; // This make other funcs shorter return FALSE; }
=== modified file 'sql/sql_string.h' --- a/sql/sql_string.h 2011-11-03 18:17:05 +0000 +++ b/sql/sql_string.h 2011-12-09 14:40:27 +0000 @@ -255,12 +255,18 @@ public: return real_alloc(arg_length); } bool real_alloc(uint32 arg_length); // Empties old string +private: + bool realloc_internal(uint32 arg_length); +public: bool realloc(uint32 arg_length); bool realloc_with_extra(uint32 arg_length) { if (extra_alloc < 4096) extra_alloc= extra_alloc*2+128; - return realloc(arg_length + extra_alloc); + if (realloc_internal(arg_length + extra_alloc)) + return TRUE; + Ptr[arg_length]= 0; + return FALSE; } bool realloc_with_extra_if_needed(uint32 arg_length) {
_______________________________________________ commits mailing list commits@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
Hi, Kristian! On Dec 09, Kristian Nielsen wrote:
Hi Monty,
While fixing test failures in the MariaDB 5.5 tree, I encountered problems in sql_string, related to changes you did there in 5.2. I managed to come up with some solution, but please see below patch and see if you can suggest something better.
Two issues:
1. You added realloc_with_extra() / realloc_with_extra_if_needed(), and used them to replace calls to realloc(). But realloc() as part of its operation puts a terminating '\0' at the end of the allocation; with your change this is now at a different (and not very useful) position in the buffer. My change tries to put it in the original place, but it is not terribly elegant, and perhaps your original change needs to be rethought.
As discussed on irc, I've fixed that in 5.5-serg
2. I found a terrible mess (IMHO): We have two distict, subtly differing, sql_string.{h,cc}, one in /sql/ and one in /client/. We even seem to have a requirement for ABI compatibility between the two, since mysql_embedded uses client/sql_string.h with sql/sql_string.cc :-( And in fact your change seems to break such ABI compatibility, so I suspect mysql_embedded might be subtly broken in 5.2.
I had it fixed, but then another change broke it again. Now I'm trying to get rid of client/sql_string.* Regards, Sergei
Hi! Sorry for the delayed reply (but better late than never).
"Kristian" == Kristian Nielsen
writes:
Kristian> Hi Monty, Kristian> While fixing test failures in the MariaDB 5.5 tree, I encountered problems in Kristian> sql_string, related to changes you did there in 5.2. I managed to come up with Kristian> some solution, but please see below patch and see if you can suggest something Kristian> better. Kristian> Two issues: Kristian> 1. You added realloc_with_extra() / realloc_with_extra_if_needed(), and Kristian> used them to replace calls to realloc(). But realloc() as part of its Kristian> operation puts a terminating '\0' at the end of the allocation; with your Kristian> change this is now at a different (and not very useful) position in the Kristian> buffer. My change tries to put it in the original place, but it is not Kristian> terribly elegant, and perhaps your original change needs to be rethought. The \0 should not have been required at the end; In some cases it's good that it's there as we can avoid an extra realloc. Code that assumes that there is always an extra \0 at end is wrong code and need to be fixed; There is many different usage of the library that can cause the code not to end with \0 even before. When adding the above functions I did find some cases in MySQL where I had to change to use ->c_ptr_safe() instead of just ->ptr() but all those cases was a potential bug in the original code. Conclusion: Don't fix realloc. Instead fix the wrong call that is using the code! Kristian> 2. I found a terrible mess (IMHO): We have two distict, subtly differing, Kristian> sql_string.{h,cc}, one in /sql/ and one in /client/. We even seem to have a Kristian> requirement for ABI compatibility between the two, since mysql_embedded uses Kristian> client/sql_string.h with sql/sql_string.cc :-( And in fact your change seems Kristian> to break such ABI compatibility, so I suspect mysql_embedded might be subtly Kristian> broken in 5.2. Sergei did fix the above by making the sql_string.cc files identical. Kristian> But why do we need to have two sql_string in the first place? Especially given Kristian> that they are apparently sufficiently close to be ABI compatible ... can't we Kristian> eliminate on of them to get at least a little sanity into the sql_string Kristian> story? The reason was originally that sql_string.cc did at some point call other server code that was not in the client. This seams to not be the case anymore so now we can use identical versions of sql_string.cc Regards, Monty
participants (3)
-
Kristian Nielsen
-
Michael Widenius
-
Sergei Golubchik