Implement VMS FTP directory listing parser.

Implement VMS FTP directory listing parser.

This may not yet correctly handle all possible variants of VMS,
because there multiple TCP/IP network stacks available for that
system.

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

[eroman, 2009-11-02 23:55:21]

This parsing code definitely looks more robust from a security standpoint (looks
like you are very careful about checking the lengths of arrays), and I can
actually understand it!

http://codereview.chromium.org/348036/diff/1/2
File net/data/ftp/dir-listing-vms-1 (right):

http://codereview.chromium.org/348036/diff/1/2#newcode6
Line 6: .WELCOME;1                 2  13-FEB-2002 23:32:40.47
That is interesting-- based on the code I would have expected this to parse ""
as the name, and "WELCOME" as the extension.

You may also want to add a unit-test for something like:

"FILE."  (I believe that is a valid input).

http://codereview.chromium.org/348036/diff/1/2#newcode9
Line 9: MADGOAT.DIR;1              2   9-MAY-2001 22:23:44.85
heh.

http://codereview.chromium.org/348036/diff/1/3
File net/data/ftp/dir-listing-vms-1.expected (right):

http://codereview.chromium.org/348036/diff/1/3#newcode2
Line 2: .welcome
Is it expected that we don't preserve case?

http://codereview.chromium.org/348036/diff/1/4
File net/data/ftp/dir-listing-vms-2 (right):

http://codereview.chromium.org/348036/diff/1/4#newcode35
Line 35: Total of 30 files, 53/464 blocks
Is there a test where 0 files are listed?

http://codereview.chromium.org/348036/diff/1/6
File net/ftp/ftp_directory_listing_buffer.cc (right):

http://codereview.chromium.org/348036/diff/1/6#newcode96
Line 96: if (i >= 1 && buffer_[i - 1] == '\r')
May want to document how we tokenize lines here:
\r*\n

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

http://codereview.chromium.org/348036/diff/1/8#newcode112
Line 112: *parsed_filename = StringToLowerASCII(filename_parts[0]);
StringToLowerASCII won't  complain if the input is not ASCII right?

http://codereview.chromium.org/348036/diff/1/8#newcode152
Line 152: std::string pattern("RWED");
I would be more comfortable if there were additionally a short-circuit for:

if (input.length() > 4)
  return false;

(That way we fail faster, and can justify calling substr() in a loop since we
are guaranteed to have few iterations.)

http://codereview.chromium.org/348036/diff/1/8#newcode293
Line 293: FtpVmsDirectoryListingParser::FtpVmsDirectoryListingParser()
Have you considered defining each implementation in its own file?

  ftp_vms_directory_listing_parser.cc ---> VMS DIRECTORY stuff
  ftp_ls_directory_listing_parser.cc  --> POSIX LS stuff

