Make the default directory size returned by stat to be 4096

[NaCl SDK] nacl_io: Make the default directory size returned by stat to be 4096

Committed: https://crrev.com/4b30ffe83c342e62be5682a1d96576367802511f
Cr-Commit-Position: refs/heads/master@{#343298}

[zhitingzhu, 2015-08-13 18:51:43]

zhitingzhu@google.com changed reviewers:
+ binji@google.com, sbc@google.com

[Sam Clegg, 2015-08-13 19:08:33]

sbc@chromium.org changed reviewers:
+ sbc@chromium.org

[Sam Clegg, 2015-08-13 19:08:33]

Can you start the CL description with "[NaCl SDK] nacl_io:"

What about other filesystem types?  Can we do this at a higher level perhaps?

https://codereview.chromium.org/1293543006/diff/1/native_client_sdk/src/libra...
File native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc (right):

https://codereview.chromium.org/1293543006/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc:171:
stat->st_size = static_cast<off_t>(info.size);
What does info.size mean for html5 fs?  Perhaps we should just completely ignore
it?

https://codereview.chromium.org/1293543006/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc:244: return 0;
Won't this early return?

Can you use a define rather then 4096?

[zhitingzhu, 2015-08-13 20:56:36]

I am not sure about other file systems. For higher level, do you mean changing
it at kernel proxy?

https://codereview.chromium.org/1293543006/diff/1/native_client_sdk/src/libra...
File native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc (right):

https://codereview.chromium.org/1293543006/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc:171:
stat->st_size = static_cast<off_t>(info.size);
On 2015/08/13 19:08:33, Sam Clegg wrote:
> What does info.size mean for html5 fs?  Perhaps we should just completely
ignore
> it?

I am not sure about where the info comes from. But it still return correct size
for files.

https://codereview.chromium.org/1293543006/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc:244: return 0;
On 2015/08/13 19:08:33, Sam Clegg wrote:
> Won't this early return?
> 
> Can you use a define rather then 4096?

Done. It seems GetSize only relates to file.

[Sam Clegg, 2015-08-13 21:33:48]

lgtm

https://codereview.chromium.org/1293543006/diff/20001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc (right):

https://codereview.chromium.org/1293543006/diff/20001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc:29: const int
DIR_SIZE = 4096;
How about EMPTY_DIR_SIZE?

https://codereview.chromium.org/1293543006/diff/20001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc:177:
stat->st_size = DIR_SIZE;
Move this up into the existing switch statement?


I might be nice to know if info.size is always zero?

[binji, 2015-08-13 21:44:51]

binji@chromium.org changed reviewers:
+ binji@chromium.org

[binji, 2015-08-13 21:44:52]

lgtm

https://codereview.chromium.org/1293543006/diff/20001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc (right):

https://codereview.chromium.org/1293543006/diff/20001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc:29: const int
DIR_SIZE = 4096;
On 2015/08/13 at 21:33:48, Sam Clegg wrote:
> How about EMPTY_DIR_SIZE?

Actually, Chromium style would be more like kEmptyDirSize

[zhitingzhu, 2015-08-13 22:26:00]

https://codereview.chromium.org/1293543006/diff/20001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc (right):

https://codereview.chromium.org/1293543006/diff/20001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc:29: const int
DIR_SIZE = 4096;
On 2015/08/13 21:44:52, binji wrote:
> On 2015/08/13 at 21:33:48, Sam Clegg wrote:
> > How about EMPTY_DIR_SIZE?
> 
> Actually, Chromium style would be more like kEmptyDirSize

Done.

https://codereview.chromium.org/1293543006/diff/20001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc:177:
stat->st_size = DIR_SIZE;
On 2015/08/13 21:33:48, Sam Clegg wrote:
> Move this up into the existing switch statement?
> 
> 
> I might be nice to know if info.size is always zero?

I try to look at the source code to see how the dir size get set, but could not
find a clue. But I guess it is always zero from what I see on devenv.

[Sam Clegg, 2015-08-13 22:29:01]

lgtm

https://codereview.chromium.org/1293543006/diff/40001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc (right):

https://codereview.chromium.org/1293543006/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc:172: if
(info.size == 0)
So I guess you can drop this conditional.  And add a comment that info.size is
always zero for directories.

[zhitingzhu, 2015-08-13 22:32:33]

https://codereview.chromium.org/1293543006/diff/40001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc (right):

https://codereview.chromium.org/1293543006/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/html5fs/html5_fs_node.cc:172: if
(info.size == 0)
On 2015/08/13 22:29:01, Sam Clegg wrote:
> So I guess you can drop this conditional.  And add a comment that info.size is
> always zero for directories.

Done.

[zhitingzhu, 2015-08-13 22:32:50]

The CQ bit was checked by zhitingzhu@google.com

[zhitingzhu, 2015-08-13 22:32:51]

The patchset sent to the CQ was uploaded after l-g-t-m from binji@chromium.org,
sbc@chromium.org
Link to the patchset: https://codereview.chromium.org/1293543006/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-08-13 22:33:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1293543006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1293543006/60001

[commit-bot: I haz the power, 2015-08-13 23:17:31]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-13 23:18:10]

Patchset 4 (id:??) landed as
https://crrev.com/4b30ffe83c342e62be5682a1d96576367802511f
Cr-Commit-Position: refs/heads/master@{#343298}