Hi! On Thu, Apr 1, 2021 at 12:03 AM Sergei Golubchik <s...@mariadb.org> wrote:
<cut> > > > > - DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %s", > > > > + DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %.*s", > > > > part_def->part_id, part_def->is_subpart, > > > > - part_name)); > > > > + length, part_name)); > > > > > > These two changes contradict each other. Either part_name must be > > > \0-terminated, and then you don't need to specify a length in printf. > > > Or it is not \0-terminated and you must specify a length. > > > > part_name is not null terminated and that is why I had to add .* and length. > > What is the contradiction ? > > You've cut too much from my email. "contradiction" referred to the > change just above this one, it was: > > > - @param part_name Partition name to match. > > + @param part_name Partition name to match. Must be \0 terminated! > > so, please, decide what it is. Either the comment is wrong or the code > is redundant. Neither. The comment is right. part_name must at this point in time be null terminated, but may not have to be that in the future. The printf string for DBUG_PRINT is correct. It uses the length, which avoids a strlen() inside sprintf() and is also future proof. The only reason that part_name needs to be null terminated is because of this code: if (!part_def) { my_error(ER_UNKNOWN_PARTITION, MYF(0), part_name, table->alias.c_ptr()); DBUG_RETURN(true); } Whenever we fix the error message to use %.*s, then we can lift the restrictions that part_name needs to be null terminated. Regards, Monty > > > > @@ -1254,21 +1262,16 @@ bool String::append_semi_hex(const char *s, > > > > uint len, CHARSET_INFO *cs) > > > > // Shrink the buffer, but only if it is allocated on the heap. > > > > void Binary_string::shrink(size_t arg_length) > > > > { > > > > - if (!is_alloced()) > > > > - return; > > > > - if (ALIGN_SIZE(arg_length + 1) < Alloced_length) > > > > + if (is_alloced() && ALIGN_SIZE(arg_length + 1) < Alloced_length) > > > > + { > > > > + char *new_ptr; > > > > + /* my_realloc() can't fail as new buffer is less than the original > > > > one */ > > > > + if ((new_ptr= (char*) my_realloc(STRING_PSI_MEMORY_KEY, Ptr, > > > > arg_length, > > > > > > you don't need an if() because, as you wrote yourself "my_realloc() > > > can't fail" here. > > > > Yes, this should be true, but one never knows with allocation libraries. > > There is implementation of realloc that makes a copy and deletes the old > > one. > > However I am not sure what happens if there is no place to make a copy. > > Better safe than sorry. > > No. I've fixed this quite a while ago. my_realloc() can *never* return > NULL when reducing a buffer size. System realloc(), indeed, can, I've > researched that topic when making the change. But my_realloc() has a > protection against it and it never returns NULL in this case. > We have few places in the code that rely on it. We also have code that relies on that my_malloc() never fails. Note what this commit did was just an cleanup of the original code, not trying to do any big changes of logic, except for c_ptr(). Anyway, I can remove the if here to keep you happy. > > > > + { > > > > + Ptr[str_length]=0; > > > > + return Ptr; > > > > + } > > > > + (void) realloc(str_length+1); /* This will add end > > > > \0 */ > > > > return Ptr; > > > > } > > > > + /* Use c_ptr() instead. This will be deleted soon, kept for > > > > compatiblity */ > > > > inline char *c_ptr_quick() > > > > { > > > > - if (Ptr && str_length < Alloced_length) > > > > - Ptr[str_length]=0; > > > > - return Ptr; > > > > + return c_ptr_safe(); > > > > > > 1. why not to remove it now? > > > 2. it's strange that you write "use c_ptr() instead" but you actually > > > use c_ptr_safe() instead. > > > > What I meant is that the developer should instead use c_ptr(). > > I will make the message more clear. > > I decided to call c_ptr_safe() here as this is the most safe > > version to use. In practice the new c_ptr() should be safe > > for 99% of our code. > > Let's remove c_ptr_quick() now. Why keep it? Not enough time to do it now. I rather work on fixing review comments. Regards, Monty _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp