On Mon, 24 May 2004, Alexander Krasnostavsky wrote:

> The implementation of FTP third party transfer was finished.
> I worked on curl-7.12.0-20040519.

Thanks for your contribution and for the efforts you donate to the project!
I'm fascinated you could squeeze this functionality in using so little source
changes.

I don't think we'll get these changes into the code base before the upcoming
7.12.0 release. I think we have several issues in here left to settle first,
as you'll see below!

My comments on the patch:

 * You totally ignore the current code style and use your own indent levels
   all over (even including - shrug - TABs). Please adjust to the currently
   used curl-style. It makes the code easier to read if it is consistent.
   (spaces only, indent 2 spaces per level)

   Alote note that we never ever write code wider than 79 columns.

 * Please don't add the "changed by Alexander Krasnostavsky" comments all
   over. On several places it would be better if you documented the code
   instead, like in lib/urldata.h.

 * The provided example app that uses these new options is great!

 * CURLOPT_SOURCE_PPEQUOTE is a spelling mistake in the code. Did you compile
   this?

 * You always set the "primary" (is that the source?) connection to use PASV
   and the second to use PORT. Won't people want that configurable?

 * The code that extracts the PASV-response seems to be too naive on the
   format of the response data:

      strcpy(pasvPort, str);
      pasvPort[strlen(str) - 2] = '\0';

   This is an obvious buffer overflow risk.

 * This code (in transfer.c) should better use snprintf():

    sprintf(url, "%s://%s/", conn->protostr, data->set.ftp_source_host);

 * At almost the same spot as the above mentioned code, you clone a full
   'SessionHandle' with memcpy(). That is major badness as you won't be able
   to know properly what data that is then allocated for real in the new copy
   or which allocated status you copied from the original struct. You only
   change three members of that huge struct after the copy. Perhaps it would
   make more sense to pass in those options another way?

 * Code-wise, I think it would make sense to call the second connection
   something else than 'ftp_source_host' since it would make the generic code
   less focused on this as a FTP-feature. We could just as well let the code
   outside of ftp.c remain unaware of what protocol it is that needs two
   connections (perhaps we'll introduce other protocols in the future that use
   two connections).

 * sec_conn is a struct field within the connectdata struct. I find that hard
   to make sense. Besides, how does this all affect connection re-use?

 * I miss the documentation of all these new options and how they should be
   used.

 * Have you tried/verified that this works using the multi interface as well?

 * Have you given any thoughts on the ability to write/create test cases that
   verify this functionality using the powers of the curl test suite?

 * Any thoughts on how the syntax/options would be to allow the command line
   tool curl to use this new ability?

 * I don't see where/how the sec_conn connection is closed down after use.
   Have you tried the README.memoryleak approach to verify that you are doing
   all the cleanups nicely?

 * Regarding your changes to Curl_debug() to include the host name as a new
   parameter. First, I think host.dispname is what should be used when sending
   the hostname to debug tracing. Then, I'm not very happy with how the debug
   callback doesn't get the same info as the internal function which needs to
   be addressed somehow. I guess the hostname can be sent with an info flag
   separately or something. I'm open for suggestions.

Again: I appreciate your contribution!

-- 
     Daniel Stenberg -- http://curl.haxx.se -- http://daniel.haxx.se
      Dedicated custom curl help for hire: http://haxx.se/curl.html