curl-library
RE: FTP third party transfer (proxy) support.
Date: Tue, 25 May 2004 13:30:42 +0200 (CEST)
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.htmlReceived on 2004-05-25