[NaCl SDK] Rewrite nacl_io::Path to avoid STL.

[NaCl SDK] Rewrite nacl_io::Path to avoid STL.

Path used to be a std::vector<std::string>. Now it is just a C array of size
PATH_MAX.

The code has a lot of nasty edge cases because it is trying to match the
previous behavior. Really, all this code should be axed -- it is incorrect to
handle paths the way nacl_io does currently. AFAICT, the only place that should
have to handle paths is the path lookup routine.

Fixing that is a much larger change.

BUG=none
R=sbc@chromium.org

Committed: https://crrev.com/fea846358e7c2efe04bdeb4b01ecccee6efe7759
Cr-Commit-Position: refs/heads/master@{#321386}

[Sam Clegg, 2015-03-18 18:11:12]

I love this kind of change.

Sad to see that tests change so much though..  the diff makes it look like you
completely re-wrote them, is this actually the case?

https://codereview.chromium.org/1016873004/diff/1/native_client_sdk/src/libra...
File native_client_sdk/src/libraries/nacl_io/path.cc (right):

https://codereview.chromium.org/1016873004/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/path.cc:108: len_ = path.length();
What happens if path.length() is greater than PATH_MAX?

Should we assert that it is < PATH_MAX here?

https://codereview.chromium.org/1016873004/diff/1/native_client_sdk/src/libra...
File native_client_sdk/src/libraries/nacl_io/path.h (right):

https://codereview.chromium.org/1016873004/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/path.h:59: char path_[PATH_MAX + 1];
No need for the +1 I think:
"Maximum number of bytes in a pathname, including the terminating null
character"

http://pubs.opengroup.org/onlinepubs/009695399/basedefs/limits.h.html

[binji, 2015-03-19 00:53:18]

Yes, I did rewrite the tests to make it more clear what was being tested. The
tests that were there didn't catch any bugs that I had in the implementation, so
I had to add new tests anyway. The way the old tests were written made this
clunky -- I'd have to extend those poorly organized tests, or end up testing the
same thing twice. I opted to just clean up the tests. Though I see that makes
your job a bit harder... perhaps I should land the test cleanup as a separate
CL?

https://codereview.chromium.org/1016873004/diff/1/native_client_sdk/src/libra...
File native_client_sdk/src/libraries/nacl_io/path.cc (right):

https://codereview.chromium.org/1016873004/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/path.cc:108: len_ = path.length();
On 2015/03/18 18:11:12, Sam Clegg wrote:
> What happens if path.length() is greater than PATH_MAX?
> 
> Should we assert that it is < PATH_MAX here?

I think we're supposed to return ENAMETOOLONG. But that is hard to shoehorn into
this CL. Once we start handling paths correctly, all of this code can be
simplified.

https://codereview.chromium.org/1016873004/diff/1/native_client_sdk/src/libra...
File native_client_sdk/src/libraries/nacl_io/path.h (right):

https://codereview.chromium.org/1016873004/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/path.h:59: char path_[PATH_MAX + 1];
On 2015/03/18 18:11:12, Sam Clegg wrote:
> No need for the +1 I think:
> "Maximum number of bytes in a pathname, including the terminating null
> character"
> 
> http://pubs.opengroup.org/onlinepubs/009695399/basedefs/limits.h.html

Done.

[Sam Clegg, 2015-03-19 01:03:05]

lgtm

https://codereview.chromium.org/1016873004/diff/1/native_client_sdk/src/libra...
File native_client_sdk/src/libraries/nacl_io/path.cc (right):

https://codereview.chromium.org/1016873004/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/path.cc:108: len_ = path.length();
On 2015/03/19 00:53:18, binji wrote:
> On 2015/03/18 18:11:12, Sam Clegg wrote:
> > What happens if path.length() is greater than PATH_MAX?
> > 
> > Should we assert that it is < PATH_MAX here?
> 
> I think we're supposed to return ENAMETOOLONG. But that is hard to shoehorn
into
> this CL. Once we start handling paths correctly, all of this code can be
> simplified.

Well at least you should never set len_ to anything larger than PATH_MAX-1,
right?  (you limit it in Append, but not here).

https://codereview.chromium.org/1016873004/diff/40001/native_client_sdk/src/t...
File native_client_sdk/src/tests/nacl_io_test/path_test.cc (right):

https://codereview.chromium.org/1016873004/diff/40001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/path_test.cc:58: EXPECT_EQ("foo",
p.Parent().Join());
Shouldn't Parent be "" for a single element?

https://codereview.chromium.org/1016873004/diff/40001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/path_test.cc:173: TEST(PathTest,
Parent) {
A lot of these are duplicated the above tests.  Is that intentional?

[binji, 2015-03-19 16:42:22]

https://codereview.chromium.org/1016873004/diff/1/native_client_sdk/src/libra...
File native_client_sdk/src/libraries/nacl_io/path.cc (right):

https://codereview.chromium.org/1016873004/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/path.cc:108: len_ = path.length();
On 2015/03/19 01:03:05, Sam Clegg wrote:
> On 2015/03/19 00:53:18, binji wrote:
> > On 2015/03/18 18:11:12, Sam Clegg wrote:
> > > What happens if path.length() is greater than PATH_MAX?
> > > 
> > > Should we assert that it is < PATH_MAX here?
> > 
> > I think we're supposed to return ENAMETOOLONG. But that is hard to shoehorn
> into
> > this CL. Once we start handling paths correctly, all of this code can be
> > simplified.
> 
> Well at least you should never set len_ to anything larger than PATH_MAX-1,
> right?  (you limit it in Append, but not here).

This is fixed in patchset 3.

https://codereview.chromium.org/1016873004/diff/40001/native_client_sdk/src/t...
File native_client_sdk/src/tests/nacl_io_test/path_test.cc (right):

https://codereview.chromium.org/1016873004/diff/40001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/path_test.cc:58: EXPECT_EQ("foo",
p.Parent().Join());
On 2015/03/19 01:03:05, Sam Clegg wrote:
> Shouldn't Parent be "" for a single element?

Actually, I think it should be "." -- but this is the previous behavior and I
didn't want to change it.

https://codereview.chromium.org/1016873004/diff/40001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/path_test.cc:173: TEST(PathTest,
Parent) {
On 2015/03/19 01:03:05, Sam Clegg wrote:
> A lot of these are duplicated the above tests.  Is that intentional?

Good point, I didn't mean to test these twice.

[binji, 2015-03-19 16:42:33]

The CQ bit was checked by binji@chromium.org

[binji, 2015-03-19 16:42:33]

The patchset sent to the CQ was uploaded after l-g-t-m from sbc@chromium.org
Link to the patchset: https://codereview.chromium.org/1016873004/#ps60001
(title: "remove redundant parent test")

[commit-bot: I haz the power, 2015-03-19 16:42:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1016873004/60001

[commit-bot: I haz the power, 2015-03-19 17:25:52]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-19 17:26:44]

Patchset 4 (id:??) landed as
https://crrev.com/fea846358e7c2efe04bdeb4b01ecccee6efe7759
Cr-Commit-Position: refs/heads/master@{#321386}