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
Received on 2011-01-28