FTP: change order of commands to improve compatibility

FTP: change order of commands to improve compatibility

It turns out some FTP servers react strangely to a RETR for an
unexistent file. They send a success reponse followed by a failure
response.

The solution is to move the file/directory sniffing logic from
RETR earlier, to SIZE.

BUG=44582
TEST=net_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=51866

[eroman, 2010-07-02 18:28:06]

http://codereview.chromium.org/2813044/diff/1/2
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/2813044/diff/1/2#newcode839
net/ftp/ftp_network_transaction.cc:839: if (resource_type_ ==
RESOURCE_TYPE_DIRECTORY)
Why does the resource type control whether or not to transition to EPSV write?

Isn't EPSV just for IPv6?

[Paweł Hajdan Jr., 2010-07-05 11:29:23]

Patch updated, please take another look.

http://codereview.chromium.org/2813044/diff/1/2
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/2813044/diff/1/2#newcode839
net/ftp/ftp_network_transaction.cc:839: if (resource_type_ ==
RESOURCE_TYPE_DIRECTORY)
On 2010/07/02 18:28:06, eroman wrote:
> Why does the resource type control whether or not to transition to EPSV write?

After SIZE succeeded we'd call EPSV next. If the resource is a dictionary, we
can omit the SIZE step because we know it will fail. Anyway, this EPSV/SIZE
choice may indeed be counterintuitive, so I've modified the code a bit so that
now we have a choice between RETR and CWD, which is hopefully more readable.

> Isn't EPSV just for IPv6?

Yeah, we added it to support IPv6. If we didn't support EPSV, there would be
STATE_CTRL_WRITE_PASV here.

[Paweł Hajdan Jr., 2010-07-06 11:45:20]

ping

[Paweł Hajdan Jr., 2010-07-07 06:02:57]

+cbentzel, willchan

Could you take a look? eroman and wtc seem to be busy.

Let me know if you'd prefer me to split this change to just the bugfix and a
cleanup change.

[cbentzel, 2010-07-07 11:57:00]

I'm sheriff today but I'll try to take a look in between fighting fires.

On Wed, Jul 7, 2010 at 2:02 AM, <phajdan.jr@chromium.org> wrote:

> +cbentzel, willchan
>
> Could you take a look? eroman and wtc seem to be busy.
>
> Let me know if you'd prefer me to split this change to just the bugfix and
> a
> cleanup change.
>
>
> http://codereview.chromium.org/2813044/show
>

[willchan no longer on Chromium, 2010-07-07 14:10:49]

Pawel: Monday and Tuesday were both holidays (4th of July weekend), so
that's why Eric didn't respond.  I forget when wtc is supposed to be
back, but it might also be why he didn't.  I suspect Eric will get to
this today, otherwise Chris/I can pick it up.

On Tue, Jul 6, 2010 at 11:02 PM,  <phajdan.jr@chromium.org> wrote:
> +cbentzel, willchan
>
> Could you take a look? eroman and wtc seem to be busy.
>
> Let me know if you'd prefer me to split this change to just the bugfix and a
> cleanup change.
>
> http://codereview.chromium.org/2813044/show
>

[cbentzel, 2010-07-07 15:01:06]

Took a quick look - I would like this to be split up as you suggested.

[Paweł Hajdan Jr., 2010-07-07 17:58:48]

Okay, this is about 30% smaller after removing the cleanup changes.

This is already changing the command sequence, so I've decided to keep the MDTM
removal in the patch. It's not technically required for the cleanup, but after
the re-ordering I'd have to somehow find the right place for MDTM, which would
be again removed by the cleanup patch.

[eroman, 2010-07-07 23:26:39]

LGTM.

Will is correct -- I was gone on vacation monday and tuesday, or I would have
responded earlier :)

[wtc, 2010-07-07 23:52:46]

I only reviewed your changes to ftp_network_transaction.{h,cc}.
They Look Good To Me.

I don't understand your changes to ftp_network_transaction_unittest.cc
related to PRE_PASV2 and PRE_EPSV2.  Hopefully eroman reviewed those
changes.

http://codereview.chromium.org/2813044/diff/12001/13001
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/2813044/diff/12001/13001#newcode1006
net/ftp/ftp_network_transaction.cc:1006: return Stop(ERR_FAILED);
Nit: Can we return a better error code than ERR_FAILED here?