Make new FTP LIST parsing code more robust.

Make new FTP LIST parsing code more robust.

Some highlights:
 - more tests (including non-ASCII characters in different encodings)
 - handling more variants of "ls" listing
 - handling spaces in file names in "ls" listings

TEST=Covered by net_unittests.
BUG=25520
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=33688

[Paweł Hajdan Jr., 2009-11-30 10:33:00]

This patch is a result of extensive testing in the field.

I have even more changes in the queue, but first after this patch I'm going to
do a little cleanup in the parsing code. I think that splitting the file for
individual parsers will be a good idea (I wasn't so convinced the first time you
suggested it, but after editing this code some more I am), and I will also
extract the commonly used utility methods from it (to ftp_util probably).

[eroman, 2009-11-30 19:21:48]

http://codereview.chromium.org/449011/diff/1/15
File net/ftp/ftp_directory_listing_parsers.cc (right):

http://codereview.chromium.org/449011/diff/1/15#newcode18
net/ftp/ftp_directory_listing_parsers.cc:18: text[2] == '-' || (text[2] == 't'
&& last)));
what is 't' mean? i'm not sure that i understand this...

http://codereview.chromium.org/449011/diff/1/15#newcode48
net/ftp/ftp_directory_listing_parsers.cc:48: if (text[i - 1] != ' ' && text[i]
== ' ') {
isn't this a memory error at i=0?

(My expectation is for text[i-1] to be a bad access)

http://codereview.chromium.org/449011/diff/1/15#newcode70
net/ftp/ftp_directory_listing_parsers.cc:70: // Special cases for listings in
German.
I think it would be easier to add the special cases into the months[] array.

i.e., turn it into an array of (string, index) pairs.

http://codereview.chromium.org/449011/diff/1/15#newcode74
net/ftp/ftp_directory_listing_parsers.cc:74: } else if (text ==
ASCIIToUTF16("Okt")) {
is case sensitivity important here?

http://codereview.chromium.org/449011/diff/1/15#newcode319
net/ftp/ftp_directory_listing_parsers.cc:319: StartsWith(line,
ASCIIToUTF16("gesamt "), false)) {
Will we need to do this for other languages too?

Perhaps the best thing is to just skip these lines, as opposed to trying to
understand them.

[Paweł Hajdan Jr., 2009-11-30 20:22:19]

On 2009/11/30 19:21:48, eroman wrote:
> http://codereview.chromium.org/449011/diff/1/15
> File net/ftp/ftp_directory_listing_parsers.cc (right):
> 
> http://codereview.chromium.org/449011/diff/1/15#newcode18
> net/ftp/ftp_directory_listing_parsers.cc:18: text[2] == '-' || (text[2] == 't'
> && last)));
> what is 't' mean? i'm not sure that i understand this...

Added an explanation.

