On Thu, 27 Jan 2011, Всеволод Новиков wrote:
The applied patch (against the developer branch) is proposed to be included
into the curl because:
Great! I really like the look of this and I must say that this is truly nice
work. This is certainly almost there now. I do have a few nits that I'd like
to see corrected:
* Failed test cases with a c-ares enabled (lib)curl:
241 528 530 540 555
I tested on my Debian Linux and the problems seem reliably repeatable
Failed test cases with the threaded resolver:
532 536 573
These do however seem to be less reliable and might depend on some kind of
race. If I for example run them with valgrind I don't get the problems,
presumably because it slows down the execution just enough.
With the synchronous resolver everything ran fine for me.
* source code style cleanup:
1 - functions have their starting brace ({) in column 0 (the left-most)
2 - no source code line is longer than 79 columns
* a pointer is set to NULL to get "cleared", not 0:
+ res->temp_ai = 0;
* I dislike compound statements such as:
if((status = ares_init((ares_channel*)resolver)) != ARES_SUCCESS) {
I prefer them to be written as separate statements as I think it
makes much more readable and more maintainable code:
status = ares_init((ares_channel*)resolver);
if(ARES_SUCCESS != status) {
* I detected a calloc call that doesn't check return code. It will cause
torture test failures (memory leaks and/or crashes in OOM situations).
hostares.c:568 with the patch applied
Similarly, you call Curl_he2ai() without properly taking care of a possible
NULL pointer returned if it runs out of memory.
* This must be a mistake:
@@ -250,7 +374,7 @@ CURLcode Curl_wait_for_resolv(struct connectdata *conn,
else
timeout_ms = 1000;
- waitperform(conn, timeout_ms);
+ Curl_is_resolved(conn,&temp_entry);
if(conn->async.done)
break;
Your replacement doesn't wait for any period, so you'll busy-loop in this
function...
* I don't like that the 'resolver' pointer in urldata.h (in the SessionHandle
struct) is a void * as it leads to many typecasts and unchecked arguments. I
have an alternative idea: make it 'struct Curl_resolver *' and forward
declare the struct in urldata.h and only define it within the actual
resolver C files. Then each resolver can have its own definition and can
avoid typecasts!
--
/ daniel.haxx.se-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
