Added simplest device mount implementation (includes NullDevice and RandomDevice)

Added simplest device mount implementation (includes NullDevice and RandomDevice)

BUG=http://code.google.com/p/nativeclient/issues/detail?id=2759
Committed: https://code.google.com/p/naclports/source/detail?r=582

[vissi, 2012-05-11 08:00:03]

Please take a look

[Evgeniy Stepanov, 2012-05-11 08:16:37]

You are not attaching a RandomDevice to the filesystem. This can not work.
Please write some tests (see test/BufferMountTest.cc for example).

https://chromiumcodereview.appspot.com/10377102/diff/7002/libraries/nacl-moun...
File libraries/nacl-mounts/dev/DevMount.h (right):

https://chromiumcodereview.appspot.com/10377102/diff/7002/libraries/nacl-moun...
libraries/nacl-mounts/dev/DevMount.h:22: // or "/def/tty".  Otherwise, Creat()
fails.
I'd skip this comment, or move it to .cc - it's going to change every time we
add a new device, no need to enumerate them all.

https://chromiumcodereview.appspot.com/10377102/diff/7002/libraries/nacl-moun...
File libraries/nacl-mounts/dev/NullDevice.h (right):

https://chromiumcodereview.appspot.com/10377102/diff/7002/libraries/nacl-moun...
libraries/nacl-mounts/dev/NullDevice.h:16: int GetInode() { return 1 };
No magic constants please. Either assign them in DevMount::Attach, or list them
in one header so it's easy to see that we don't assign the same inode number to
different devices by mistake.

https://chromiumcodereview.appspot.com/10377102/diff/7002/libraries/nacl-moun...
File libraries/nacl-mounts/dev/RandomDevice.cc (right):

https://chromiumcodereview.appspot.com/10377102/diff/7002/libraries/nacl-moun...
libraries/nacl-mounts/dev/RandomDevice.cc:7: #include "../dev/NullDevice.h"
RandomDevice.h

