Hi! On Thu, Apr 1, 2021 at 12:03 AM Sergei Golubchik <serg@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