On Sun, 18 Dec 2011, Gokhan Sengun wrote:

> I have implemented required functionality to make "Establishing data
> connections in FTP non-blocking for multi interface".

Great! Many thanks for your work on this. In general it looks very good and it
ran all the tests fine in my end as well.

I only have some comments and two questions.

- I converted your patch to be a patch on top of my previous change so
   that it would be easier to review separately. See the attach file.

- I made both new error codes re-use obsoleted numbers. I updated the code
   and test cases accordingly.

- I removed one of the new options, there's no need to have two for the same
   thing. Previous options that have one for full seconds and one for
   milliseconds were added before we had any millisecond options at all.

- I fixed test case 1119 (the symbols check) to run fine.

Now for my questions:

- Line 2359 in lib/ftp.c now has a comment that says:
   /* TODO: gseng - do it once for connected */

   Can you explain that for us? Do what and why is it a TODO and not done now?

- You only did a very minor change to lib/multi.c but I don't like it:

      case CURLM_STATE_DO_DONE:
+
+ if(easy->easy_conn->bits.wait_data_conn == TRUE) {
+ multistate(easy, CURLM_STATE_DO_MORE);
+ result = CURLM_OK;
+ break;
+ }
+

    Why would you switch to the CURLM_STATE_DO_DONE state in the first place if
    the conditions say it should actually be in the CURLM_STATE_DO_MORE state?
    I don't like this. The condition should be added to the code that changed
    the state in the first place I think, unless you have a proper motivation
    for this peculiarity.

-- 
  / daniel.haxx.se

-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html