Clean up locking.

Clean up locking.

1- Move all kernel locks to KernelObject
2- Never take a kernel lock outside of KernelObject functions.
3- Rename locks to more accurately describe what they protect.
4- Never take more than one kernel lock at a time.
5- "Hide" CWD from KernelProxy, it always deals with relative paths.

BUG=none
NOTRY=true

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

[noelallen1, 2013-07-10 18:52:34]

ptal

This CL is a precursor to the Event/Epoll CLs to clean up locking and make
locking constrains easier to reason.

[binji, 2013-07-10 21:24:33]

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/kernel_object.cc (right):

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:41: std::string
abs_targ = GetAbsPath(path);
nit: how about just "abs_path"

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:52: std::string
abs_targ = GetAbsPath(path);
ditto

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:66: 
nit: remove extra line

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:75: AutoLock
lock(&mount_lock_);
This can move below clearing out_mount/out_path.

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:95: // Given a path,
acquire the associated mount and node, creating then
s/then/the/

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:115: std::string
KernelObject::GetAbsPath(const std::string& path) {
Why not return this is a Path, like before? Otherwise if a caller wants a Path
they'll have to re-split it.

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:146: struct stat
statbuf;
unused, remove

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/kernel_object.h (right):

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:21: class
PepperInterface;
this isn't used in this file, remove

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:38: Error
MountAtPath(const ScopedMount& mnt, const std::string& path);
MountAtPath is a bit confusing because mount can be interpreted as a noun or
verb. Maybe AttachMountAtPath?

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:41: Error
UnmountPath(const std::string& path);
Maybe rename this to DetachMountAtPath to match above?

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:50: // Find the mount
and node for the given path, acquiring/creating it add
remove "add"

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:82: // Lock to protect
mapping from FD to KernelHandle
I'd prefer specifying the member variable that is protected here instead:

"Lock to protect handle_map_."

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:85: // Lock to protect
mapping from path to Mount
ditto

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (right):

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:76: 
move open_resource up here to match header order?

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:204: int
KernelProxy::utime(const char* path, const struct utimbuf* times) {
"filename" matches the linux man page... but if you want to change it here,
change it in the header too

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:272: smap["TARGET"] =
target;
need to make this absolute, right?

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:292: Error error =
fits on one line now

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.h (right):

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.h:27: // MountNode's lock.
Might be nice to have a quick diagram of the locking order here:

KernelObject -> KernelHandle -> MountNode

[noelallen1, 2013-07-10 22:11:07]

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/kernel_object.cc (right):

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:41: std::string
abs_targ = GetAbsPath(path);
On 2013/07/10 21:24:34, binji wrote:
> nit: how about just "abs_path"

Done.

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:52: std::string
abs_targ = GetAbsPath(path);
On 2013/07/10 21:24:34, binji wrote:
> ditto

Done.

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:66: 
On 2013/07/10 21:24:34, binji wrote:
> nit: remove extra line

Done.

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:75: AutoLock
lock(&mount_lock_);
On 2013/07/10 21:24:34, binji wrote:
> This can move below clearing out_mount/out_path.

Done.

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:95: // Given a path,
acquire the associated mount and node, creating then
On 2013/07/10 21:24:34, binji wrote:
> s/then/the/

Done.

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:115: std::string
KernelObject::GetAbsPath(const std::string& path) {
On 2013/07/10 21:24:34, binji wrote:
> Why not return this is a Path, like before? Otherwise if a caller wants a Path
> they'll have to re-split it.

Done.

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:146: struct stat
statbuf;
On 2013/07/10 21:24:34, binji wrote:
> unused, remove

Done.

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/kernel_object.h (right):

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:21: class
PepperInterface;
On 2013/07/10 21:24:34, binji wrote:
> this isn't used in this file, remove

Done.

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:38: Error
MountAtPath(const ScopedMount& mnt, const std::string& path);
On 2013/07/10 21:24:34, binji wrote:
> MountAtPath is a bit confusing because mount can be interpreted as a noun or
> verb. Maybe AttachMountAtPath?

Done.

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:41: Error
UnmountPath(const std::string& path);
On 2013/07/10 21:24:34, binji wrote:
> Maybe rename this to DetachMountAtPath to match above?

Done.

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:50: // Find the mount
and node for the given path, acquiring/creating it add
On 2013/07/10 21:24:34, binji wrote:
> remove "add"

Done.

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:82: // Lock to protect
mapping from FD to KernelHandle
On 2013/07/10 21:24:34, binji wrote:
> I'd prefer specifying the member variable that is protected here instead:
> 
> "Lock to protect handle_map_."

Done.

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:85: // Lock to protect
mapping from path to Mount
On 2013/07/10 21:24:34, binji wrote:
> ditto

Done.

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (right):

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:76: 
On 2013/07/10 21:24:34, binji wrote:
> move open_resource up here to match header order?

Done.

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:204: int
KernelProxy::utime(const char* path, const struct utimbuf* times) {
On 2013/07/10 21:24:34, binji wrote:
> "filename" matches the linux man page... but if you want to change it here,
> change it in the header too

Done.

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:272: smap["TARGET"] =
target;
On 2013/07/10 21:24:34, binji wrote:
> need to make this absolute, right?

Done.

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:292: Error error =
On 2013/07/10 21:24:34, binji wrote:
> fits on one line now

Done.

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.h (right):

https://codereview.chromium.org/18129008/diff/10001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.h:27: // MountNode's lock.
On 2013/07/10 21:24:34, binji wrote:
> Might be nice to have a quick diagram of the locking order here:
> 
> KernelObject -> KernelHandle -> MountNode

There is only the one legal lock chain:
   KernelHandle -> MountNode

No other lock multiple lock conditions are legal.   Is the text above confusing?

[binji, 2013-07-10 22:16:32]

lgtm

https://codereview.chromium.org/18129008/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/kernel_object.cc (right):

https://codereview.chromium.org/18129008/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:41: std::string
abs_path = GetAbsPath(path);
GetAbsParts?

[commit-bot: I haz the power, 2013-07-11 01:28:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noelallen@chromium.org/18129008/23001

[commit-bot: I haz the power, 2013-07-11 01:28:58]

Change committed as 211003