FTP: fix compatibility problems with MLSD by removing MLSD support.

FTP: fix compatibility problems with MLSD by removing MLSD support.

Some FTP servers send MLSD listings with no facts, just
the file names. This is a valid RFC-3659 listing, but
is useless for web browsers.

In theory we could check whether a server advertises
MLSD support in FEAT response, or use LIST when MLSD
only returns file names, but the extra complexity
is not worth it.

All servers supporting MLSD must also support LIST,
so this should not regress compatibility.

This change also removes the "HPRC" directory listing parser.
Actually, it was just MLSD listing with no facts.

BUG=65328, 65584
TEST=net_unittests, also see the bugs

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

[Paweł Hajdan Jr., 2010-12-08 10:41:41]

The change is really simple, just removing code.

[wtc, 2010-12-08 20:12:01]

I only reviewed the .h files, so please consider this
a drive-by review.  Please wait for eroman's full review.

http://codereview.chromium.org/5669001/diff/1/net/ftp/ftp_server_type_histogr...
File net/ftp/ftp_server_type_histograms.h (right):

http://codereview.chromium.org/5669001/diff/1/net/ftp/ftp_server_type_histogr...
net/ftp/ftp_server_type_histograms.h:28: // Types 13-14 are RESERVED (were
earlier used for MLSD listings).
This will change the value of NUM_OF_SERVER_TYPES.
Do you want to instead keep the two enums but rename them
to show they're reserved?
  // Types 13-14 are RESERVED (were earlier used for MLSD listings).
  SERVER_MLSD_RESERVED = 13,
  SERVER_HPRC_RESERVED = 14,

[jar (doing other things), 2010-12-08 20:50:12]

[per request for a drive by review on a tiny issue...]

I agree with wtc's suggestion... see below.

http://codereview.chromium.org/5669001/diff/1/net/ftp/ftp_server_type_histogr...
File net/ftp/ftp_server_type_histograms.h (right):

http://codereview.chromium.org/5669001/diff/1/net/ftp/ftp_server_type_histogr...
net/ftp/ftp_server_type_histograms.h:28: // Types 13-14 are RESERVED (were
earlier used for MLSD listings).
Assuming the only use of NUM_OF_SERVER_TYPES is in a histogram....

This code will work fine as is.... and would work fine with the reserved names. 
The time you'll have trouble is IF you add more items to the enums, and they
overlap the previous entries.  It will cause some confusion, and force you to
rename the histogram's buckets depending on the version (which is not supported
:-/) ... or revise the histogram name.

My suggestion: Go with the reservation as shown, and add a comment that they are
deprecated, and not used anymore (but reserved to avoid accidental re-use). 
This will create (for now) a pair of unused buckets, which means about 2 ints of
storage.... and that will be doubled since there is a snapshot.  For the 4 ints
total cost, you are future proofing bothersome coding confusion.

On 2010/12/08 20:12:01, wtc wrote:
> This will change the value of NUM_OF_SERVER_TYPES.
> Do you want to instead keep the two enums but rename them
> to show they're reserved?
>   // Types 13-14 are RESERVED (were earlier used for MLSD listings).
>   SERVER_MLSD_RESERVED = 13,
>   SERVER_HPRC_RESERVED = 14,

[Paweł Hajdan Jr., 2010-12-08 21:12:23]

I'd prefer to leave the enum as-is.

If we add RESERVED entries for 13 and 14, we should also add those entries for
1-8 for consistency. I've tried that change locally, and in my opinion it hugely
decreases the readability of that header.

What do you think?

[wtc, 2010-12-08 22:04:56]

http://codereview.chromium.org/5669001/diff/1/net/ftp/ftp_server_type_histogr...
File net/ftp/ftp_server_type_histograms.h (right):

http://codereview.chromium.org/5669001/diff/1/net/ftp/ftp_server_type_histogr...
net/ftp/ftp_server_type_histograms.h:20: // Types 1-8 are RESERVED (were earlier
used for listings recognized by
I see.  One difference of 1-8 is that they don't change the
value of NUM_OF_SERVER_TYPES.  My concern is really about
the value of NUM_OF_SERVER_TYPES: whether it's OK for
NUM_OF_SERVER_TYPES to decrease.  If it's OK, then your
current change to this file is fine.

[jar (doing other things), 2010-12-08 22:54:30]

As noted, if the issue is ONLY histograms, things will work perfectly with
the NUM_OF_SERVER_TYPES moving around (assuming it is only used as an
argument to HISTOGRAM_ENUMERATION(), and never as a sample values).  So long
as the bounding value supplied to the macro is one larger than ANY sample,
it creates a distinct top-end bucket, that is never confused, and all the
real buckets have a width of exactly 1... which means they won't change when
you add more enums.

The ONLY time you get into some trouble if if you use that reserved last
bucket, which officially has a range of [N, INT_MAX).  If you later add
another bucket, your samples at N will now be put in a new [N, N+) bucket...
which is different.. and won't blend nicely with the other bucket.

Jim



On Wed, Dec 8, 2010 at 2:04 PM, <wtc@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/5669001/diff/1/net/ftp/ftp_server_type_histogr...
> File net/ftp/ftp_server_type_histograms.h (right):
>
>
>
http://codereview.chromium.org/5669001/diff/1/net/ftp/ftp_server_type_histogr...
> net/ftp/ftp_server_type_histograms.h:20: // Types 1-8 are RESERVED (were
> earlier used for listings recognized by
> I see.  One difference of 1-8 is that they don't change the
> value of NUM_OF_SERVER_TYPES.  My concern is really about
> the value of NUM_OF_SERVER_TYPES: whether it's OK for
> NUM_OF_SERVER_TYPES to decrease.  If it's OK, then your
> current change to this file is fine.
>
>
> http://codereview.chromium.org/5669001/
>

[Paweł Hajdan Jr., 2010-12-09 07:29:34]

eroman: ping

NUM_OF_SERVER_TYPES is only used for histograms, and is never used to record
data.

[eroman, 2010-12-10 04:07:49]

LGTM