http://codereview.chromium.org/348036/diff/1/8#newcode299
Line 299: switch (state_) {
The tricky part when reading this switch, is some cases return, whereas others
break.

As this method is already quite large, I recommend breaking it up in some way.

One way could be to move the parsing code out into a method, say
"ParseEntry(line)", and call "return ParseEntry(line);" from the switch (so all
codepaths return).

http://codereview.chromium.org/348036/diff/1/9
File net/ftp/ftp_directory_listing_parsers.h (right):

http://codereview.chromium.org/348036/diff/1/9#newcode62
Line 62: class FtpVmsDirectoryListingParser : public FtpDirectoryListingParser {
Comment-Nit: I strongly recommend adding a comment block for both
FtpLsDirectoryListingParser and FtpLsDirectoryListingParser explaining how they
differ.

i.e. FtpLsDirectoryListingParser --> Corresponds with the POSIX "ls -l" command.

FtpVmsDirectoryListingParser --> Corresponds with the VMS "DIRECTORY" command.
And possibly include a link to some documentation.

http://codereview.chromium.org/348036/diff/1/9#newcode77
Line 77: STATE_HEADER,
My bias is to think "STATE_HEADER" means we are processing the header, not that
have already processed the header. I would suggest wording this as
STATE_HAVE_HEADER, or shift them backwards by 1.

http://codereview.chromium.org/348036/diff/1/9#newcode83
Line 83: // Indicates that we expect the footer, like this:
This state description is inconsistent with the others (it describes a future
expectation, whereas the others describe data that has been received).

http://codereview.chromium.org/348036/diff/1/9#newcode93
Line 93: string16 last_filename_;
I don't get why we only need to store last_filename_/last_is_directory_, and not
the other data that was on that line (modified timestamp).

It seems like we could instead save a |FtpDirectoryListingEntry
in_progress_entry| and have it contain the parsed information we got so far.

I must be missing something...

[Paweł Hajdan Jr., 2009-11-04 16:00:13]

Thanks for review. While fixing the initial issues, I discovered a new variant
of VMS so I fixed a few more things. There are two more tests now.

Please take a look.

http://codereview.chromium.org/348036/diff/1/2
File net/data/ftp/dir-listing-vms-1 (right):

http://codereview.chromium.org/348036/diff/1/2#newcode6
Line 6: .WELCOME;1                 2  13-FEB-2002 23:32:40.47
On 2009/11/02 23:55:21, eroman wrote:
> That is interesting-- based on the code I would have expected this to parse ""
> as the name, and "WELCOME" as the extension.

Right, but the final filename is name + extension (we only remove extension for
directories).

> You may also want to add a unit-test for something like:
> 
> "FILE."  (I believe that is a valid input).

Indeed, added. I verified that myself on a VMS system.

http://codereview.chromium.org/348036/diff/1/3
File net/data/ftp/dir-listing-vms-1.expected (right):

http://codereview.chromium.org/348036/diff/1/3#newcode2
Line 2: .welcome
On 2009/11/02 23:55:21, eroman wrote:
> Is it expected that we don't preserve case?

Yes. VMS is case-insensitive, I added a comment about that in code. Generally I
don't like all-uppercase.

http://codereview.chromium.org/348036/diff/1/4
File net/data/ftp/dir-listing-vms-2 (right):

http://codereview.chromium.org/348036/diff/1/4#newcode35
Line 35: Total of 30 files, 53/464 blocks
On 2009/11/02 23:55:21, eroman wrote:
> Is there a test where 0 files are listed?

Good idea, added (and fixed a bug or two).

http://codereview.chromium.org/348036/diff/1/6
File net/ftp/ftp_directory_listing_buffer.cc (right):

http://codereview.chromium.org/348036/diff/1/6#newcode96
Line 96: if (i >= 1 && buffer_[i - 1] == '\r')
On 2009/11/02 23:55:21, eroman wrote:
> May want to document how we tokenize lines here:
> \r*\n

Done. I think that in the end I want it to be more strict.

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

http://codereview.chromium.org/348036/diff/1/8#newcode112
Line 112: *parsed_filename = StringToLowerASCII(filename_parts[0]);
On 2009/11/02 23:55:21, eroman wrote:
> StringToLowerASCII won't  complain if the input is not ASCII right?

Yes, I verified that.

http://codereview.chromium.org/348036/diff/1/8#newcode152
Line 152: std::string pattern("RWED");
On 2009/11/02 23:55:21, eroman wrote:
> I would be more comfortable if there were additionally a short-circuit for:
> 
> if (input.length() > 4)
>   return false;
> 
> (That way we fail faster, and can justify calling substr() in a loop since we
> are guaranteed to have few iterations.)

Done.

http://codereview.chromium.org/348036/diff/1/8#newcode293
Line 293: FtpVmsDirectoryListingParser::FtpVmsDirectoryListingParser()
On 2009/11/02 23:55:21, eroman wrote:
> Have you considered defining each implementation in its own file?
> 
>   ftp_vms_directory_listing_parser.cc ---> VMS DIRECTORY stuff
>   ftp_ls_directory_listing_parser.cc  --> POSIX LS stuff

Yes, I was thinking about physical layout. Well, I wouldn't like to have each
parser in its own file. I'm going to add support for at least two more listing
styles. That'd be 8 files total. But I think that after writing the third
parser, or even earlier, I'll extract some code from here to ftp_util.cc or
something. What do you think?

http://codereview.chromium.org/348036/diff/1/8#newcode299
Line 299: switch (state_) {
On 2009/11/02 23:55:21, eroman wrote:
> The tricky part when reading this switch, is some cases return, whereas others
> break.
> 
> As this method is already quite large, I recommend breaking it up in some way.
> 
> One way could be to move the parsing code out into a method, say
> "ParseEntry(line)", and call "return ParseEntry(line);" from the switch (so
all
> codepaths return).

Fixed.

http://codereview.chromium.org/348036/diff/1/9
File net/ftp/ftp_directory_listing_parsers.h (right):

http://codereview.chromium.org/348036/diff/1/9#newcode62
Line 62: class FtpVmsDirectoryListingParser : public FtpDirectoryListingParser {
Comments added. But the formats are not really documented. I generally
reverse-engineered them, and there are many variants, especially for VMS.

http://codereview.chromium.org/348036/diff/1/9#newcode77
Line 77: STATE_HEADER,
On 2009/11/02 23:55:21, eroman wrote:
> My bias is to think "STATE_HEADER" means we are processing the header, not
that
> have already processed the header. I would suggest wording this as
> STATE_HAVE_HEADER, or shift them backwards by 1.

Good point. Should be a little better now.

http://codereview.chromium.org/348036/diff/1/9#newcode83
Line 83: // Indicates that we expect the footer, like this:
On 2009/11/02 23:55:21, eroman wrote:
> This state description is inconsistent with the others (it describes a future
> expectation, whereas the others describe data that has been received).

Indeed, I knew that when writing the code... after some though, it should be a
bit better.

http://codereview.chromium.org/348036/diff/1/9#newcode93
Line 93: string16 last_filename_;
On 2009/11/02 23:55:21, eroman wrote:
> I don't get why we only need to store last_filename_/last_is_directory_, and
not
> the other data that was on that line (modified timestamp).
> 
> It seems like we could instead save a |FtpDirectoryListingEntry
> in_progress_entry| and have it contain the parsed information we got so far.
> 
> I must be missing something...

Indeed, I didn't document that the filename is all we will get in the first
line, always.

[eroman, 2009-11-04 19:57:15]

lgtm

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

http://codereview.chromium.org/348036/diff/1/8#newcode293
Line 293: FtpVmsDirectoryListingParser::FtpVmsDirectoryListingParser()
On 2009/11/04 16:00:13, Paweł Hajdan Jr. wrote:
> On 2009/11/02 23:55:21, eroman wrote:
> > Have you considered defining each implementation in its own file?
> > 
> >   ftp_vms_directory_listing_parser.cc ---> VMS DIRECTORY stuff
> >   ftp_ls_directory_listing_parser.cc  --> POSIX LS stuff
> 
> Yes, I was thinking about physical layout. Well, I wouldn't like to have each
> parser in its own file. I'm going to add support for at least two more listing
> styles. That'd be 8 files total. But I think that after writing the third
> parser, or even earlier, I'll extract some code from here to ftp_util.cc or
> something. What do you think?

Up to you.

My personal preference is one class per file, as it keeps each file compact, and
the local symbol names in each file can be short and simple (as their won't be
conflicts). I don't find number of files on disk to be a problem.

http://codereview.chromium.org/348036/diff/4001/3012
File net/ftp/ftp_directory_listing_buffer_unittest.cc (right):

http://codereview.chromium.org/348036/diff/4001/3012#newcode23
Line 23: //"dir-listing-vms-3",
Why are these lines commented out?

[Paweł Hajdan Jr., 2009-11-04 20:56:29]

On 2009/11/04 19:57:15, eroman wrote:
> My personal preference is one class per file, as it keeps each file compact,
and
> the local symbol names in each file can be short and simple (as their won't be
> conflicts). I don't find number of files on disk to be a problem.

Yes, it's a good point. I'll consider it after adding more parsers, I'll have
better overview then.

> http://codereview.chromium.org/348036/diff/4001/3012
> File net/ftp/ftp_directory_listing_buffer_unittest.cc (right):
> 
> http://codereview.chromium.org/348036/diff/4001/3012#newcode23
> Line 23: //"dir-listing-vms-3",
> Why are these lines commented out?

Ooops. Good catch. It was easier for me to debug when running just the failing
test case. Uncommented the lines and committed.