> http://codereview.chromium.org/449011/diff/1/15#newcode48
> net/ftp/ftp_directory_listing_parsers.cc:48: if (text[i - 1] != ' ' && text[i]
> == ' ') {
> isn't this a memory error at i=0?
> 
> (My expectation is for text[i-1] to be a bad access)

Good catch (there was one more memory error there, also fixed).

> http://codereview.chromium.org/449011/diff/1/15#newcode70
> net/ftp/ftp_directory_listing_parsers.cc:70: // Special cases for listings in
> German.
> I think it would be easier to add the special cases into the months[] array.
> 
> i.e., turn it into an array of (string, index) pairs.

There is a problem, because one of the German months has a non-ASCII character,
so we can't use StringToLowerASCII on it. I considered such array, but provided
we only have a special case for German, the solution in the patch seems to be
simpler. Alternatively, I could match patterns on the received string. Like M*r
and O?t (but is looks a bit strange).

> http://codereview.chromium.org/449011/diff/1/15#newcode74
> net/ftp/ftp_directory_listing_parsers.cc:74: } else if (text ==
> ASCIIToUTF16("Okt")) {
> is case sensitivity important here?

No, it seems to be just simpler. With ls-style listings we get the first letter
capitalized and the rest lowercase, and in VMS we get everything capitalized,
but always English.

> http://codereview.chromium.org/449011/diff/1/15#newcode319
> net/ftp/ftp_directory_listing_parsers.cc:319: StartsWith(line,
> ASCIIToUTF16("gesamt "), false)) {
> Will we need to do this for other languages too?

I hope not. I gathered quite a lot of data from real servers and only some (not
all) German ones do that. I think it's bad practice to do such a thing on an FTP
server. But if a localized month name is in a real listing line (file or
directory), then the Mozilla parser would silently drop it. The new parser would
before reject the entire listing.

What I could test here for is a word followed by an integer. A bit more code,
but should work for all languages in which "total" translates to one word. What
do you think?

> Perhaps the best thing is to just skip these lines, as opposed to trying to
> understand them.

I don't really like adding more complexity here, but it's not so easy. That
would introduce ambiguity. Please note that for one-line listings we have to be
really careful so that only one parser matches it. I could try to account for it
on the higher level (like "several parsers accept this input, but they
understand it the same way, so that's OK"), but it seems fragile to me.

[wtc, 2009-11-30 20:42:17]

Hi Pawel,

I have some review comments below.  I only did a cursive
review of the non-test files.  Please rely on eroman as
the reviewer of this CL.

http://codereview.chromium.org/449011/diff/18/32
File net/ftp/ftp_directory_listing_parsers.cc (right):

http://codereview.chromium.org/449011/diff/18/32#newcode11
net/ftp/ftp_directory_listing_parsers.cc:11: bool LooksLikeUnixPermission(const
string16& text, bool last) {
Please document what 'last' means.  Perhaps you can name
the bool parameter "other" (vs. "user" and "group"), or
invent an enum type with three values (user, group, other).

http://codereview.chromium.org/449011/diff/18/32#newcode77
net/ftp/ftp_directory_listing_parsers.cc:77: if (text ==
UTF8ToUTF16("M\xc3\xa4r")) {
Please document what the "M\xc3\xa4r" string means,
and why we need to handle "Okt" (but not any other
month) as a special case.

http://codereview.chromium.org/449011/diff/18/32#newcode80
net/ftp/ftp_directory_listing_parsers.cc:80: } else if (text ==
ASCIIToUTF16("Okt")) {
Nit: no "else" after a return statement.

http://codereview.chromium.org/449011/diff/18/32#newcode325
net/ftp/ftp_directory_listing_parsers.cc:325: StartsWith(line,
ASCIIToUTF16("gesamt "), false)) {
Please document what "gesamt" means.

http://codereview.chromium.org/449011/diff/18/32#newcode335
net/ftp/ftp_directory_listing_parsers.cc:335: } else if (line.empty() &&
!received_nonempty_line_) {
Nit: no "else" after a return statement.

[Paweł Hajdan Jr., 2009-12-01 09:39:11]

On 2009/11/30 20:42:17, wtc wrote:
> I have some review comments below.  I only did a cursive
> review of the non-test files.  Please rely on eroman as
> the reviewer of this CL.
> 
> http://codereview.chromium.org/449011/diff/18/32
> File net/ftp/ftp_directory_listing_parsers.cc (right):
> 
> http://codereview.chromium.org/449011/diff/18/32#newcode11
> net/ftp/ftp_directory_listing_parsers.cc:11: bool
LooksLikeUnixPermission(const
> string16& text, bool last) {
> Please document what 'last' means.  Perhaps you can name
> the bool parameter "other" (vs. "user" and "group"), or
> invent an enum type with three values (user, group, other).

I decided to drop the additional logic. It should be still "strict enough", but
more clear.

> http://codereview.chromium.org/449011/diff/18/32#newcode77
> net/ftp/ftp_directory_listing_parsers.cc:77: if (text ==
> UTF8ToUTF16("M\xc3\xa4r")) {
> Please document what the "M\xc3\xa4r" string means,
> and why we need to handle "Okt" (but not any other
> month) as a special case.

Actually, I forgot to add more special cases. Added an explanation too.

> http://codereview.chromium.org/449011/diff/18/32#newcode80
> net/ftp/ftp_directory_listing_parsers.cc:80: } else if (text ==
> ASCIIToUTF16("Okt")) {
> Nit: no "else" after a return statement.

Done.

> http://codereview.chromium.org/449011/diff/18/32#newcode325
> net/ftp/ftp_directory_listing_parsers.cc:325: StartsWith(line,
> ASCIIToUTF16("gesamt "), false)) {
> Please document what "gesamt" means.

Done.

> http://codereview.chromium.org/449011/diff/18/32#newcode335
> net/ftp/ftp_directory_listing_parsers.cc:335: } else if (line.empty() &&
> !received_nonempty_line_) {
> Nit: no "else" after a return statement.

Done.

[Paweł Hajdan Jr., 2009-12-01 20:53:14]

eroman: ping

[wtc, 2009-12-01 23:29:55]

http://codereview.chromium.org/449011/diff/3002/5014
File net/ftp/ftp_directory_listing_parsers.cc (right):

http://codereview.chromium.org/449011/diff/3002/5014#newcode15
net/ftp/ftp_directory_listing_parsers.cc:15: // Meaning of the flags reference:
Nit: remove "reference"?

http://codereview.chromium.org/449011/diff/3002/5014#newcode24
net/ftp/ftp_directory_listing_parsers.cc:24: text[2] == '-' || text[2] == 't' ||
text[2] == 'T'));
Nit: it would be more consistent with the text[0] and
text[1] tests above to list text[2] == '-' last.

http://codereview.chromium.org/449011/diff/3002/5014#newcode78
net/ftp/ftp_directory_listing_parsers.cc:78: if (text ==
UTF8ToUTF16("M\xc3\xa4r")) {
For these German month abbreviations, should we also do
case-insensitive comparison as we do at line 70 above?

http://codereview.chromium.org/449011/diff/3002/5014#newcode336
net/ftp/ftp_directory_listing_parsers.cc:336: if (StartsWith(line,
ASCIIToUTF16("total "), true) ||
Why do we use 'true' with "total " but 'false' with "gesamt "?

[Paweł Hajdan Jr., 2009-12-02 18:11:41]

On 2009/12/01 23:29:55, wtc wrote:
> http://codereview.chromium.org/449011/diff/3002/5014
> File net/ftp/ftp_directory_listing_parsers.cc (right):
> 
> http://codereview.chromium.org/449011/diff/3002/5014#newcode15
> net/ftp/ftp_directory_listing_parsers.cc:15: // Meaning of the flags
reference:
> Nit: remove "reference"?

Done.

> http://codereview.chromium.org/449011/diff/3002/5014#newcode24
> net/ftp/ftp_directory_listing_parsers.cc:24: text[2] == '-' || text[2] == 't'
||
> text[2] == 'T'));
> Nit: it would be more consistent with the text[0] and
> text[1] tests above to list text[2] == '-' last.

Done.

> http://codereview.chromium.org/449011/diff/3002/5014#newcode78
> net/ftp/ftp_directory_listing_parsers.cc:78: if (text ==
> UTF8ToUTF16("M\xc3\xa4r")) {
> For these German month abbreviations, should we also do
> case-insensitive comparison as we do at line 70 above?

1. It's not easy (they're non-ASCII).
2. It's not needed (they will always appear exactly like in the code; no case
differences). Please see my previous comments about that.

> http://codereview.chromium.org/449011/diff/3002/5014#newcode336
> net/ftp/ftp_directory_listing_parsers.cc:336: if (StartsWith(line,
> ASCIIToUTF16("total "), true) ||
> Why do we use 'true' with "total " but 'false' with "gesamt "?

VMS's line also starts with Total (note the upper case), so we must do a
case-sensitive compare. For gesamt, I used case-insensitive just to catch
improbable case when it's misspelled that way (I believe in German it should
always start with upper case). Adding a comment for that would probably add even
more confusion. If you don't like it, I can make both a case-sensitive compare.

[wtc, 2009-12-02 19:43:59]

Note: eroman is taking today off.  I'll try to review this
CL today.

http://codereview.chromium.org/449011/diff/3002/5014
File net/ftp/ftp_directory_listing_parsers.cc (right):

http://codereview.chromium.org/449011/diff/3002/5014#newcode336
net/ftp/ftp_directory_listing_parsers.cc:336: if (StartsWith(line,
ASCIIToUTF16("total "), true) ||
Thanks for explaining this.  This is not obvious at all.
I encourage you to add a comment about this, even though
you're worried that the comment could cause more confusion.
All of my stupid questions are evidence that it's important
to record your knowledge about subtle differences between
directory listings of various FTP servers.

[Paweł Hajdan Jr., 2009-12-02 20:02:21]

> Thanks for explaining this.  This is not obvious at all.
> I encourage you to add a comment about this, even though
> you're worried that the comment could cause more confusion.
> All of my stupid questions are evidence that it's important
> to record your knowledge about subtle differences between
> directory listings of various FTP servers.

Changed the code a bit and added more comments. Thanks for pointing out such
cases - one of the goals of this new parsing code is to be more clear and
understandable.

As written in my previous reply, the code is now doing a case-sensitive compare
in both cases (total and Gesamt). This is less surprising, and the accuracy
should still be fine.

[wtc, 2009-12-03 00:23:08]

LGTM.

http://codereview.chromium.org/449011/diff/7019/8014
File net/ftp/ftp_directory_listing_parsers.cc (right):

http://codereview.chromium.org/449011/diff/7019/8014#newcode54
net/ftp/ftp_directory_listing_parsers.cc:54: if (text[i - 1] != ' ' && text[i]
== ' ') {
I'm wondering if we should use isspace() instead of comparing
with ' '.  For example, do we want to allow the TAB character
('\t') as a column separator?

http://codereview.chromium.org/449011/diff/7019/8014#newcode350
net/ftp/ftp_directory_listing_parsers.cc:350: if (received_nonempty_line_ ||
received_total_line_)
You can get rid of received_total_line_.

It is enough to just check received_nonempty_line_
here because if received_total_line_ is true,
received_nonempty_line_ must be true (see line
353 below).

http://codereview.chromium.org/449011/diff/7019/8014#newcode367
net/ftp/ftp_directory_listing_parsers.cc:367: // We may receive file names
containing spaces, which can make number of
Nit: add "the" before "number".

http://codereview.chromium.org/449011/diff/7019/8014#newcode398
net/ftp/ftp_directory_listing_parsers.cc:398: entry.name =
GetStringPartAfterColumns(line, 8);
Why can't you construct this string part using the 'columns'
vector?

[Paweł Hajdan Jr., 2009-12-03 11:37:22]

On 2009/12/03 00:23:08, wtc wrote:
> LGTM.

I'll wait for eroman's review as well, see below:

On 2009/11/30 20:42:17, wtc wrote:
> I only did a cursive
> review of the non-test files.  Please rely on eroman as
> the reviewer of this CL.

> http://codereview.chromium.org/449011/diff/7019/8014
> File net/ftp/ftp_directory_listing_parsers.cc (right):
> 
> http://codereview.chromium.org/449011/diff/7019/8014#newcode54
> net/ftp/ftp_directory_listing_parsers.cc:54: if (text[i - 1] != ' ' && text[i]
> == ' ') {
> I'm wondering if we should use isspace() instead of comparing
> with ' '.  For example, do we want to allow the TAB character
> ('\t') as a column separator?

Sounds good. I think I've not seen such a case, but I'd want to handle that if
it happens.

> http://codereview.chromium.org/449011/diff/7019/8014#newcode350
> net/ftp/ftp_directory_listing_parsers.cc:350: if (received_nonempty_line_ ||
> received_total_line_)
> You can get rid of received_total_line_.
> 
> It is enough to just check received_nonempty_line_
> here because if received_total_line_ is true,
> received_nonempty_line_ must be true (see line
> 353 below).

Indeed. A bit simpler now.

> http://codereview.chromium.org/449011/diff/7019/8014#newcode367
> net/ftp/ftp_directory_listing_parsers.cc:367: // We may receive file names
> containing spaces, which can make number of
> Nit: add "the" before "number".

Done.

> http://codereview.chromium.org/449011/diff/7019/8014#newcode398
> net/ftp/ftp_directory_listing_parsers.cc:398: entry.name =
> GetStringPartAfterColumns(line, 8);
> Why can't you construct this string part using the 'columns'
> vector?

So, I'm using this function to extract the file name. It's needed when the file
name contains spaces. If you have no two or more consecutive spaces in the file
name, reconstructing this info from the columns vector is trivial. However, in
general we can't make that assumption (that there is always exactly one space
between each column).

[wtc, 2009-12-03 16:07:15]

Pawel: I reviewed your entire CL yesterday, so you can
commit it without waiting for eroman's review.  You can
always address his review comments in a follow-up CL.

[Paweł Hajdan Jr., 2009-12-03 16:14:26]

Landing then.

eroman, if you have any comments, I'm going to address them in a follow-up CL,
after I split the parsers file.