https://chromiumcodereview.appspot.com/10377102/diff/7002/libraries/nacl-moun...
libraries/nacl-mounts/dev/RandomDevice.cc:9: int NullDevice::Read(off_t offset,
void *buf, size_t count) {
RandomDevice

[vissi, 2012-05-11 08:31:05]

Yes, that was intentional, random device impl should be discussed. NaCl SSH has
the following code commented out:

  // NACL_IRT_RANDOM_v0_1 is available starting from M18. For testing purpose
  // is JS path for all browsers.
//  nacl_irt_random random;
//  if (nacl_interface_query(NACL_IRT_RANDOM_v0_1, &random, sizeof(random))) {
//    AddPathHandler("/dev/random",
//                   new DevRandomHandler(random.get_random_bytes));
//  } else {
    // LOG("Can't get " NACL_IRT_RANDOM_v0_1 " interface\n");

Can it be used now?

https://chromiumcodereview.appspot.com/10377102/diff/7002/libraries/nacl-moun...
File libraries/nacl-mounts/dev/DevMount.h (right):

https://chromiumcodereview.appspot.com/10377102/diff/7002/libraries/nacl-moun...
libraries/nacl-mounts/dev/DevMount.h:22: // or "/def/tty".  Otherwise, Creat()
fails.
On 2012/05/11 08:16:37, Evgeniy Stepanov wrote:
> I'd skip this comment, or move it to .cc - it's going to change every time we
> add a new device, no need to enumerate them all.

Done.

https://chromiumcodereview.appspot.com/10377102/diff/7002/libraries/nacl-moun...
File libraries/nacl-mounts/dev/NullDevice.h (right):

https://chromiumcodereview.appspot.com/10377102/diff/7002/libraries/nacl-moun...
libraries/nacl-mounts/dev/NullDevice.h:16: int GetInode() { return 1 };
On 2012/05/11 08:16:37, Evgeniy Stepanov wrote:
> No magic constants please. Either assign them in DevMount::Attach, or list
them
> in one header so it's easy to see that we don't assign the same inode number
to
> different devices by mistake.

Done.

https://chromiumcodereview.appspot.com/10377102/diff/7002/libraries/nacl-moun...
File libraries/nacl-mounts/dev/RandomDevice.cc (right):

https://chromiumcodereview.appspot.com/10377102/diff/7002/libraries/nacl-moun...
libraries/nacl-mounts/dev/RandomDevice.cc:7: #include "../dev/NullDevice.h"
On 2012/05/11 08:16:37, Evgeniy Stepanov wrote:
> RandomDevice.h

Done.

https://chromiumcodereview.appspot.com/10377102/diff/7002/libraries/nacl-moun...
libraries/nacl-mounts/dev/RandomDevice.cc:9: int NullDevice::Read(off_t offset,
void *buf, size_t count) {
On 2012/05/11 08:16:37, Evgeniy Stepanov wrote:
> RandomDevice

Done.

[Evgeniy Stepanov, 2012-05-11 08:46:38]

What does nacl-ssh fallback to then? It can not work without a random number
generator.
Anyway, I think we can target M18+.

https://chromiumcodereview.appspot.com/10377102/diff/5/libraries/nacl-mounts/...
File libraries/nacl-mounts/dev/DevMount.cc (right):

https://chromiumcodereview.appspot.com/10377102/diff/5/libraries/nacl-mounts/...
libraries/nacl-mounts/dev/DevMount.cc:70: return -1;
Please set errno when returning with an error.

[vissi, 2012-05-11 08:55:39]

It uses JSFileHandler

https://chromiumcodereview.appspot.com/10377102/diff/5/libraries/nacl-mounts/...
File libraries/nacl-mounts/dev/DevMount.cc (right):

https://chromiumcodereview.appspot.com/10377102/diff/5/libraries/nacl-mounts/...
libraries/nacl-mounts/dev/DevMount.cc:70: return -1;
On 2012/05/11 08:46:38, Evgeniy Stepanov wrote:
> Please set errno when returning with an error.

Done.

[vissi, 2012-05-11 09:45:37]

On 2012/05/11 08:55:39, vissi wrote:
> It uses JSFileHandler
> 
>
https://chromiumcodereview.appspot.com/10377102/diff/5/libraries/nacl-mounts/...
> File libraries/nacl-mounts/dev/DevMount.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10377102/diff/5/libraries/nacl-mounts/...
> libraries/nacl-mounts/dev/DevMount.cc:70: return -1;
> On 2012/05/11 08:46:38, Evgeniy Stepanov wrote:
> > Please set errno when returning with an error.
> 
> Done.

Updated makefile and build shell script.

[Evgeniy Stepanov, 2012-05-11 10:46:56]

On 2012/05/11 09:45:37, vissi wrote:
> On 2012/05/11 08:55:39, vissi wrote:
> > It uses JSFileHandler
> > 
> >
>
https://chromiumcodereview.appspot.com/10377102/diff/5/libraries/nacl-mounts/...
> > File libraries/nacl-mounts/dev/DevMount.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10377102/diff/5/libraries/nacl-mounts/...
> > libraries/nacl-mounts/dev/DevMount.cc:70: return -1;
> > On 2012/05/11 08:46:38, Evgeniy Stepanov wrote:
> > > Please set errno when returning with an error.
> > 
> > Done.
> 
> Updated makefile and build shell script.

Please write some sel_ldr-based tests (see test.nacl/SimpleTest.cc).

[Evgeniy Stepanov, 2012-05-11 10:47:34]

https://chromiumcodereview.appspot.com/10377102/diff/13005/libraries/nacl-mou...
File libraries/nacl-mounts/dev/DevMount.cc (right):

https://chromiumcodereview.appspot.com/10377102/diff/13005/libraries/nacl-mou...
libraries/nacl-mounts/dev/DevMount.cc:39: inode_to_ref[node]--;
I suggest dropping all reference counting in DevMount (inode_to_ref, Ref,
Unref). It does not do anything in the current implementation, and it is not
needed for a static file system.

https://chromiumcodereview.appspot.com/10377102/diff/13005/libraries/nacl-mou...
libraries/nacl-mounts/dev/DevMount.cc:43: int DevMount::Creat(const std::string&
path, mode_t mode, struct stat *buf) {
Missing return on one of the code paths.
This function is not needed anyway - you should not be able to create new files
in /dev.
Rename it to GetNode and handle the missing path case.

https://chromiumcodereview.appspot.com/10377102/diff/13005/libraries/nacl-mou...
libraries/nacl-mounts/dev/DevMount.cc:51: errno = EIO;
ENOENT

https://chromiumcodereview.appspot.com/10377102/diff/13005/libraries/nacl-mou...
libraries/nacl-mounts/dev/DevMount.cc:60: int DevMount::Getdents(ino_t slot,
off_t offset,
You don't have inodes for any directories, so this can not work, either. Just
drop it.

[vissi, 2012-05-11 11:49:18]

https://chromiumcodereview.appspot.com/10377102/diff/13005/libraries/nacl-mou...
File libraries/nacl-mounts/dev/DevMount.cc (right):

https://chromiumcodereview.appspot.com/10377102/diff/13005/libraries/nacl-mou...
libraries/nacl-mounts/dev/DevMount.cc:39: inode_to_ref[node]--;
On 2012/05/11 10:47:34, Evgeniy Stepanov wrote:
> I suggest dropping all reference counting in DevMount (inode_to_ref, Ref,
> Unref). It does not do anything in the current implementation, and it is not
> needed for a static file system.

Done.

https://chromiumcodereview.appspot.com/10377102/diff/13005/libraries/nacl-mou...
libraries/nacl-mounts/dev/DevMount.cc:43: int DevMount::Creat(const std::string&
path, mode_t mode, struct stat *buf) {
On 2012/05/11 10:47:34, Evgeniy Stepanov wrote:
> Missing return on one of the code paths.
> This function is not needed anyway - you should not be able to create new
files
> in /dev.
> Rename it to GetNode and handle the missing path case.

Done.

https://chromiumcodereview.appspot.com/10377102/diff/13005/libraries/nacl-mou...
libraries/nacl-mounts/dev/DevMount.cc:51: errno = EIO;
On 2012/05/11 10:47:34, Evgeniy Stepanov wrote:
> ENOENT

Done.

https://chromiumcodereview.appspot.com/10377102/diff/13005/libraries/nacl-mou...
libraries/nacl-mounts/dev/DevMount.cc:60: int DevMount::Getdents(ino_t slot,
off_t offset,
On 2012/05/11 10:47:34, Evgeniy Stepanov wrote:
> You don't have inodes for any directories, so this can not work, either. Just
> drop it.

Should we be able to list existing devices? That's the only purpose for now.

[Evgeniy Stepanov, 2012-05-11 12:02:54]

https://chromiumcodereview.appspot.com/10377102/diff/13005/libraries/nacl-mou...
File libraries/nacl-mounts/dev/DevMount.cc (right):

https://chromiumcodereview.appspot.com/10377102/diff/13005/libraries/nacl-mou...
libraries/nacl-mounts/dev/DevMount.cc:60: int DevMount::Getdents(ino_t slot,
off_t offset,
On 2012/05/11 11:49:19, vissi wrote:
> On 2012/05/11 10:47:34, Evgeniy Stepanov wrote:
> > You don't have inodes for any directories, so this can not work, either.
Just
> > drop it.
> 
> Should we be able to list existing devices? That's the only purpose for now.

Does it work this way? I suspect not. We need a test to figure it out.

Do we really need this functionality? It will complicate stuff a bit, we'll need
to allocate inodes for directories then (at least for "/").

[vissi, 2012-05-11 13:22:14]

On 2012/05/11 12:02:54, Evgeniy Stepanov wrote:
>
https://chromiumcodereview.appspot.com/10377102/diff/13005/libraries/nacl-mou...
> File libraries/nacl-mounts/dev/DevMount.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10377102/diff/13005/libraries/nacl-mou...
> libraries/nacl-mounts/dev/DevMount.cc:60: int DevMount::Getdents(ino_t slot,
> off_t offset,
> On 2012/05/11 11:49:19, vissi wrote:
> > On 2012/05/11 10:47:34, Evgeniy Stepanov wrote:
> > > You don't have inodes for any directories, so this can not work, either.
> Just
> > > drop it.
> > 
> > Should we be able to list existing devices? That's the only purpose for now.
> 
> Does it work this way? I suspect not. We need a test to figure it out.
> 
> Do we really need this functionality? It will complicate stuff a bit, we'll
need
> to allocate inodes for directories then (at least for "/").

Added a test.

[vissi, 2012-05-11 13:54:36]

On 2012/05/11 13:22:14, vissi wrote:
> On 2012/05/11 12:02:54, Evgeniy Stepanov wrote:
> >
>
https://chromiumcodereview.appspot.com/10377102/diff/13005/libraries/nacl-mou...
> > File libraries/nacl-mounts/dev/DevMount.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10377102/diff/13005/libraries/nacl-mou...
> > libraries/nacl-mounts/dev/DevMount.cc:60: int DevMount::Getdents(ino_t slot,
> > off_t offset,
> > On 2012/05/11 11:49:19, vissi wrote:
> > > On 2012/05/11 10:47:34, Evgeniy Stepanov wrote:
> > > > You don't have inodes for any directories, so this can not work, either.
> > Just
> > > > drop it.
> > > 
> > > Should we be able to list existing devices? That's the only purpose for
now.
> > 
> > Does it work this way? I suspect not. We need a test to figure it out.
> > 
> > Do we really need this functionality? It will complicate stuff a bit, we'll
> need
> > to allocate inodes for directories then (at least for "/").
> 
> Added a test.

Added sel_ldr test.

[vissi, 2012-05-11 14:22:26]

On 2012/05/11 13:54:36, vissi wrote:
> On 2012/05/11 13:22:14, vissi wrote:
> > On 2012/05/11 12:02:54, Evgeniy Stepanov wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/10377102/diff/13005/libraries/nacl-mou...
> > > File libraries/nacl-mounts/dev/DevMount.cc (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/10377102/diff/13005/libraries/nacl-mou...
> > > libraries/nacl-mounts/dev/DevMount.cc:60: int DevMount::Getdents(ino_t
slot,
> > > off_t offset,
> > > On 2012/05/11 11:49:19, vissi wrote:
> > > > On 2012/05/11 10:47:34, Evgeniy Stepanov wrote:
> > > > > You don't have inodes for any directories, so this can not work,
either.
> > > Just
> > > > > drop it.
> > > > 
> > > > Should we be able to list existing devices? That's the only purpose for
> now.
> > > 
> > > Does it work this way? I suspect not. We need a test to figure it out.
> > > 
> > > Do we really need this functionality? It will complicate stuff a bit,
we'll
> > need
> > > to allocate inodes for directories then (at least for "/").
> > 
> > Added a test.
> 
> Added sel_ldr test.

Better test

[Evgeniy Stepanov, 2012-05-11 14:41:30]

lgtm