[NaCl SDK] nacl_io: big refactor to return error value (errno).

[NaCl SDK] nacl_io: big refactor to return error value (errno).

This is done to prevent forgetting to set it. It also isolates the
responsibility of setting errno to kernel_proxy.

BUG=244171
R=noelallen@chromium.org

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

[binji, 2013-05-31 16:49:54]

still a few things to clean up, but that is the brunt of the change. PTAL

[Sam Clegg, 2013-05-31 18:40:29]

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/error.h (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/error.h:8: // HACK(binji)
This whole file is a hack?  Why?

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/kernel_handle.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_handle.cc:34: return 0;
Should you return a special value here, or 0 OK as a value of Error?

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:321: }
Repetition like this makes me want to use a HANDLE_ERROR macro...

[binji, 2013-06-03 17:16:46]

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/error.h (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/error.h:8: // HACK(binji)
On 2013/05/31 18:40:29, Sam Clegg wrote:
> This whole file is a hack?  Why?

Sorry, didn't remove this yet. I was planning on having Error be a typedef for
int, instead of its own struct.

The reason for having a struct here is to have the compiler catch errors --
though maybe it is worthwhile to keep (with a #define?)

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/kernel_handle.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_handle.cc:34: return 0;
On 2013/05/31 18:40:29, Sam Clegg wrote:
> Should you return a special value here, or 0 OK as a value of Error?

Yes, 0 is OK. AFAIK, there is no EOK, value and I use the standard errno values
elsewhere. Maybe I should just add an EOK define?

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:321: }
On 2013/05/31 18:40:29, Sam Clegg wrote:
> Repetition like this makes me want to use a HANDLE_ERROR macro...

Agreed, though the if logic depends on what needs to be cleaned up for now. I'd
like to add a scoped kernel handle and scoped mount so cleanup is automatic. But
I think that makes sense for another CL.

[binji, 2013-06-05 16:09:59]

ping

[noelallen1, 2013-06-07 21:48:43]

Changing the interfaces now and dealing with the automation of Error, scoped
handles, etc.. For a later CL probably makes more sense.

Also, make sure that you document the use of errno.
if only KernelProxy sets and returns errno, then add a comment as such.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/error.h (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/error.h:10: Error(int error) :
error(error) {}
#ifdef DEBUG
  Error(int errno, const char *file, int line)
  #define Error(x) Error(x, __FILE__, __LINE__)
#endif

We could actually develop an error class that on debug versions could provide us
useful information.  This info could get logged to buffer and then periodically
scraped by the pepper thread, or even just available to via dubugging.
since we can't guaranty that we can safely printf due to reentrency/locking.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/kernel_handle.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_handle.cc:34: return 0;
If we do end up doing something interesting then

return Error(EOK);

Gives us the opportunity to track that we exited this function gracefully.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/kernel_object.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:64: // Acquire the
mount while we hold the proxy lock
if (*out_mount) {

Should we support only computing the path?  Otherwise EINVAL? for and invalid
param?

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:67: return 0;
Error(EOK)

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:77: 
Same.   EINVAL?

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:94: return 0;
Error(EOK)

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:119: // Force lower
numbered FD to be available first.
I don't think this comment or code is correct.
I believe the behavior we are looking for is the last FD freed, should be the
first one available, not the lowest numbered one.

So:
  close(0)
  close(1)
  open("/dev/stdout")
should in theory set STDOUT to /dev/stdout assuming no race.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/kernel_object.h (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:32: int
AcquireMountAndPath(const std::string& relpath,
Error?

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:38: int
AcquireHandle(int fd, KernelHandle** handle);
Error?

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:67: int
KernelProxy::open(const char* path, int oflags) {
Error KernelProxy::open?

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:102: int
KernelProxy::close(int fd) {
Error KernelProxy::close?

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:106: errno = error;
Should a non EOK set errno, so that we don't need to think about this?
Or is the most recent Error not the right thing?

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:127: int
KernelProxy::dup(int oldfd) {
Same and below.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:284: errno = EBUSY;
Error(EBUSY) first?

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:321: }
You mean?
#define  CHECK_ERRNO_RETURN(r, expr)
{
  Error error( expr );
  if (error) {
    errno = error;
    return r;
  }
}
 


On 2013/06/03 17:16:46, binji wrote:
> On 2013/05/31 18:40:29, Sam Clegg wrote:
> > Repetition like this makes me want to use a HANDLE_ERROR macro...
> 
> Agreed, though the if logic depends on what needs to be cleaned up for now.
I'd
> like to add a scoped kernel handle and scoped mount so cleanup is automatic.
But
> I think that makes sense for another CL.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:340: errno = EINVAL;
Error(EINVAL)?  Same below.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:397: KernelHandle*
handle;
   ScopedHandle handle;
   CHECK_ERRNO_RET(-1, AcquireHandle(fd, handle) );
   CHECK_ERRNO_RET(-1, handle->node_->GetStat(buf));
   return 0;
}

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:446: error =
handle->node_->FTruncate(length);
Should fill with 0 happen here?  Is there a way to make sure every virtual
FTruncate does the correct thing when growing file.   Maybe just add a comment
about this expectation in mountnode.h

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/mount.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount.cc:53: case O_RDONLY:
out = S_IREAD; break;
Doesn't match single line kernel_handle.cc switch case.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/mount.h (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount.h:94: Error error = mnt->Init(dev,
args, ppapi);
if (NULL == *out_mount)?

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/mount_dev.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount_dev.cc:330: return 0;
Error

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount_dev.cc:334: if (root_)
This assumes derefing the root will defref the children which is currently not
true.  We should fix that in MountNodeDir.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/mount_http.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount_http.cc:169: if (error)
style: Single line or {}

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/mount_mem.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount_mem.cc:47: if (node == NULL)
Single line?

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/mount_node.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount_node.cc:150: return 0;
Not ENOTDIR?

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/mount_node_mem.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount_node_mem.cc:67: size_t old_size =
stat_.st_size;
Losing virtual behavior or GetSize, is that what you want to do?

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io_test/example.dsc (left):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io_test/example.dsc:2: # TODO(binji): pnacl
doesn't build right now because gtest doesn't build yet.
Isn't this comment still valid?

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io_test/kernel_proxy_test.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io_test/kernel_proxy_test.cc:113: if (len >
-0)
-0?

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io_test/mount_http_test.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io_test/mount_http_test.cc:382:
EXPECT_EQ(0, node_->Read(0, buf, sizeof(buf) - 1, &result_bytes));
Set but not compared?

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io_test/mount_http_test.cc:411:
EXPECT_EQ(0, node_->Read(10, buf, sizeof(buf) - 1, &result_bytes));
result_butes set but not compared?

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io_test/mount_http_test.cc:420: size_t
result_size = 0;
unused?

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io_test/mount_http_test.cc:453: size_t
result_size = 0;
unused?

[binji, 2013-06-07 23:23:11]

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/error.h (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/error.h:10: Error(int error) :
error(error) {}
On 2013/06/07 21:48:43, noelallen1 wrote:
> #ifdef DEBUG
>   Error(int errno, const char *file, int line)
>   #define Error(x) Error(x, __FILE__, __LINE__)
> #endif
> 
> We could actually develop an error class that on debug versions could provide
us
> useful information.  This info could get logged to buffer and then
periodically
> scraped by the pepper thread, or even just available to via dubugging.
> since we can't guaranty that we can safely printf due to reentrency/locking.

Added TODO

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/kernel_handle.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_handle.cc:34: return 0;
On 2013/06/07 21:48:43, noelallen1 wrote:
> If we do end up doing something interesting then
> 
> return Error(EOK);
> 
> Gives us the opportunity to track that we exited this function gracefully.

OK, I'll do that as part of the next CL.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/kernel_object.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:64: // Acquire the
mount while we hold the proxy lock
On 2013/06/07 21:48:43, noelallen1 wrote:
> if (*out_mount) {
> 
> Should we support only computing the path?  Otherwise EINVAL? for and invalid
> param?

As per our discussion, non-NULL is assumed and documented in the header.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:67: return 0;
On 2013/06/07 21:48:43, noelallen1 wrote:
> Error(EOK)

Will fix in next CL.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:77: 
On 2013/06/07 21:48:43, noelallen1 wrote:
> Same.   EINVAL?

Done.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:94: return 0;
On 2013/06/07 21:48:43, noelallen1 wrote:
> Error(EOK)

Done.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/kernel_object.h (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:32: int
AcquireMountAndPath(const std::string& relpath,
On 2013/06/07 21:48:43, noelallen1 wrote:
> Error?

Done.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:38: int
AcquireHandle(int fd, KernelHandle** handle);
On 2013/06/07 21:48:43, noelallen1 wrote:
> Error?

Done.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:67: int
KernelProxy::open(const char* path, int oflags) {
On 2013/06/07 21:48:43, noelallen1 wrote:
> Error KernelProxy::open?

No, KernelProxy mimicks the actual open call, so it returns 0/-1 and sets errno,
etc.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:321: }
On 2013/06/07 21:48:43, noelallen1 wrote:
> You mean?
> #define  CHECK_ERRNO_RETURN(r, expr)
> {
>   Error error( expr );
>   if (error) {
>     errno = error;
>     return r;
>   }
> }
>  

Yes.

> 
> 
> On 2013/06/03 17:16:46, binji wrote:
> > On 2013/05/31 18:40:29, Sam Clegg wrote:
> > > Repetition like this makes me want to use a HANDLE_ERROR macro...
> > 
> > Agreed, though the if logic depends on what needs to be cleaned up for now.
> I'd
> > like to add a scoped kernel handle and scoped mount so cleanup is automatic.
> But
> > I think that makes sense for another CL.
>

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:397: KernelHandle*
handle;
On 2013/06/07 21:48:43, noelallen1 wrote:
>    ScopedHandle handle;
>    CHECK_ERRNO_RET(-1, AcquireHandle(fd, handle) );
>    CHECK_ERRNO_RET(-1, handle->node_->GetStat(buf));
>    return 0;
> }

Yep, future CL. :)

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:446: error =
handle->node_->FTruncate(length);
On 2013/06/07 21:48:43, noelallen1 wrote:
> Should fill with 0 happen here?  Is there a way to make sure every virtual
> FTruncate does the correct thing when growing file.   Maybe just add a comment
> about this expectation in mountnode.h

Done.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/mount.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount.cc:53: case O_RDONLY:
On 2013/06/07 21:48:43, noelallen1 wrote:
> out = S_IREAD; break;
> Doesn't match single line kernel_handle.cc switch case.

Ran clang-format on this and it did this. I just ran it on kernel_handle.cc and
it did it there too.

Do you prefer it single line? I'd rather not have to fix it after a
clang-format, personally.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/mount.h (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount.h:94: Error error = mnt->Init(dev,
args, ppapi);
On 2013/06/07 21:48:43, noelallen1 wrote:
> if (NULL == *out_mount)?

Added comment to Create declaration above.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/mount_dev.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount_dev.cc:330: return 0;
On 2013/06/07 21:48:43, noelallen1 wrote:
> Error

Done.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount_dev.cc:334: if (root_)
On 2013/06/07 21:48:43, noelallen1 wrote:
> This assumes derefing the root will defref the children which is currently not
> true.  We should fix that in MountNodeDir.

Done.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/mount_http.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount_http.cc:169: if (error)
On 2013/06/07 21:48:43, noelallen1 wrote:
> style: Single line or {}

After discussion, we'll leave without {}

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/mount_mem.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount_mem.cc:47: if (node == NULL)
On 2013/06/07 21:48:43, noelallen1 wrote:
> Single line?

Done.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/mount_node.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount_node.cc:150: return 0;
On 2013/06/07 21:48:43, noelallen1 wrote:
> Not ENOTDIR?

No, I figure that returning an error from ChildCount is more trouble than its
worth. It's not called directly, and anything that actually cares about the file
type will check it and set its own error.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/mount_node_mem.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount_node_mem.cc:67: size_t old_size =
stat_.st_size;
On 2013/06/07 21:48:43, noelallen1 wrote:
> Losing virtual behavior or GetSize, is that what you want to do?

Fair point, though any derived class (none of which currently exist) will likely
need to override more functions than just GetSize().

GetSize() now potentially returns an error, but that's silly to do for a memory
node, so using the value directly greatly simplifies the code.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io_test/example.dsc (left):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io_test/example.dsc:2: # TODO(binji): pnacl
doesn't build right now because gtest doesn't build yet.
On 2013/06/07 21:48:43, noelallen1 wrote:
> Isn't this comment still valid?

Whoops. No, pnacl works fine, I was uncommenting it because it was slow when I
was iterating. :)

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io_test/kernel_proxy_test.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io_test/kernel_proxy_test.cc:113: if (len >
-0)
On 2013/06/07 21:48:43, noelallen1 wrote:
> -0?

Done.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io_test/mount_http_test.cc (right):

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io_test/mount_http_test.cc:382:
EXPECT_EQ(0, node_->Read(0, buf, sizeof(buf) - 1, &result_bytes));
On 2013/06/07 21:48:43, noelallen1 wrote:
> Set but not compared?

Done.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io_test/mount_http_test.cc:411:
EXPECT_EQ(0, node_->Read(10, buf, sizeof(buf) - 1, &result_bytes));
On 2013/06/07 21:48:43, noelallen1 wrote:
> result_butes set but not compared?

Done.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io_test/mount_http_test.cc:420: size_t
result_size = 0;
On 2013/06/07 21:48:43, noelallen1 wrote:
> unused?

Done.

https://codereview.chromium.org/16232016/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io_test/mount_http_test.cc:453: size_t
result_size = 0;
On 2013/06/07 21:48:43, noelallen1 wrote:
> unused?

Done.

[noelallen1, 2013-06-07 23:44:03]

lgtm

https://codereview.chromium.org/16232016/diff/20001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_html5fs.h (right):

https://codereview.chromium.org/16232016/diff/20001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_html5fs.h:38: Error
filesystem_open_error_;      // protected by lock_.
This should probably become an int.  Either now or when Error supports logging.

[binji, 2013-06-10 21:16:58]

Committed patchset #5 manually as r205308 (presubmit successful).