Add mount sources for a MountNode (inode)

Add mount sources for a MountNode (inode)

Split abstact base mount.h and mount_mem.h
Add mount_mem test.

BUG=122229
R=binji@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=135033

[noelallen1, 2012-04-16 21:48:16]

NOTE:  There's a large block move of functions from mount.h to mount_mem.h.

[binji, 2012-04-16 22:43:05]

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
File native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc (right):

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:21:
MountMem::MountMem(int dev) :
nit: move : to next line, indent 4

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:34: root_->Release();
set root_ to NULL

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:37: MountNode
*MountMem::AllocatePath(int mode) {
nit: swap * side

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:40: // If we fail to
initialize, let the smart pointer delete it
Where is the smart pointer?

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:41: MountNode *ptr =
new MountNodeDir(this, ino, dev_);
nit: swap * side

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:42: if
(!ptr->Init(mode, 1002, 1003)) {
what are 1002, 1003?

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:50: MountNode
*MountMem::AllocateData(int mode) {
nit: swap * side

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:54: MountNode *ptr =
new MountNodeMem(this, ino, dev_);
nit: swap * side

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:70: for (int a=0; a <
7; a++) {
nit: spaces around =

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:74: return max_ino_ -
8;
How does this work? It doesn't look like anything is ever removed from inos_

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:81: MountNode
*MountMem::FindNode(const Path& path, int type) {
Can we assert in debug versions that the lock is held when these helper
functions are called?

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:82: MountNode *node =
root_;
nit: swap * side

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:122: // We now have a
valid directory object, so increment the ref count
Where do we increment a refcount?

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:127: MountNode
*MountMem::Open(const Path& path, int mode) {
nit: swap * side

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:140: mode =
OpenModeToPermission(mode);
Is OpenModeToPermission new? I can't find it.

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:143: 
nit: remove extra blank line

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:234: node->Release();
Document this release

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
File native_client_sdk/src/libraries/nacl_mounts/mount_mem.h (right):

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.h:16: MountMem(int dev);
explicit

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.h:21: // Allocata a node
assigning it's first reference to the passed in
Spelling: "Allocate" "its"

[noelallen1, 2012-04-21 18:09:53]

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
File native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc (right):

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:21:
MountMem::MountMem(int dev) :
On 2012/04/16 22:43:05, binji wrote:
> nit: move : to next line, indent 4

Done.

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:34: root_->Release();
On 2012/04/16 22:43:05, binji wrote:
> set root_ to NULL

Done.

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:37: MountNode
*MountMem::AllocatePath(int mode) {
On 2012/04/16 22:43:05, binji wrote:
> nit: swap * side

Done.

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:40: // If we fail to
initialize, let the smart pointer delete it
On 2012/04/16 22:43:05, binji wrote:
> Where is the smart pointer?

Done.

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:41: MountNode *ptr =
new MountNodeDir(this, ino, dev_);
On 2012/04/16 22:43:05, binji wrote:
> nit: swap * side

Done.

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:42: if
(!ptr->Init(mode, 1002, 1003)) {
On 2012/04/16 22:43:05, binji wrote:
> what are 1002, 1003?

Done.

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:50: MountNode
*MountMem::AllocateData(int mode) {
On 2012/04/16 22:43:05, binji wrote:
> nit: swap * side

Done.

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:54: MountNode *ptr =
new MountNodeMem(this, ino, dev_);
On 2012/04/16 22:43:05, binji wrote:
> nit: swap * side

Done.

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:70: for (int a=0; a <
7; a++) {
On 2012/04/16 22:43:05, binji wrote:
> nit: spaces around =

Done.

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:74: return max_ino_ -
8;
On 2012/04/16 22:43:05, binji wrote:
> How does this work? It doesn't look like anything is ever removed from inos_

Whoops.  Fixed.

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:81: MountNode
*MountMem::FindNode(const Path& path, int type) {
On 2012/04/16 22:43:05, binji wrote:
> Can we assert in debug versions that the lock is held when these helper
> functions are called?

Unless we specialize the lock, it won't tell us if it's held by this thread,
which is what we really want to know.  However that's probably a good feature to
add if we end up abstracting the lock.

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:82: MountNode *node =
root_;
On 2012/04/16 22:43:05, binji wrote:
> nit: swap * side

Done.

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:122: // We now have a
valid directory object, so increment the ref count
On 2012/04/16 22:43:05, binji wrote:
> Where do we increment a refcount?

Bad comment, fixed.

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:127: MountNode
*MountMem::Open(const Path& path, int mode) {
On 2012/04/16 22:43:05, binji wrote:
> nit: swap * side

Done.

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:140: mode =
OpenModeToPermission(mode);
On 2012/04/16 22:43:05, binji wrote:
> Is OpenModeToPermission new? I can't find it.

A public static defined in the parent class.

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:143: 
On 2012/04/16 22:43:05, binji wrote:
> nit: remove extra blank line

Done.

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:234: node->Release();
On 2012/04/16 22:43:05, binji wrote:
> Document this release

Done.

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
File native_client_sdk/src/libraries/nacl_mounts/mount_mem.h (right):

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.h:16: MountMem(int dev);
On 2012/04/16 22:43:05, binji wrote:
> explicit

Done globally

http://codereview.chromium.org/9956156/diff/1/native_client_sdk/src/libraries...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.h:21: // Allocata a node
assigning it's first reference to the passed in
On 2012/04/16 22:43:05, binji wrote:
> Spelling: "Allocate" "its"

Done.

[binji, 2012-04-23 17:51:40]

lgtm, just a couple things.

http://codereview.chromium.org/9956156/diff/7001/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc (right):

http://codereview.chromium.org/9956156/diff/7001/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:46: if
(!ptr->Init(mode, 1002, 1003)) {
USR_ID, GRP_ID

http://codereview.chromium.org/9956156/diff/7001/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:79:
inos_.push_back((max_ino_ - a) + 1);
If I'm reading this right, when this is first called:
inos_.size() == 0, so max_ino_ = 8. Then we loop a from 0 to 7, and push
(8-a)+1, which means inos_ has values (9,8,...,3,2), not (8,7,...,2,1).

http://codereview.chromium.org/9956156/diff/7001/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_mounts/nacl_mounts.vcproj (right):

http://codereview.chromium.org/9956156/diff/7001/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_mounts/nacl_mounts.vcproj:157:
RelativePath=".\mount_hfs.cc"
Where are these files?

[noelallen1, 2012-05-02 23:35:24]

http://codereview.chromium.org/9956156/diff/7001/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc (right):

http://codereview.chromium.org/9956156/diff/7001/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:46: if
(!ptr->Init(mode, 1002, 1003)) {
On 2012/04/23 17:51:40, binji wrote:
> USR_ID, GRP_ID

Done.

http://codereview.chromium.org/9956156/diff/7001/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_mounts/mount_mem.cc:79:
inos_.push_back((max_ino_ - a) + 1);
On 2012/04/23 17:51:40, binji wrote:
> If I'm reading this right, when this is first called:
> inos_.size() == 0, so max_ino_ = 8. Then we loop a from 0 to 7, and push
> (8-a)+1, which means inos_ has values (9,8,...,3,2), not (8,7,...,2,1).

Done.

http://codereview.chromium.org/9956156/diff/7001/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_mounts/nacl_mounts.vcproj (right):

http://codereview.chromium.org/9956156/diff/7001/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_mounts/nacl_mounts.vcproj:157:
RelativePath=".\mount_hfs.cc"
On 2012/04/23 17:51:40, binji wrote:
> Where are these files?

Done.