[NaCl SDK] nacl_io: Replace allocated ino with hash of path.

Replace allocated ino with hash of path.

HTML5FS (PPAPI) does not provide an INO so it is impossible to determine
a files uniqueness.  In addition, node information does not persist in
memory since nodes could be transparently destroyed from JavaScript, thus
creating leaks.  Because of this the current stack based INO allocation
causes sequentially accesed file handles to report the same INO.

HTML5FS (including media galleries) does not traverse links.  This implies
that all paths normalized to the mount are unique.  We use this property
to generate a 64 bit hash.

R=binji@chromium.org
BUG=414938
Committed: https://crrev.com/40b895fd4ef2aef2d1ae1ba2c66c2f9f58dc6310
Cr-Commit-Position: refs/heads/master@{#296840}

[noelallen1, 2014-09-25 02:53:36]

PTAL

[binji, 2014-09-25 15:01:17]

Also can you add a test for this?

https://codereview.chromium.org/605513002/diff/1/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc (right):

https://codereview.chromium.org/605513002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc:34: static inline
ino_t HashChar(ino_t hash, char c) {
I'd add these to the anonymous namespace above rather than making them static

https://codereview.chromium.org/605513002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc:45: while (ptr) {
while (*ptr)

https://codereview.chromium.org/605513002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc:52: // For HTML5,
the INO should be the one used by the system, however PPAPI
It is not clear from the comment that you are overriding this function to
prevent st_ino from being set by the default OnNodeCreated call. Can you clarify
that here?

https://codereview.chromium.org/605513002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc:79:
node->stat_.st_ino = HashPath(path);
This will fix GetStat, but GetDents is also returning an incorrect ino_t value.

[Sam Clegg, 2014-09-25 15:49:31]

sbc@chromium.org changed reviewers:
+ sbc@chromium.org

[Sam Clegg, 2014-09-25 15:49:32]

Can you prefix the CL descriptions with "[NaCl SDK] nacl_io:"?

"current" is misspelled in CL description.

I don't really understand why this problem is unique the
html5 filesystem.  Don't all the filesystems suffer from the
same issue?

Also, is there a specific application that I suffering from
the problem?  I know there are at least a couple of apps
in naclports that test for ino number to see if two files
are the same (currently never true with nacl_io).

[noelallen1, 2014-09-25 18:10:09]

binji - I'm not sure what a meaningful test is.  Since there can be a collision,
even the trivial case of stat X.st_ino != stat Y.st_ino is not guaranteed, just
improved.  We could just be adding a test that randomly breaks later.

sbc - Using this as a default might make sense but is a bigger change.  We've
changed the behavior of NaCl IO for a wider range.  I'm not convinced we should
change MemFS which currently should be doing the correct thing.  I'm also not
sure this makes sense for URLLoader since the remote serve controls the file. 
What would an INO mean in this case?

To answer your AFAIK the problem has only been reported on HTML5FS.   

I think it makes more sense to make a change that is more easy to reason is
positive in all reasonable cases, and then iterate once we have more time.

https://codereview.chromium.org/605513002/diff/1/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc (right):

https://codereview.chromium.org/605513002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc:34: static inline
ino_t HashChar(ino_t hash, char c) {
On 2014/09/25 15:01:17, binji wrote:
> I'd add these to the anonymous namespace above rather than making them static

Done.

https://codereview.chromium.org/605513002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc:45: while (ptr) {
On 2014/09/25 15:01:17, binji wrote:
> while (*ptr)

Interesting, why didn't our current tests catch this?

https://codereview.chromium.org/605513002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc:52: // For HTML5,
the INO should be the one used by the system, however PPAPI
On 2014/09/25 15:01:17, binji wrote:
> It is not clear from the comment that you are overriding this function to
> prevent st_ino from being set by the default OnNodeCreated call. Can you
clarify
> that here?

Done.

https://codereview.chromium.org/605513002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc:79:
node->stat_.st_ino = HashPath(path);
On 2014/09/25 15:01:17, binji wrote:
> This will fix GetStat, but GetDents is also returning an incorrect ino_t
value.

Done.

[Sam Clegg, 2014-09-25 19:12:09]

I think it should be possible to write a test that compares the inode number of
"/foo" and "/bar", if the hash function is weak and they collide we want to know
in testing, right?

https://codereview.chromium.org/605513002/diff/1/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc (right):

https://codereview.chromium.org/605513002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc:37: return out ^ c;
Is this is well known hash function?  Perhaps at a reference to where it comes
from?

[Sam Clegg, 2014-09-25 19:14:57]

I chatted with Noel about this and we both agree that a more extensive fix that
applies to all filesystem types is needed here, but the change in this form
acceptable as a partial/interim fix.

Once we get the full fix there are several naclports patches (that remove inode
comparisons) that we can then remove!

On 2014/09/25 19:12:09, Sam Clegg wrote:
> I think it should be possible to write a test that compares the inode number
of
> "/foo" and "/bar", if the hash function is weak and they collide we want to
know
> in testing, right?
> 
>
https://codereview.chromium.org/605513002/diff/1/native_client_sdk/src/librar...
> File native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc (right):
> 
>
https://codereview.chromium.org/605513002/diff/1/native_client_sdk/src/librar...
> native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc:37: return out ^
c;
> Is this is well known hash function?  Perhaps at a reference to where it comes
> from?

[noelallen1, 2014-09-25 21:01:38]

Since we are moving from what is effectively one random number generator to
effectively another random number, testing INO values doesn't make sense. 
Adding a test here could be fragile or flaky.

However testing that GetDents and GetStat agree on the INO value is, so I've
added a test for that.

https://codereview.chromium.org/605513002/diff/1/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc (right):

https://codereview.chromium.org/605513002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc:37: return out ^ c;
On 2014/09/25 19:12:09, Sam Clegg wrote:
> Is this is well known hash function?  Perhaps at a reference to where it comes
> from?

I actually went ahead and changed this to a well known function which has
similar properties.

[Sam Clegg, 2014-09-25 21:30:36]

On 2014/09/25 21:01:38, noelallen1 wrote:
> Since we are moving from what is effectively one random number generator to
> effectively another random number, testing INO values doesn't make sense. 
> Adding a test here could be fragile or flaky.

Yes, I wasn't suggesting testing the absolute value of ino, only that two files
don't have the same value. It looks like that is what you done.

lgtm


> 
> However testing that GetDents and GetStat agree on the INO value is, so I've
> added a test for that.
> 
>
https://codereview.chromium.org/605513002/diff/1/native_client_sdk/src/librar...
> File native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc (right):
> 
>
https://codereview.chromium.org/605513002/diff/1/native_client_sdk/src/librar...
> native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc:37: return out ^
c;
> On 2014/09/25 19:12:09, Sam Clegg wrote:
> > Is this is well known hash function?  Perhaps at a reference to where it
comes
> > from?
> 
> I actually went ahead and changed this to a well known function which has
> similar properties.

[binji, 2014-09-25 21:52:04]

lgtm

https://codereview.chromium.org/605513002/diff/40001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc (right):

https://codereview.chromium.org/605513002/diff/40001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc:49: size_t len =
strlen(ptr);
OK, but you could just use path.Part(segment).length() and avoid the extra loop
over the string.

https://codereview.chromium.org/605513002/diff/40001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc (right):

https://codereview.chromium.org/605513002/diff/40001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc:133:
file_name_length);
nit: file_name_length should align with stat_ above.

[noelallen1, 2014-09-25 22:39:55]

https://codereview.chromium.org/605513002/diff/40001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc (right):

https://codereview.chromium.org/605513002/diff/40001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc:49: size_t len =
strlen(ptr);
On 2014/09/25 21:52:04, binji wrote:
> OK, but you could just use path.Part(segment).length() and avoid the extra
loop
> over the string.

Done.

https://codereview.chromium.org/605513002/diff/40001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc (right):

https://codereview.chromium.org/605513002/diff/40001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc:133:
file_name_length);
On 2014/09/25 21:52:04, binji wrote:
> nit: file_name_length should align with stat_ above.

Done.

[noelallen1, 2014-09-25 22:39:59]

The CQ bit was checked by noelallen@chromium.org

[commit-bot: I haz the power, 2014-09-25 22:41:23]

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

[commit-bot: I haz the power, 2014-09-26 00:15:28]

Committed patchset #4 (id:60001) as f6b2de16639e5ac9b58e18ae58fb8372ebaa6366

[commit-bot: I haz the power, 2014-09-26 00:16:10]

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