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