> 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!

When you are planning to release 7.12.0?

> * 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.

These are not a big issues, I will fix it send you the "diff" as you
asked.

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

Right, I fixed it before and probably forget to update you.

> * 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?

As far as I know, both remote hosts should support third party transfer,
meaning both of them support passive mode. I just apply to RFC 959 -
that is the way the transfer done between hosts, the "primary" is the
target (according to RFC 959) and I am not sure that it can be switched.
So I don't think it is important which one is PASV and which one is
PORT.

> * 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.

OK, I changed it to:
snprintf(pasvPort, sizeof(pasvPort), "%d,%d,%d,%d,%d,%d", ip[0], ip[1],
         ip[2], ip[3], port[0], port[1]);

> * This code (in transfer.c) should better use snprintf():
> sprintf(url, "%s://%s/", conn->protostr,
data->set.ftp_source_host);

Already changed to:
snprintf(url, sizeof(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?

But this is exactly what I mean. I want all the data from the primary
connection to be copied to the secondary, and only three members should
be changed: source host, source port and source user:password. It means
that except these three members all the data should be the same, like
file data type. I did it because I am not sure what data from
'SessionHandle' is relevant for FTP. Indeed the relevant data can be
copied field by field, but using memcpy(), the data will be cloned
always, even when new fields will be added in future. You can change it
in future if you decide that another way is better.

> * 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).

There's something in what you say, but the reason I called it "source"
is because it is not just a connection to a host, but because it is an
interaction between two remote hosts, where one of them source and
another target. I can't see how other protocols can use two connections
without interaction between the hosts; otherwise, still one of them will
be the source.
Another solution is to go back to the original word (that used in FTP
CLI) "proxy" ('ftp_proxy_host'). In FTP CLI third party transfer used
with word "proxy":
  proxy open
  proxy get
  proxy put
and so on.

Both options are acceptable for me. I have no problem to accept your
options for the name.

> * 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?

In third party transfer mode I made the "connectdata struct" one entity,
because this is the only way to pass sec_conn with the original conn to
all functions without changing the interface. It is not affect
connection reuse because connection itself created by the same function.
I test it by performing regular transfer (using the same host and user)
after the third party transfer, and I saw that the connection was
reused.

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

CURLOPT_FTP_SOURCE_HOST
CURLOPT_FTP_SOURCE_PORT
CURLOPT_FTP_SOURCE_USERPWD
CURLOPT_FTP_SOURCE_PATH
CURLOPT_SOURCE_PREQUOTE
CURLOPT_SOURCE_POSTQUOTE

As I mentioned in the previous email, all the options have the same
definitions as the original one (without "SOURCE") except
CURLOPT_FTP_SOURCE_HOST, which is the same as a host name in url option.
Obviously, all of them refer for the source host, meaning that the
transfer will be done between the source and target hosts.
Setting CURLOPT_FTP_SOURCE_HOST option means that the transfer will be
performed in third party transfer mode.

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

No. I have no resources to do it.

> * 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?

Actually I thought that it will be easy to somebody, who knows the curl
test suit well, to write test cases using the example I send (which is
actually includes test cases).

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

The following new options can be added:
--ftp-source-host <host> 3rd party transfer source host
--ftp-source-port <port> 3rd party transfer source port
--ftp-source-user <user[:password]> 3rd party transfer source user
--ftp-source-path <filepath> 3rd party transfer source file path
--ftp-source_prequote <cmd> Send command(s) to source server before file
transfer
--ftp-source_postquote <cmd> Send command(s) to source server after file
transfer

> * 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?

The sec_conn connection is closed as any other connection when
disconnecting. Until then the connection can be reused. Mixing third
party transfer and the regular one is possible.
I don't familiar with README.memoryleak approach, but I will try it.

> * 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.

No problem: host.dispname is already used.
Regarding the debug callback: it was already different from the internal
function by the following line:
fwrite(s_infotype[type], 2, 1, data->set.err);

So one solution, as you said, is to change the debug callback, but then
it won't be backward compatible.
The second solution is to prepare a temporary string that will contain
final output for both of them. And therefore it will be compatible in
future when we will need to add some more information.
I think that the second one is better.

Let me know what you think. Meanwhile I will continue to improve the
current curl-style code. I hope the changes will be implemented in
7.12.0 release.

The information contained in this message is proprietary of Amdocs,
protected from disclosure, and may be privileged.
The information is intended to be conveyed only to the designated recipient(s)
of the message. If the reader of this message is not the intended recipient,
you are hereby notified that any dissemination, use, distribution or copying of
this communication is strictly prohibited and may be unlawful.
If you have received this communication in error, please notify us immediately
by replying to the message and deleting it from your computer.
Thank you.
Received on 2004-05-27