[NaCl SDK] nacl_io: Allows subtree of html5fs to be mounted.

[NaCl SDK] nacl_io: Allows subtree of html5fs to be mounted.

Honor the first argument of mount() when mount html5fs
filesystem. This allows us to mount just part of an
html5fs fileystem.

Also, fix and re-enable the rmdir/unlink/remove tests for 
html5fs and add more error checking to that parameter 
parsing in Init().

R=binji

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276393

[Sam Clegg, 2014-06-09 18:47:51]

I'd like to use this in naclports for this kind of thing:

mount("/home", "/home", "html5", NULL);

This only the /home tree from html5fs is mounted there.

Thinking about it now, I supposed that adding symlinks would
be an alternative way of doing this and perhaps more unixy.

[Matthew Turk, 2014-06-09 20:30:09]

On 2014/06/09 18:47:51, Sam Clegg wrote:
> I'd like to use this in naclports for this kind of thing:
> 
> mount("/home", "/home", "html5", NULL);
> 
> This only the /home tree from html5fs is mounted there.
> 
> Thinking about it now, I supposed that adding symlinks would
> be an alternative way of doing this and perhaps more unixy.

This would also make it cleaner to mount external filesystems.  If you select a
directory, that directory exists in the filesystem passed in to pepper, so this
would remove one additional level.

[binji, 2014-06-09 20:54:30]

lgtm

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

https://codereview.chromium.org/320983002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc:60: //if
(!prefix_.empty())
remove

https://codereview.chromium.org/320983002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc:155: oldpath_full));
do these fit on the previous line now?

https://codereview.chromium.org/320983002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc:204: } else if
(iter->second == "") {
why allow this? The default is LOCALPERSISTENT

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

https://codereview.chromium.org/320983002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.h:41: virtual Error
RemoveInternal(const Path& path, int remove_type);
does this need to be virtual?

https://codereview.chromium.org/320983002/diff/1/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (left):

https://codereview.chromium.org/320983002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:427: smap["TARGET"] =
abs_path;
why?

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

https://codereview.chromium.org/320983002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/path.cc:47: if (paths.size() == 0)
I prefer paths.empty() for this, but it looks like paths.size() is used
elsewhere

https://codereview.chromium.org/320983002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/path.cc:70:
paths.push_back(paths_[index]);
why? If needed, be consistent with same in Path::Append

[Sam Clegg, 2014-06-10 17:20:59]

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

https://codereview.chromium.org/320983002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc:60: //if
(!prefix_.empty())
On 2014/06/09 20:54:29, binji wrote:
> remove

Done.

https://codereview.chromium.org/320983002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc:155: oldpath_full));
On 2014/06/09 20:54:29, binji wrote:
> do these fit on the previous line now?

Nope :(

https://codereview.chromium.org/320983002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc:204: } else if
(iter->second == "") {
On 2014/06/09 20:54:29, binji wrote:
> why allow this? The default is LOCALPERSISTENT

The unit tests rely on this.. maybe I should change them?

https://codereview.chromium.org/320983002/diff/1/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (left):

https://codereview.chromium.org/320983002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:427: smap["TARGET"] =
abs_path;
On 2014/06/09 20:54:29, binji wrote:
> why?

Its not uses, and shouldn't be.  The filesystem shouldn't need to know know
where its mounted.

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

https://codereview.chromium.org/320983002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/path.cc:47: if (paths.size() == 0)
On 2014/06/09 20:54:29, binji wrote:
> I prefer paths.empty() for this, but it looks like paths.size() is used
> elsewhere

Done.

https://codereview.chromium.org/320983002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/path.cc:70:
paths.push_back(paths_[index]);
On 2014/06/09 20:54:29, binji wrote:
> why? If needed, be consistent with same in Path::Append

This is simply a bug.

Append is pushing all the paths_ onto path... Prepend is pushing paths_ onto
paths.

The bug here is that paths was being pushed onto itself.

Guess we should have a test.

[Sam Clegg, 2014-06-10 17:50:02]

Add Path::Prepend() unittest.. and cleaned up Path a little while I was there.

Any reason why more methods on Path are not inline?

[Sam Clegg, 2014-06-10 17:51:18]

On 2014/06/10 17:50:02, Sam Clegg wrote:
> Add Path::Prepend() unittest.. and cleaned up Path a little while I was there.
> 
> Any reason why more methods on Path are not inline?

On second thought, I'll put the path fixes in a separate CL

[Sam Clegg, 2014-06-10 17:57:19]

Path changes split out.

PTAL

[binji, 2014-06-10 18:00:48]

On 2014/06/10 17:51:18, Sam Clegg wrote:
> On 2014/06/10 17:50:02, Sam Clegg wrote:
> > Add Path::Prepend() unittest.. and cleaned up Path a little while I was
there.
> > 
> > Any reason why more methods on Path are not inline?

Nope, I think they can be. Though the coding style says that in general
functions shouldn't be inline unless they are pure accessors.

> 
> On second thought, I'll put the path fixes in a separate CL

Well, you still need the Prepend fix, right?

[binji, 2014-06-10 18:00:58]

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

https://codereview.chromium.org/320983002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs.cc:204: } else if
(iter->second == "") {
On 2014/06/10 17:20:58, Sam Clegg wrote:
> On 2014/06/09 20:54:29, binji wrote:
> > why allow this? The default is LOCALPERSISTENT
> 
> The unit tests rely on this.. maybe I should change them?

I'd prefer it, it seems weird to add support for an empty type here.

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

https://codereview.chromium.org/320983002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/path.cc:70:
paths.push_back(paths_[index]);
On 2014/06/10 17:20:59, Sam Clegg wrote:
> On 2014/06/09 20:54:29, binji wrote:
> > why? If needed, be consistent with same in Path::Append
> 
> This is simply a bug.
> 
> Append is pushing all the paths_ onto path... Prepend is pushing paths_ onto
> paths.
> 
> The bug here is that paths was being pushed onto itself.
> 
> Guess we should have a test.
> 

I see.

[Sam Clegg, 2014-06-10 23:10:58]

The CQ bit was checked by sbc@chromium.org

[commit-bot: I haz the power, 2014-06-10 23:12:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sbc@chromium.org/320983002/60001

[commit-bot: I haz the power, 2014-06-11 05:16:00]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 14:40:38]

Change committed as 276393