maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxxx>]

Hi, Michael!

On Oct 18, Michael Widenius wrote:
> +
> +    // create a background thread to handle urls, if any
> +    if (url_count)
> +    {
> +      pthread_mutex_init(&sleep_mutex, 0);
> +      pthread_cond_init(&sleep_condition, 0);
> +      shutdown_plugin= false;
> 
> Please add a comment why you set 'shutdown_plugin' to false (as this
> is not a variable only for the feedback plugin).

This is a variable only for the feedback plugin.

> +      pthread_attr_t attr;
> +      pthread_attr_init(&attr);
> +      pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE);
> +      if (pthread_create(&sender_thread, &attr, background_thread, 0) != 0)
> +      {
> 
> --- a/plugin/feedback/sender_thread.cc	1970-01-01 00:00:00 +0000
> +++ b/plugin/feedback/sender_thread.cc	2010-09-30 14:24:31 +0000
...
> +      if (thd) // for nicer SHOW PROCESSLIST
> +        thd->set_query(const_cast<char*>(url->url()), url->url_length());
> 
> Wouldn't it be better if url->url() would return const char * ?

It does return const char*, but thd->set_query() wants char* (for no
good reason), so I need to cast const away.

> +
> +      if (url->send(str.c_ptr(), str.length()))
> 
> Use str.ptr() instead of str,c_ptr() (We already checked that this is ok)

Why?

> === added file 'plugin/feedback/url_base.cc'
> 
> +Url* Url::create(const char *url, size_t url_length)
> +{
> +  url= my_strndup(url, url_length, MYF(MY_WME));
> 
> my_strndup() -> my_strmake()

Pardon me?
There's no such a function as my_strmake(). And strmake() only copies the
string, while my_strndup() allocates and copies. I need the latter.
 
> +  if (!url)
> +    return NULL;
> +
> --- a/plugin/feedback/url_http.cc	1970-01-01 00:00:00 +0000
> +++ a/plugin/feedback/url_http.cc	1970-01-01 00:00:00 +0000
> <cut>
> 
> +Url* http_create(const char *url, size_t url_length)
> +{
> +  const char *s;
> +  LEX_STRING full_url= {const_cast<char*>(url), url_length};
> +  LEX_STRING host, port, path;
> 
> Would it not be better to introduce LEX_CONS_STRING and use these to
> avoid const away casts? 

Perhaps, if I'd have more casts because of that. But just because of one
or two - I'd rather keep the code uniform instead and use LEX_STRING
everywhere.
 
> +  bool ssl= false;
> +
> <cut>
> 
> +    /*
> +      if the data were send successfully, read the reply.
> +      Extract the first string between <h1>...</h1> tags
> +      and put it as a server reply into the error log.
> +    */
> +    len= vio_read(vio, (uchar*)buf, sizeof(buf)-1);
> +    if (len && len < sizeof(buf))
> 
> Isn't len guaranteed to be < sizeof(buf) here ?
> I would change the check to 'if (len > 0)'

len is unsigned. vio_read() can return (uint)-1
which indicates and error and is certainly > sizeof(buf)
 
> +    {
> +      char *from;
> +
> +      buf[len+1]= 0; // safety
> +
> 
> Other ideas and suggestions:
> 
> - It would be very imporant for us to know which plugins are
>   loaded in the feedback. How can we do that ? (I assume we can't get
>   that information with the current code)

We do it. fill_plugin_version() function in the utils.cc gets the list
of installed plugins and their versions.

By the way, I've added few more lines of information to the report -
after you've seen the code. On my computer they contain:

Uname_sysname        Linux
Uname_release        2.6.34-gentoo-r6
Uname_version        #1 SMP Mon Sep 6 15:26:42 CEST 2010
Uname_machine        x86_64
Uname_distribution   Gentoo Base System release 1.12.13
 
> From my point of view, when you have fixed the bugs and considered the
> suggestions it's ok to push in 5.1.

Thanks for the review! You've catched quite a few problems in the code.
 
Regards,
Sergei