[NaCl SDK] Expose Google Drive to nacl_io.

[NaCl SDK] Expose Google Drive to nacl_io.

R=binji@chromium.org

[chanpatorikku, 2016-07-15 06:38:54]

Description was changed from

==========
[NaCl SDK] Expose Google Drive to nacl_io.

[NaCl SDK] Expose Google Drive to nacl_io.

R=binji@chromium.org
==========

to

==========
[NaCl SDK] Expose Google Drive to nacl_io.

R=binji@chromium.org
==========

[chanpatorikku, 2016-07-15 06:42:51]

Description was changed from

==========
[NaCl SDK] Expose Google Drive to nacl_io.

R=binji@chromium.org
==========

to

==========
[NaCl SDK] Expose Google Drive to nacl_io.

R=binji@chromium.org

Notes:

- Could you please reply what you think is expected in the code but is not found
in the code?

- I don't know about certain implementation details and would like to ask for
your comments.

The access time, modified time, and creation time are set as 0 in my
implementation of GetStat().
I based the code on HttpFsNode setting all 3 types of times to 0.
Google Drive has sharedWithMeTime, viewedBy, and modified time to set the access
time and modified time of a file, but creation time cannot be requested.

Could you please reply what you think about setting all 3 types of times to 0?

- stat.st_mode is not set in my implementation of GetStat().
I based the code on HttpFsNode not setting stat.st_mode in HttpFs.

Could you please reply what you think about my implementation of not
setting stat.st_mode?

- stat.st_nlink is not set in my implementation of GetStat().
I based the code on Html5FsNode not setting stat.st_nlink in Html5.

Could you please reply what you think about my implementation of not
setting stat.st_nlink?

- Each function call of GoogleDriveFsNode::Write() writes to Google Drive, but
compared to writing to a buffer in GoogleDriveFsNode::Write() and writing to
Google Drive onNode::Destroy(), it may be slow.

Could you please reply what you think about my implementation
of GoogleDriveFsNode::Write()?

- When there are too many directory entries in Google Drive,
in GoogleDriveFsNode::GetDentsGoogleDriveFsNode::GetDents

Could you please reply if this is OK not to deal with too many directory entries
at this moment?

- GoogleDriveFsNode::Remove(), GoogleDriveFsNode::Rename(), and
GoogleDriveFsNode::Unlink() are not supported.
I coded just enough to minimize the time before getting comments from you.

Could you please reply what you think about my not supporting the 3 functions?

- There is not a test written in
chromium/src/native_client_sdk/src/tests/nacl_io_test.  Except passthroughfs,
most filesystems possess a test.  Html5Fs has FileIoInterface, etc that can be
implemented to use fake interfaces.  Nonetheless, GoogleDriveFs is utilizing
URLLoader, URLRequestInfo, etc, which cannot be set to get fake interfaces.

Could you please reply what you think about my not having tests for
googledrivefs?
==========

[binji, 2016-07-18 22:45:39]

Thanks for adding your notes here, but please remove them from the CL
description. The purpose of the CL description is to explain to people who are
looking at the repository what changed and why. Also, if a particular question
is associated with a line of code, you can use the codereview tool to add your
own comment to that line. This will make it easier for me to see exactly what
you're referring to.

On 2016/07/15 06:42:51, chanpatorikku wrote:
> Description was changed from
> 
> ==========
> [NaCl SDK] Expose Google Drive to nacl_io.
> 
> mailto:R=binji@chromium.org
> ==========
> 
> to
> 
> ==========
> [NaCl SDK] Expose Google Drive to nacl_io.
> 
> mailto:R=binji@chromium.org
> 
> Notes:
> 
> - Could you please reply what you think is expected in the code but is not
found
> in the code?
> 
> - I don't know about certain implementation details and would like to ask for
> your comments.
> 
> The access time, modified time, and creation time are set as 0 in my
> implementation of GetStat().
> I based the code on HttpFsNode setting all 3 types of times to 0.
> Google Drive has sharedWithMeTime, viewedBy, and modified
time to set the access
> time and modified time of a file, but creation time cannot be requested.
> 
> Could you please reply what you think about setting all 3 types of times to 0?

I think it's reasonable as a start. But you could also just set mtime to
modified time.

> 
> - stat.st_mode is not set in my implementation of GetStat().
> I based the code on HttpFsNode not
setting stat.st_mode in HttpFs.
> 
> Could you please reply what you think about my implementation of not
> setting stat.st_mode?
> 
> - stat.st_nlink is not set in my implementation of GetStat().
> I based the code on Html5FsNode not
setting stat.st_nlink in Html5.
> 
> Could you please reply what you think about my implementation of not
> setting stat.st_nlink?

We don't use st_nlink currently, so this is OK.

> 
> - Each function call of GoogleDriveFsNode::Write() writes to Google
Drive, but
> compared to writing to a buffer in GoogleDriveFsNode::Write() and
writing to
> Google Drive onNode::Destroy(), it may be slow.
> 
> Could you please reply what you think about my implementation
> of GoogleDriveFsNode::Write()?

Yes, this is probably going to be problematic. The real solution is to introduce
generic buffering for nodes, but this is not currently implemented. Often what
we've done instead is use the libc buffering (calling setbuffer, etc.) This
doesn't help if the user calls write(), however.

> 
> - When there are too many directory entries in Google Drive,
> in GoogleDriveFsNode::GetDentsGoogleDriveFsNode::GetDents
> 
> Could you please reply if this is OK not to deal with too many directory
entries
> at this moment?

I'm not certain, it would probably be best to try and see if it is too slow. Or
is there a different issue (I can't tell from your comment).

> 
> - GoogleDriveFsNode::Remove(), GoogleDriveFsNode::Rename(), and
> GoogleDriveFsNode::Unlink() are not supported.
> I coded just enough to minimize the time before getting comments from you.
> 
> Could you please reply what you think about my not supporting the 3 functions?

These are commonly used (unlink, at least), and it will be a bit strange if they
are not supported. But it's OK to have a TODO here.

> 
> - There is not a test written in
> chromium/src/native_client_sdk/src/tests/nacl_io_test.  Except passthroughfs,
> most filesystems possess a test.  Html5Fs has FileIoInterface, etc that can be
> implemented to use fake interfaces.  Nonetheless, GoogleDriveFs is utilizing
> URLLoader, URLRequestInfo, etc, which cannot be set to get fake interfaces.

We do have a fake url loader, see
https://chromium.googlesource.com/chromium/src/+/master/native_client_sdk/src....

I agree that it will be difficult to write tests, but it is also not great to
add new code without tests.

> 
> Could you please reply what you think about my not having tests for
> googledrivefs?
> ==========

[binji, 2016-07-18 23:24:03]

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/examp...
File
native_client_sdk/src/examples/demo/googledrivefs_demo/googledrivefs_demo.cc
(right):

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/examp...
native_client_sdk/src/examples/demo/googledrivefs_demo/googledrivefs_demo.cc:1:
/* Copyright (c) 2016 The Chromium Authors. All rights reserved.
This ends up duplicating quite a bit of code from the nacl_io_demo. I think the
best choice here is to either add the functionality to the nacl_io_demo, or to
create a new standalone demo that is specialized to just the googledrivefs.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc
(right):

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:23:
#include "json/reader.h"
nacl_io currently does not depend on json; We're not going to want to add it as
a new dependency.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:31:
#include <windows.h>
what is this needed for?

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:65: ino_t
HashPath(const Path& path) {
better to share the code with Html5Fs rather than duplicating it here.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:89:
std::string url = "https://www.googleapis.com/drive/v3/files";
It's better to have these strings be constants (as much as possible) rather than
build them up inside the function.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:101:
GoogleDriveFs* googledrivefs = dynamic_cast<GoogleDriveFs*>(filesystem);
don't use dynamic_cast, it relies on RTTI. static_cast is sufficient, as these
functions should never be called when the filesystem is not a GoogleDriveFs.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:106:
pp::URLLoader loader(pp::InstanceHandle(googledrivefs->instance()));
nacl_io uses the Pepper C interface, not the C++ interface.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:109: bool
set_url_ok = request.SetURL(url);
it's better to just inline this than to create a new variable that's only used
once

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:119:
std::map<std::string, std::string> headers_map;
This seems to be duplicated a number of times in the file, it should probably be
a function.

You might try factoring it in a similar way as done in the Drive example.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:163: char
buffer[BUFFER_SIZE];
What if the result is greater than 1024 bytes?

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:167:
while (byte_read == 0) {
This loop is strange, what problem are you trying to fix? Ultimately, the
responsibility of retrying a failed read is up to the caller.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:171:
usleep(1000000);
We never want to call sleep in nacl_io, it's usually a sign that there is a
timing bug.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:335:
std::string& parent_dir_id) {
Out parameters should be pointers, not references, according to the style guide.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:362:
class GoogleDriveFsNode : public Node {
It's generally nicer when the nodes are defined in a separate file.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:423: char
file_buf[file_buf_size];
This will allocate space for the file on the stack. Typical stack sizes are
measured in megabytes, so this won't allow for files larger than that.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:767: if
(response.GetStatusCode() == 200 || response.GetStatusCode() == 206) {
use consts for the status codes instead (see http_fs_node.cc)

[chanpatorikku, 2016-07-19 18:35:52]

Description was changed from

==========
[NaCl SDK] Expose Google Drive to nacl_io.

R=binji@chromium.org

Notes:

- Could you please reply what you think is expected in the code but is not found
in the code?

- I don't know about certain implementation details and would like to ask for
your comments.

The access time, modified time, and creation time are set as 0 in my
implementation of GetStat().
I based the code on HttpFsNode setting all 3 types of times to 0.
Google Drive has sharedWithMeTime, viewedBy, and modified time to set the access
time and modified time of a file, but creation time cannot be requested.

Could you please reply what you think about setting all 3 types of times to 0?

- stat.st_mode is not set in my implementation of GetStat().
I based the code on HttpFsNode not setting stat.st_mode in HttpFs.

Could you please reply what you think about my implementation of not
setting stat.st_mode?

- stat.st_nlink is not set in my implementation of GetStat().
I based the code on Html5FsNode not setting stat.st_nlink in Html5.

Could you please reply what you think about my implementation of not
setting stat.st_nlink?

- Each function call of GoogleDriveFsNode::Write() writes to Google Drive, but
compared to writing to a buffer in GoogleDriveFsNode::Write() and writing to
Google Drive onNode::Destroy(), it may be slow.

Could you please reply what you think about my implementation
of GoogleDriveFsNode::Write()?

- When there are too many directory entries in Google Drive,
in GoogleDriveFsNode::GetDentsGoogleDriveFsNode::GetDents

Could you please reply if this is OK not to deal with too many directory entries
at this moment?

- GoogleDriveFsNode::Remove(), GoogleDriveFsNode::Rename(), and
GoogleDriveFsNode::Unlink() are not supported.
I coded just enough to minimize the time before getting comments from you.

Could you please reply what you think about my not supporting the 3 functions?

- There is not a test written in
chromium/src/native_client_sdk/src/tests/nacl_io_test.  Except passthroughfs,
most filesystems possess a test.  Html5Fs has FileIoInterface, etc that can be
implemented to use fake interfaces.  Nonetheless, GoogleDriveFs is utilizing
URLLoader, URLRequestInfo, etc, which cannot be set to get fake interfaces.

Could you please reply what you think about my not having tests for
googledrivefs?
==========

to

==========
[NaCl SDK] Expose Google Drive to nacl_io.

R=binji@chromium.org
==========

[chanpatorikku, 2016-08-04 13:10:53]

Description was changed from

==========
[NaCl SDK] Expose Google Drive to nacl_io.

R=binji@chromium.org
==========

to

==========
[NaCl SDK] Expose Google Drive to nacl_io.

R=binji@chromium.org
BUG=
==========

[chanpatorikku, 2016-08-07 01:03:34]

Patchset #2 (id:20001) has been deleted

[chanpatorikku, 2016-08-07 01:15:37]

Description was changed from

==========
[NaCl SDK] Expose Google Drive to nacl_io.

R=binji@chromium.org
BUG=
==========

to

==========
[NaCl SDK] Expose Google Drive to nacl_io.

R=binji@chromium.org
==========

[chanpatorikku, 2016-08-07 01:22:50]

"BUG=" was added to the description, but the description was not being changed
intentionally.  Please ignore.   

Patch set 2 was deleted.  Somehow the same patches were uploaded twice, so the
first patch, patch set 2, was deleted.  Please ignore.

For your comments about my concerns inquired in native-client-discuss, my reply
is shown below.

On 2016/07/18 22:45:39, binji wrote:
> Thanks for adding your notes here, but please remove them from the CL
> description. The purpose of the CL description is to explain to people who are
> looking at the repository what changed and why. Also, if a particular question
> is associated with a line of code, you can use the codereview tool to add your
> own comment to that line. This will make it easier for me to see exactly what
> you're referring to.
> 
> On 2016/07/15 06:42:51, chanpatorikku wrote:
> > Description was changed from
> > 
> > ==========
> > [NaCl SDK] Expose Google Drive to nacl_io.
> > 
> > mailto:R=binji@chromium.org
> > ==========
> > 
> > to
> > 
> > ==========
> > [NaCl SDK] Expose Google Drive to nacl_io.
> > 
> > mailto:R=binji@chromium.org
> > 
> > Notes:
> > 
> > - Could you please reply what you think is expected in the code but is not
> found
> > in the code?
> > 
> > - I don't know about certain implementation details and would like to ask
for
> > your comments.
> > 
> > The access time, modified time, and creation time are set as 0 in my
> > implementation of GetStat().
> > I based the code on HttpFsNode setting all 3 types of times to 0.
> > Google Drive has sharedWithMeTime, viewedBy, and modified
> time to set the access
> > time and modified time of a file, but creation time cannot be requested.
> > 
> > Could you please reply what you think about setting all 3 types of times to
0?
> 
> I think it's reasonable as a start. But you could also just set mtime to
> modified time.
> 

Thanks.
mtime set to modified time. 

> > 
> > - stat.st_mode is not set in my implementation of GetStat().
> > I based the code on HttpFsNode not
> setting stat.st_mode in HttpFs.
> > 
> > Could you please reply what you think about my implementation of not
> > setting stat.st_mode?
> > 
> > - stat.st_nlink is not set in my implementation of GetStat().
> > I based the code on Html5FsNode not
> setting stat.st_nlink in Html5.
> > 
> > Could you please reply what you think about my implementation of not
> > setting stat.st_nlink?
> 
> We don't use st_nlink currently, so this is OK.
>

Gotcha about st_nlink.

Got an update about st_mode.
I did not know how to use st_mode which can record the read/write permission and
file/dir type.
st_mode has been used in the current submission.

> > 
> > - Each function call of GoogleDriveFsNode::Write() writes to Google
> Drive, but
> > compared to writing to a buffer in GoogleDriveFsNode::Write() and
> writing to
> > Google Drive onNode::Destroy(), it may be slow.
> > 
> > Could you please reply what you think about my implementation
> > of GoogleDriveFsNode::Write()?
> 
> Yes, this is probably going to be problematic. The real solution is to
introduce
> generic buffering for nodes, but this is not currently implemented. Often what
> we've done instead is use the libc buffering (calling setbuffer, etc.) This
> doesn't help if the user calls write(), however.
>

On apologizing my question about Destroy() & Write().
Write() is called when the buffered characters can write to Google Drive.

> > 
> > - When there are too many directory entries in Google Drive,
> > in GoogleDriveFsNode::GetDentsGoogleDriveFsNode::GetDents
> > 
> > Could you please reply if this is OK not to deal with too many directory
> entries
> > at this moment?
> 
> I'm not certain, it would probably be best to try and see if it is too slow.
Or
> is there a different issue (I can't tell from your comment).
> 

The question was partly shown after pasting from native-client-discuss forum.

The question has become not important.
I was not able to deal with too many directory entries, but I see a way now.

The original question is in native-client-discuss forum if you would like to
know the full comment.


> > 
> > - GoogleDriveFsNode::Remove(), GoogleDriveFsNode::Rename(), and
> > GoogleDriveFsNode::Unlink() are not supported.
> > I coded just enough to minimize the time before getting comments from you.
> > 
> > Could you please reply what you think about my not supporting the 3
functions?
> 
> These are commonly used (unlink, at least), and it will be a bit strange if
they
> are not supported. But it's OK to have a TODO here.
>

Thanks.
Wrote TODO.
 
> > 
> > - There is not a test written in
> > chromium/src/native_client_sdk/src/tests/nacl_io_test.  Except
passthroughfs,
> > most filesystems possess a test.  Html5Fs has FileIoInterface, etc that can
be
> > implemented to use fake interfaces.  Nonetheless, GoogleDriveFs is utilizing
> > URLLoader, URLRequestInfo, etc, which cannot be set to get fake interfaces.
> 
> We do have a fake url loader, see
>
https://chromium.googlesource.com/chromium/src/+/master/native_client_sdk/src....
> 
> I agree that it will be difficult to write tests, but it is also not great to
> add new code without tests.
>

Added tests.
The tests are built on the code of the fake url loader.
 
> > 
> > Could you please reply what you think about my not having tests for
> > googledrivefs?
> > ==========

[chanpatorikku, 2016-08-07 02:41:03]

Ben,

Codes of GoogleDriveFs have been rewritten for addressing your comments.

Here is information that is good to know about GoogleDriveFs:

- GoogleDriveFs functionality is defined by the test cases commented as:

// 
// <test case description>
// 

- CLIENT_ID and CLIENT_SECRET in nacl_io_demo/example.js is set to "TO FILL" to
be filled in from the values from Google Developers Console.

The message may be better sent on the weekday for fitting your time.  About the
patch set, patch uploads were not going well until the current patch was
uploaded.  To prevent delays, the upload was done as soon as I could.

When you have time, could you please review the patch set?

Thanks,
Patrick

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/examp...
File
native_client_sdk/src/examples/demo/googledrivefs_demo/googledrivefs_demo.cc
(right):

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/examp...
native_client_sdk/src/examples/demo/googledrivefs_demo/googledrivefs_demo.cc:1:
/* Copyright (c) 2016 The Chromium Authors. All rights reserved.
On 2016/07/18 23:24:02, binji wrote:
> This ends up duplicating quite a bit of code from the nacl_io_demo. I think
the
> best choice here is to either add the functionality to the nacl_io_demo, or to
> create a new standalone demo that is specialized to just the googledrivefs.

Done.

Thanks.
Added the functionality to the nacl_io_demo.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc
(right):

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:23:
#include "json/reader.h"
On 2016/07/18 23:24:02, binji wrote:
> nacl_io currently does not depend on json; We're not going to want to add it
as
> a new dependency.

Done.

Thanks.
Not going to add it as a new dependency.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:31:
#include <windows.h>
On 2016/07/18 23:24:02, binji wrote:
> what is this needed for?

This used to be needed for Sleep() from windows.h.
Originally the implementation used usleep().
I saw that in Windows, Sleep(), but not usleep(), is used, and Sleep() is
included from windows.h.
The current submission removes windows.h and does not use Sleep() & usleep().

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:65: ino_t
HashPath(const Path& path) {
On 2016/07/18 23:24:02, binji wrote:
> better to share the code with Html5Fs rather than duplicating it here.

Done.

Thanks.
Shared the code with Html5Fs.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:89:
std::string url = "https://www.googleapis.com/drive/v3/files";
On 2016/07/18 23:24:02, binji wrote:
> It's better to have these strings be constants (as much as possible) rather
than
> build them up inside the function.


Each URL may be different.
I don't know if the current submission, which imitates how the Drive example
implements the URL strings, 
is practical.
Please review the codes that imitates the Drive example on implementing the URL
strings.
The lines of codes are commented for review.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:101:
GoogleDriveFs* googledrivefs = dynamic_cast<GoogleDriveFs*>(filesystem);
On 2016/07/18 23:24:02, binji wrote:
> don't use dynamic_cast, it relies on RTTI. static_cast is sufficient, as these
> functions should never be called when the filesystem is not a GoogleDriveFs.

Done.

Thanks.
static_cast is called in the current submission.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:106:
pp::URLLoader loader(pp::InstanceHandle(googledrivefs->instance()));
On 2016/07/18 23:24:02, binji wrote:
> nacl_io uses the Pepper C interface, not the C++ interface.

Done.

Thanks.
Pepper C interface is used in the current submission.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:109: bool
set_url_ok = request.SetURL(url);
On 2016/07/18 23:24:02, binji wrote:
> it's better to just inline this than to create a new variable that's only used
> once

I don't know exactly about inlining.
Please review the codes that attempts inlining.
The lines of codes are commented for review.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:119:
std::map<std::string, std::string> headers_map;
On 2016/07/18 23:24:02, binji wrote:
> This seems to be duplicated a number of times in the file, it should probably
be
> a function.
> 
> You might try factoring it in a similar way as done in the Drive example.

Done.

Thanks.
Tried factoring it in a similar way as done in the Drive example.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:163: char
buffer[BUFFER_SIZE];
On 2016/07/18 23:24:02, binji wrote:
> What if the result is greater than 1024 bytes?

Done.

Thanks.
Code is rewritten for the result greater than 1024 bytes in the current
submission.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:167:
while (byte_read == 0) {
On 2016/07/18 23:24:02, binji wrote:
> This loop is strange, what problem are you trying to fix? Ultimately, the
> responsibility of retrying a failed read is up to the caller.

Done.

The problem I was trying to fix was that FileIoInterface Read() returned 0 bytes
for multiple calls
before returning >0 bytes.  The original work around was to have a while loop
with usleep() so the 
bytes could be read until >0 bytes were read from FileIoInterface Read().

The problem has been fixed by using URLLoaderInterface FinishStreamingToFile().
Calling URLLoaderInterface FinishStreamingToFile before FileIoInterface Read()
reads >0 bytes
in the first call of FileIoInterface Read().

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:171:
usleep(1000000);
On 2016/07/18 23:24:02, binji wrote:
> We never want to call sleep in nacl_io, it's usually a sign that there is a
> timing bug.

Done.

Sleep is not called in nacl_io in the current submission.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:335:
std::string& parent_dir_id) {
On 2016/07/18 23:24:02, binji wrote:
> Out parameters should be pointers, not references, according to the style
guide.

Done.

Thanks.
Out parameters are pointers and are placed at last parameters in a function from
what the style guide writes.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:362:
class GoogleDriveFsNode : public Node {
On 2016/07/18 23:24:02, binji wrote:
> It's generally nicer when the nodes are defined in a separate file.

Done.

Thanks.
Nodes are defined in a separate file in the current submission.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:423: char
file_buf[file_buf_size];
On 2016/07/18 23:24:02, binji wrote:
> This will allocate space for the file on the stack. Typical stack sizes are
> measured in megabytes, so this won't allow for files larger than that.

Done.

Thanks.
Heap memory, as automatically allocated in string class and vector class, is
used in the current submission.

https://codereview.chromium.org/2156503002/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:767: if
(response.GetStatusCode() == 200 || response.GetStatusCode() == 206) {
On 2016/07/18 23:24:02, binji wrote:
> use consts for the status codes instead (see http_fs_node.cc)

Done.

Thanks.
Used consts for the status codes, and shared the status codes with
http_fs_node.cc.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/e...
File native_client_sdk/src/examples/demo/nacl_io_demo/example.dsc (right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io_demo/example.dsc:2: 'TOOLS':
['pnacl'],
Only pnacl is tested.  Could you please reply if only pnacl in the toolchain is
good?

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc
(right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:99: p.url
= base_url;
Lines 99-113 are samples of how codes imitate the Drive example.

They are continuing the comments in patch set 1:

"Each URL may be different.
I don't know if the current submission, which imitates how the Drive example
implements the URL strings, 
is practical.
Please review the codes that imitates the Drive example on implementing the URL
strings.
The lines of codes are commented for review."

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc
(right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:589:
GetValue(output, "modifiedTime", 0, &modified_time_value,
I don't know about how practical my implementation of GetValue() is.

GetValue() is called only at the time a response with status code 200 is there.

Response body is never cutoff in a response with status code 200.

GetValue() assumes that response body is never cutoff. When a response body is,
for example,

{

  “modifiedTime”: “2016-08-07T10:44:55.342Z”

}

, GetValue() never checks the case when there is not a second double quote after
the first double quote of the time value.

Could you please reply if the implementation is practical?

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc
(right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc:100:
if (!url_request_info_iface->SetProperty(url_request_info_object,
Lines 100-101 are samples of code that attempts inlining.

They are continuing the comments in patch set 1:

"I don't know exactly about inlining.
Please review the codes that attempts inlining.
The lines of codes are commented for review."

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/t...
File
native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_url_loader.cc
(right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_url_loader.cc:504:
// Throw the value away for now. TODO: add tests for this.
Another TODO, FYI.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/t...
File native_client_sdk/src/tests/nacl_io_test/googledrivefs_test.cc (right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/googledrivefs_test.cc:165: // open a
non-existing file in r mode.
nacl_io_demo covers only 6 different flags in file open.

Every test case of googledrivefs_test.cc covers any of the 6 flags.

Could you please reply if it's practical of the tests only covering 6 different
flags?

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/googledrivefs_test.cc:537: //
1300628744, though, is returned by mktime.
Test cases of lines 537-538 and lines 564-565 are verifying the values of mtime
calculated by mktime that uses the UTC time of modifiedTime responded from
Google Drive API v3.

I don't know why mktime does not return a UNIX timestamp equal to the target UTC
time.

Could you please reply what you think is practical on resolving?

[binji, 2016-08-22 19:21:53]

This is much better. I still think there are some things that can be simplified,
though. I read through all the code, but I still haven't done a very thorough
review yet. There's quite a bit here, and it takes a while to go over.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/e...
File native_client_sdk/src/examples/demo/nacl_io_demo/example.dsc (right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io_demo/example.dsc:2: 'TOOLS':
['pnacl'],
On 2016/08/07 02:41:02, chanpatorikku wrote:
> Only pnacl is tested.  Could you please reply if only pnacl in the toolchain
is
> good?

You should leave all the others in, even if you haven't tested them. The change
can't land, as-is.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/e...
File native_client_sdk/src/examples/demo/nacl_io_demo/example.js (right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io_demo/example.js:1: // Copyright (c)
2016 The Chromium Authors. All rights reserved.
It's not necessary to change the copyright year.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io_demo/example.js:5: var CLIENT_ID =
"TO FILL";
Where is this filled in? If it is meant to be filled in by the developer
building the example, that needs to be documented somewhere. The best place
would be on the example page. Similarly, if the value has not been filled in, a
clear error message should be displayed, explaining where to fill in the values
(and how to get them).

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io_demo/example.js:32: var codeKeyIndex
= currentURL.search("code=");
you can just use indexOf instead of search. search performs a regular expression
search.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc
(right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:33:
instance_ = atoi(instance_it->second.c_str());
you don't need to get this instance from the user, it is accessible through
ppapi_->GetInstance().

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:35:
std::map<std::string, std::string>::const_iterator token_it =
nit: Use StringMap_t instead of std::map<std::string, std::string>

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:66:
URLLoaderInterface* url_loader_iface = ppapi()->GetURLLoaderInterface();
you could cache these interface pointers as members (see html5_fs for example)

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:74:
std::string parent_dir_id = "";
nit: it's not necessary to initialize to "", that is the default value of
std::string.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:137: if
(url_loader_object) {
Use the ScopedResource instead (see pepper_interface.h)

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:163:
static const char base_url[] = "https://www.googleapis.com/drive/v3/files";
This seems to be repeated quite a bit, can you move it to a shared location?

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc
(right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:99:
off_t file_buffer_size = std::max((unsigned long long int)file_size,
use std::max<off_t> instead of casting the arguments

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:120:
error = WriteHelper(file_buffer.c_str(), file_buffer_size);
Is there no better way than reading the entire file, modifying a small portion,
then writing it back?

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:184:
pChar[i] = file_buffer[i];
why not read directly into buf?

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:402:
char_written = sprintf(range_value_buffer, "bytes=%lli-%lli",
Use snprintf instead of sprintf

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:409:
AddHeaders("Range", std::string(range_value_buffer), &p.headers);
Remove std::string constructor, it will be automatically coerced when you pass
range_value_buffer.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:439:
(*out_string_buffer)[i] = output[i];
looks like you are assuming that out_string_buffer is large enough to fit this,
probably worth adding an assert.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:447:
error = 0;
why isn't this an error?

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:589:
GetValue(output, "modifiedTime", 0, &modified_time_value,
On 2016/08/07 02:41:03, chanpatorikku wrote:
> I don't know about how practical my implementation of GetValue() is.
> 
> GetValue() is called only at the time a response with status code 200 is
there.
> 
> Response body is never cutoff in a response with status code 200.
> 
> GetValue() assumes that response body is never cutoff. When a response body
is,
> for example,
> 
> {
> 
>   “modifiedTime”: “2016-08-07T10:44:55.342Z”
> 
> }
> 
> , GetValue() never checks the case when there is not a second double quote
after
> the first double quote of the time value.
> 
> Could you please reply if the implementation is practical?

You may want to specify the expected format below, and check that the length of
the response is what you expected as well. I think that will be a sufficient
test.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:746:
AddUrlFirstQueryParameter("q", "%27" + item_id_ + "%27+in+parents", &url);
this expression ("%27" + ...) is very common, probably would be good to make a
helper function for it.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc
(right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc:235:
return;
This should be an error.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc:238:
size_t start_value_index = key_location + key.size() + 5;
Why 5?

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc:241:
size_t end_value_index = json.find("\"", start_value_index);
If GetValue is always returning a string, you may want to rename it to
GetValueString.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc:463:
Error GetListFileResponseBody(const std::string& url,
These last few functions look very similar with a lot of duplicated code. Can
they be combined?

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/t...
File
native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_googledrivefs.cc
(right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_googledrivefs.cc:132:
int start_page_token_index = url.find("pageToken", 0) + 10;
better to do something like:

const char s_page_token = "pageToken";
... url.find(s_page_token, 0) + strlen(s_page_token)

But actually, it will return -1 if not found, so this isn't great either. Still,
it's better to use a const than a magic number like this.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/t...
File
native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_googledrivefs.h
(right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_googledrivefs.h:29:
: id_(""), name_(""), contents_(""), modified_time_(""), is_dir_(true) {}
not necessary to initialize strings to "", that's the default

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/t...
File native_client_sdk/src/tests/nacl_io_test/googledrivefs_test.cc (right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/googledrivefs_test.cc:165: // open a
non-existing file in r mode.
On 2016/08/07 02:41:03, chanpatorikku wrote:
> nacl_io_demo covers only 6 different flags in file open.
> 
> Every test case of googledrivefs_test.cc covers any of the 6 flags.
> 
> Could you please reply if it's practical of the tests only covering 6
different
> flags?

Yes, this is fine.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/googledrivefs_test.cc:537: //
1300628744, though, is returned by mktime.
On 2016/08/07 02:41:03, chanpatorikku wrote:
> Test cases of lines 537-538 and lines 564-565 are verifying the values of
mtime
> calculated by mktime that uses the UTC time of modifiedTime responded from
> Google Drive API v3.
> 
> I don't know why mktime does not return a UNIX timestamp equal to the target
UTC
> time.
> 
> Could you please reply what you think is practical on resolving?

mktime is defined as generating a time in local time, not UTC.

[chanpatorikku, 2016-08-29 17:14:04]

Somehow there's a like of coding with high speed. In patch set 2, however, there
were still quite a lot of code to be commented on.  A lot simplified code has
been done in patch set 3.

There're comments of yours and codes of mine that are not very clear.  Please
review cautiously so it's not difficult to own the code when you become the code
owner.

Could you please reply about at least how long you may review patch set 3?

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/e...
File native_client_sdk/src/examples/demo/nacl_io_demo/example.dsc (right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io_demo/example.dsc:2: 'TOOLS':
['pnacl'],
On 2016/08/22 19:21:52, binji wrote:
> On 2016/08/07 02:41:02, chanpatorikku wrote:
> > Only pnacl is tested.  Could you please reply if only pnacl in the toolchain
> is
> > good?
> 
> You should leave all the others in, even if you haven't tested them. The
change
> can't land, as-is.

Done.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/e...
File native_client_sdk/src/examples/demo/nacl_io_demo/example.js (right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io_demo/example.js:1: // Copyright (c)
2016 The Chromium Authors. All rights reserved.
On 2016/08/22 19:21:52, binji wrote:
> It's not necessary to change the copyright year.

Done.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io_demo/example.js:5: var CLIENT_ID =
"TO FILL";
On 2016/08/22 19:21:52, binji wrote:
> Where is this filled in? If it is meant to be filled in by the developer
> building the example, that needs to be documented somewhere. The best place
> would be on the example page. Similarly, if the value has not been filled in,
a
> clear error message should be displayed, explaining where to fill in the
values
> (and how to get them).

This is filled in in example.js.

The Drive example in chromium/src/native_client_sdk/src/examples/demo/drive
stores client ID in manifest.json.  The client ID and the client secret are
meant to be filled in in a similar way in example.js.  

Documentation is written in index.html as:

...
      <li><i>/googledrive</i> a Google Drive mount. Before using, client ID
          and client secret, created from Google Developers Console, have to
          be filled in in example.js. When they have already been filled,
          use makefile on nacl_io_demo, and click on the sign-in button to
          expose Google Drive to nacl_io.</li>
...

If the value has not been filled in, and the “Sign in” button is clicked for
signing in to a Google account, an error message is shown as a JavaScript alert:

  if (CLIENT_ID == "TO FILL" || CLIENT_SECRET == "TO FILL") {
    alert('Error: CLIENT_ID or CLIENT_SECRET is not set.' +
          ' Please fill in CLIENT_ID and CLIENT_SECRET,' +
          ' created from Google Developers Console, in example.js.' +
          ' After that, use makefile on nacl_io_demo and rerun the' +
          ' demo.');
  }

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io_demo/example.js:32: var codeKeyIndex
= currentURL.search("code=");
On 2016/08/22 19:21:52, binji wrote:
> you can just use indexOf instead of search. search performs a regular
expression
> search.

Done.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc
(right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:33:
instance_ = atoi(instance_it->second.c_str());
On 2016/08/22 19:21:52, binji wrote:
> you don't need to get this instance from the user, it is accessible through
> ppapi_->GetInstance().

Done.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:35:
std::map<std::string, std::string>::const_iterator token_it =
On 2016/08/22 19:21:52, binji wrote:
> nit: Use StringMap_t instead of std::map<std::string, std::string>

Done.

StringMap_t is defined the same in http_fs_node.cc and filesystem.h as:

typedef std::map<std::string, std::string> StringMap_t;

Could you please reply if you would like to refactor to have 1 copy of
StringMap_t in some other CL, as it is out of scope of GoogleDriveFs?

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:66:
URLLoaderInterface* url_loader_iface = ppapi()->GetURLLoaderInterface();
On 2016/08/22 19:21:52, binji wrote:
> you could cache these interface pointers as members (see html5_fs for example)

The interface pointers in googledrivefs are not cached since the interface
pointers in html5_fs looked not to be for caching but for logging.

The code review of the CL when the interface pointers were added is:
Issue 349703003: [NaCl SDK] Add some more logging to nacl_io. (Closed)
https://codereview.chromium.org/349703003/#ps1

Some errors were logged and some were not.
Verifying if an interface pointer is NULL and showing an error on NULL pointers
seems to be a good idea, but not all interface pointers were verified if they
are NULL.

There are interface pointers that are not cached in other Filesystem.

Designs on the interface pointers can vary, depending on the situations.  There
are times when the code is good for a purpose and later on finds another purpose
that is also excellent for.  

Perhaps double checking in various perspectives is a good follow-up.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:74:
std::string parent_dir_id = "";
On 2016/08/22 19:21:52, binji wrote:
> nit: it's not necessary to initialize to "", that is the default value of
> std::string.

Done.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:137: if
(url_loader_object) {
On 2016/08/22 19:21:52, binji wrote:
> Use the ScopedResource instead (see pepper_interface.h)

Done.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:163:
static const char base_url[] = "https://www.googleapis.com/drive/v3/files";
On 2016/08/22 19:21:52, binji wrote:
> This seems to be repeated quite a bit, can you move it to a shared location?

Yes. Done.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc
(right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:99:
off_t file_buffer_size = std::max((unsigned long long int)file_size,
On 2016/08/22 19:21:53, binji wrote:
> use std::max<off_t> instead of casting the arguments

Done. std::max< 'C++ variable type' > is used in various places.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:120:
error = WriteHelper(file_buffer.c_str(), file_buffer_size);
On 2016/08/22 19:21:53, binji wrote:
> Is there no better way than reading the entire file, modifying a small
portion,
> then writing it back?

No.  Google Drive API v3 supports only file overwrite.  

Because only file overwrite is available in Google Drive API v3, and
UrlRequestInfoInterface::AppendDataToBody(..) can write data up to the max
number of uint32_t, a file written from GoogleDriveFs can be at most UINT_MAX
bytes.

For further information, a stackoverflow post confirmed that the Drive API dealt
with files as a whole and there was only file overwrite.

http://stackoverflow.com/questions/19478354/add-lines-to-a-file-without-overw...

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:184:
pChar[i] = file_buffer[i];
On 2016/08/22 19:21:53, binji wrote:
> why not read directly into buf?

buf has to be copied from a std::string object at some time of the function
calls before GoogleDriveFsNode::Read(..) returns buf.  The copying has to be
there, so in patch set 2, buf was not read directly into.

The function calls in patch set 2 were
GoogleDriveFsNode::Read(.. void* buf ..), 
GoogleDriveFsNode::ReadHelper(.. std::string* out_helper_buffer), 
and GoogleDriveFsUtil::ReadResponseBody(.. std::string* out_body_buffer).

out_helper_buffer copies from out_body_buffer, and buf copies from
out_helper_buffer.

GoogleDriveFsUtil::ReadResponseBody(..) can read a response body string with
both an unknown length and a known length.  The length can be unknown because
the response body can be the contents of an arbitrary Google Drive file that has
an arbitrary length.

GoogleDriveFsNode::ReadHelper(..) can be designed to read just a string with
known length.

To simplify the code, ReadHelper is modified to become
GoogleDriveFsNode::ReadHelper(.. char* out_buffer).

The function calls in patch set 3 becomes GoogleDriveFsNode::Read(.. void* buf
..), GoogleDriveFsNode::ReadHelper(.. char* out_helper_buffer), and
GoogleDriveFsUtil::ReadResponseBody(.. std::string* out_body_buffer).

out_helper_buffer copies from out_body_buffer, and buf is passed as
out_helper_buffer.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:402:
char_written = sprintf(range_value_buffer, "bytes=%lli-%lli",
On 2016/08/22 19:21:53, binji wrote:
> Use snprintf instead of sprintf

Done.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:409:
AddHeaders("Range", std::string(range_value_buffer), &p.headers);
On 2016/08/22 19:21:53, binji wrote:
> Remove std::string constructor, it will be automatically coerced when you pass
> range_value_buffer.

Done.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:439:
(*out_string_buffer)[i] = output[i];
On 2016/08/22 19:21:53, binji wrote:
> looks like you are assuming that out_string_buffer is large enough to fit
this,
> probably worth adding an assert.

Not adding assert here, but adding assert in various places.

The caller of ReadHelper makes sure that the buffer, whose size is end - start +
1, has enough space.  

ReadHelper(off_t start, off_t end ...) is changed to ReadHelper(uint32_t start,
uint32_t end …). The size of a Google Drive file is <= UINT_MAX when it is
written from nacl_io, so start and end are in uint32_t.

Start and end are made sure to be between 0 and UINT_MAX.

Before calling ReadHelper(..), the caller makes sure start and end are <=
UINT_MAX (KernelHandle makes sure HandleAttr.offs >= 0, and start and end >=
HandleAttr.offs),

The buffer size is therefore at most UINT_MAX.  The new, revised
ReadResponseBody(..), which ReadHelper(..) calls, returns at most UINT_MAX
number of bytes, so the buffer size does not have to be asserted.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:447:
error = 0;
On 2016/08/22 19:21:53, binji wrote:
> why isn't this an error?

Google Drive API v3, HttpFs, and Html5Fs do not show an error when the file read
range low and high has a low value >= file size, so ReadHelper(..) does not
return an error either.

When low value >= the file size, Google Drive API v3 returns an empty response
body in addition to the status code STATUSCODE_REQUESTED_RANGE_NOT_SATISFIABLE. 
When Google Drive API v3 returns an error, the response body is non empty and
shows an error description.  Since an empty response body but not an error
description is returned, it can be concluded that Google Drive API v3 does not
return an error.

Moreover, when low value >= the file size, HttpFsNode::Read(..) and
Html5FsNode::Read(..), are designed to return no error.  There are test cases in
http_fs_test.cc and html5_fs_test.cc testing the scenario.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:589:
GetValue(output, "modifiedTime", 0, &modified_time_value,
On 2016/08/22 19:21:53, binji wrote:
> On 2016/08/07 02:41:03, chanpatorikku wrote:
> > I don't know about how practical my implementation of GetValue() is.
> > 
> > GetValue() is called only at the time a response with status code 200 is
> there.
> > 
> > Response body is never cutoff in a response with status code 200.
> > 
> > GetValue() assumes that response body is never cutoff. When a response body
> is,
> > for example,
> > 
> > {
> > 
> >   “modifiedTime”: “2016-08-07T10:44:55.342Z”
> > 
> > }
> > 
> > , GetValue() never checks the case when there is not a second double quote
> after
> > the first double quote of the time value.
> > 
> > Could you please reply if the implementation is practical?
> 
> You may want to specify the expected format below, and check that the length
of
> the response is what you expected as well. I think that will be a sufficient
> test.

The comment becomes not applicable now.

GetValue(..) is redesigned to return an error when a second double quote is not
there.

GetValue(..) is renamed to GetValueStringAndValuePos(..) and becomes an usual
function that gets a key value from a key.  It returns an error when the key is
not there or the json body to look for the keys and values is not in the
expected format.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:746:
AddUrlFirstQueryParameter("q", "%27" + item_id_ + "%27+in+parents", &url);
On 2016/08/22 19:21:53, binji wrote:
> this expression ("%27" + ...) is very common, probably would be good to make a
> helper function for it.

Done.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc
(right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc:235:
return;
On 2016/08/22 19:21:53, binji wrote:
> This should be an error.

Done. The function is redesigned to return an error.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc:238:
size_t start_value_index = key_location + key.size() + 5;
On 2016/08/22 19:21:53, binji wrote:
> Why 5?

From your other comments, you already figured out the answer, but just in case,
here's the answer.

start_value_index is designed to store the index of the first character of
<value> when the expected format is “<key>”: “<value>”, and <key>.size()+5 means

the number of <key> characters + 
'”' (1st character) + 
':' (2nd character) + 
' ' (3rd character) + 
'”' (4th character) + 
first character of <value> (5th character).

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc:241:
size_t end_value_index = json.find("\"", start_value_index);
On 2016/08/22 19:21:53, binji wrote:
> If GetValue is always returning a string, you may want to rename it to
> GetValueString.

GetValue(..) is renamed to GetValueStringAndValuePos(..).

The renaming follows the design of AcquireFsAndNode(..) which also returns 2
values.

  // Find the filesystem and node for the given path, acquiring/creating it as
  // specified by the |oflags|.
  // Assumes |out_fs| and |out_node| are non-NULL.
  Error AcquireFsAndNode(const std::string& path,
                         int oflags, mode_t mflags,
                         ScopedFilesystem* out_fs,
                         ScopedNode* out_node);

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc:463:
Error GetListFileResponseBody(const std::string& url,
On 2016/08/22 19:21:53, binji wrote:
> These last few functions look very similar with a lot of duplicated code. Can
> they be combined?

Some combination has been done.

RequestDirent(..) is redesigned to combine RequestDirent(..),
RequestDirentWithNextPageToken(..) and RequestDirentHelper(..).

The new, revised RequestDirent(const std::string& optional_page_token ..) takes
an optional page token.  The design follows Tcsetattr which takes optional
actions as a flag that changes the behavior of the function.

  virtual Error Tcsetattr(int optional_actions,
                          const struct termios* termios_p);

RequestDirId(..) and RequestItemId(..) are combined to become
RequestItemIdAndItemType(std::string* out_item_id, bool* out_is_dir_type).  The
parameters are self explanatory.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/t...
File
native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_googledrivefs.cc
(right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_googledrivefs.cc:132:
int start_page_token_index = url.find("pageToken", 0) + 10;
On 2016/08/22 19:21:53, binji wrote:
> better to do something like:
> 
> const char s_page_token = "pageToken";
> ... url.find(s_page_token, 0) + strlen(s_page_token)
> 
> But actually, it will return -1 if not found, so this isn't great either.
Still,
> it's better to use a const than a magic number like this.

The function is redesigned to return a pp error if the string is not found.
When the string is found, the function does something like
url.find(s_page_token, 0) + strlen(s_page_token).

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/t...
File
native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_googledrivefs.h
(right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_googledrivefs.h:29:
: id_(""), name_(""), contents_(""), modified_time_(""), is_dir_(true) {}
On 2016/08/22 19:21:53, binji wrote:
> not necessary to initialize strings to "", that's the default

Done.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/t...
File native_client_sdk/src/tests/nacl_io_test/googledrivefs_test.cc (right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/googledrivefs_test.cc:165: // open a
non-existing file in r mode.
On 2016/08/22 19:21:53, binji wrote:
> On 2016/08/07 02:41:03, chanpatorikku wrote:
> > nacl_io_demo covers only 6 different flags in file open.
> > 
> > Every test case of googledrivefs_test.cc covers any of the 6 flags.
> > 
> > Could you please reply if it's practical of the tests only covering 6
> different
> > flags?
> 
> Yes, this is fine.

Acknowledged.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/googledrivefs_test.cc:537: //
1300628744, though, is returned by mktime.
On 2016/08/22 19:21:53, binji wrote:
> On 2016/08/07 02:41:03, chanpatorikku wrote:
> > Test cases of lines 537-538 and lines 564-565 are verifying the values of
> mtime
> > calculated by mktime that uses the UTC time of modifiedTime responded from
> > Google Drive API v3.
> > 
> > I don't know why mktime does not return a UNIX timestamp equal to the target
> UTC
> > time.
> > 
> > Could you please reply what you think is practical on resolving?
> 
> mktime is defined as generating a time in local time, not UTC.

mktime generates time in local time, but st_mtime stores a UNIX timestamp that
is an epoch time in UTC +0 time, so to generate the epoch time in UTC +0 time, a
function ConvertDateTimeToEpochTime(..) is implemented.

mktime looks to me to be buggy.

Before writing about the buggy behavior, the terms local time, UTC +0 time, and
epoch time need to be discussed.

Local time usually means the time of a specific time zone.  If the time zone is
UTC -8, the local time is the time in UTC +0 time zone – 8 hours.

The UTC +0 time I typed means the time in the UTC +0 time zone.

Epoch time is the number of seconds elapsed since 01/01/1970 00:00:00 in UTC +0
time zone.  Leap seconds are excluded in the calculation.

The time “2016-07-24T08:21:28.940Z” in the unit tests means 07/24/2016 08:21:28
in UTC +0 time zone.  Its epoch time is 1469348488, the number of seconds
between 01/01/1970 00:00:00 and 07/24/2016 08:21:28.

However, the mktime compiled from pnacl-clang++ returns 1300628744.  If
1300628744 is an epoch time, it means 03/20/2011 13:45:44.   

The mktime compiled from g++ 5.2.1 returns 1430138488.  If 1430138488 is an
epoch time, it means 04/27/2015 12:41:28.

mktime returns different values in different compilers probably using different
library files.  Could you please reply if filing a bug is practical?

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc
(right):

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:40:
GetDentsHelper helper(HashPath(Path(".")), HashPath(Path("..")));
The code handles the TODO of html5_fs_node.cc:

  // TODO(binji): Better handling of ino numbers.
  const ino_t kCurDirIno = -1;
  const ino_t kParentDirIno = -2;
  GetDentsHelper helper(kCurDirIno, kParentDirIno);

Could you please reply if you would like the TODO to be done in a different CL,
as the handling of ino numbers looks to be out of scope of GoogleDriveFs?

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc
(right):

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc:223:

Not calling 

url_loader_iface->Close(url_loader_object);

as in patch set 2. 

ScopedResource looks to be able to release resource automatically, as shown in
http_fs_node.cc and fake_resource_manager.cc. Close(..) is not called to release
resources.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc:347:
NACL_ARRAY_SIZE_UNSAFE(attribute_values),
NACL_ARRAY_SIZE_UNSAFE(arr) borrows from
native_client/src/include/nacl_macros.h.

In native_client/src/include/nacl_macros.h, the safe version of NACL_ARRAY_SIZE
is defined in a complicated way and depends on _MSC_VER and __GNUC__.

If you would like the complete macro specified in nacl_macros.h to be copied to,
for example, native_client_sdk/src/libraries/sdk_util/macros.h, please let me
know.

[chanpatorikku, 2016-08-29 18:10:39]

I can't compare patch sets as well for patch set 3 as in the other patch sets
and see some other codes and comments that are not meant to be in the patch set
until now. 

Some more comments are added for patch set 3.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/t...
File native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_util.cc (right):

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_util.cc:66: int32_t
IntegerToString(size_t integer, std::string* out_string) {
Sorry. The function was not meant to be in the patch set.

I am going to make sure when the CL is landed, the function is not there.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/t...
File native_client_sdk/src/tests/nacl_io_test/googledrivefs_test.cc (right):

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/googledrivefs_test.cc:732: // cast
signed int on sizeof(..) but not signed long long int on
Sorry. The comments were not meant to be in the patch set.

      // cast signed int on sizeof(..) but not signed long long int on
      // both operands because sizeof(buffer) <= INT_MAX.

I am going to make sure when the CL is landed, the comments are not there.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/googledrivefs_test.cc:759: // cast
signed int on sizeof(..) but not signed long long int on
Sorry. The comments were not meant to be in the patch set.

      // cast signed int on sizeof(..) but not signed long long int on
      // both operands because sizeof(buffer) <= INT_MAX.

I am going to make sure when the CL is landed, the comments are not there.

[binji, 2016-08-30 01:39:35]

another quick review; I haven't looked at all at the fake implementation yet.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc
(right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs.cc:35:
std::map<std::string, std::string>::const_iterator token_it =
On 2016/08/29 17:14:03, chanpatorikku wrote:
> On 2016/08/22 19:21:52, binji wrote:
> > nit: Use StringMap_t instead of std::map<std::string, std::string>
> 
> Done.
> 
> StringMap_t is defined the same in http_fs_node.cc and filesystem.h as:
> 
> typedef std::map<std::string, std::string> StringMap_t;
> 
> Could you please reply if you would like to refactor to have 1 copy of
> StringMap_t in some other CL, as it is out of scope of GoogleDriveFs?

It's fine to redefine it.

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/t...
File native_client_sdk/src/tests/nacl_io_test/googledrivefs_test.cc (right):

https://codereview.chromium.org/2156503002/diff/40001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/googledrivefs_test.cc:537: //
1300628744, though, is returned by mktime.
> However, the mktime compiled from pnacl-clang++ returns 1300628744.  If
> 1300628744 is an epoch time, it means 03/20/2011 13:45:44.   
> 

I'm not seeing this behavior. Using pnacl-clang, I made a test that sets a tm
struct to these values and calls mktime, and I get 1469348488.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/e...
File native_client_sdk/src/examples/demo/nacl_io_demo/nacl_io_demo.c (right):

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io_demo/nacl_io_demo.c:394: if
(char_written < 0 || char_written >= (signed int) sizeof(buffer)) {
this new check is not necessary, snprintf will truncate the output if it won't
fit in the buffer.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc
(right):

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:40:
GetDentsHelper helper(HashPath(Path(".")), HashPath(Path("..")));
On 2016/08/29 17:14:04, chanpatorikku wrote:
> The code handles the TODO of html5_fs_node.cc:
> 
>   // TODO(binji): Better handling of ino numbers.
>   const ino_t kCurDirIno = -1;
>   const ino_t kParentDirIno = -2;
>   GetDentsHelper helper(kCurDirIno, kParentDirIno);
> 
> Could you please reply if you would like the TODO to be done in a different
CL,
> as the handling of ino numbers looks to be out of scope of GoogleDriveFs?

This is fine.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:104:
assert(attr.offs + count <= UINT_MAX);
I don't think an assertion is correct here; the user can pass a count that is >
UINT_MAX.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:121:
strncpy(&file_buffer[0] + attr.offs, (char*)buf, count);
don't use C-style casts

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:151:
assert(length <= UINT_MAX);
I don't think an assertion is correct here; the user could pass a value to
FTruncate larger than UINT_MAX.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:188:
return 0;
shouldn't this return an error?

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:199:
ReadHelper(attr.offs, read_end, (char*)buf, (uint32_t*)out_bytes);
don't use C-style casts

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:199:
ReadHelper(attr.offs, read_end, (char*)buf, (uint32_t*)out_bytes);
don't cast int* to uint32_t*, instead use a local uint32_t and assign out_bytes
after.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:349:
char* out_buffer,
if you make the other changes below, you can just treat this as a raw buffer,
and make this type void*

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:351:
out_buffer[0] = '\0';
This buffer is not a string, just raw data.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:372:
char_written >= (signed int)sizeof(range_value_buffer)) {
no need to check this, it's impossible for the length to be greater than 1024

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:396:
strncpy(out_buffer, &output[0], output.size());
use memcpy, this data could include \0

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc
(right):

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc:257:
std::string string_buffer(1024, '\0');
Use std::vector instead of std::string for raw data.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc:347:
NACL_ARRAY_SIZE_UNSAFE(attribute_values),
On 2016/08/29 17:14:04, chanpatorikku wrote:
> NACL_ARRAY_SIZE_UNSAFE(arr) borrows from
> native_client/src/include/nacl_macros.h.
> 
> In native_client/src/include/nacl_macros.h, the safe version of
NACL_ARRAY_SIZE
> is defined in a complicated way and depends on _MSC_VER and __GNUC__.
> 
> If you would like the complete macro specified in nacl_macros.h to be copied
to,
> for example, native_client_sdk/src/libraries/sdk_util/macros.h, please let me
> know.

This definition is fine. Also, don't use the NACL_ prefix, it would be better to
use NACL_IO_ or move it into the .cc file if possible and remove the prefix.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/t...
File native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_util.cc (right):

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_util.cc:66: int32_t
IntegerToString(size_t integer, std::string* out_string) {
On 2016/08/29 18:10:39, chanpatorikku wrote:
> Sorry. The function was not meant to be in the patch set.
> 
> I am going to make sure when the CL is landed, the function is not there.

You can just upload a new patch with these removed.

[chanpatorikku, 2016-08-30 14:16:45]

Your comments help make the code more correct. :)

Please add comments on other codes not reviewed yet if there are any so the next
patch contains all changes.

[chanpatorikku, 2016-09-06 04:09:52]

Patchset #4 (id:80001) has been deleted

[chanpatorikku, 2016-09-06 14:49:38]

Increment improvement on patch set 4.  Thank you for your review of patch set 3.

There were some functions that I could not fully understand how to call
(snprintf returns < 0 on encoding error, and the encoding error means encoding
from multibytes, not from encoding string formats like “%i%s”; strncpy copies
characters but not '\0' even though '\0' is also a character.), so I went
through each function and checked how to call exactly.

The observation about the # of bits in off_t, size_t, etc was not seen until the
last patch set.  I did some investigations on the types and rewrote the related
code.

There're some boundary checks that were missed although the boundary checks were
observed.  Most boundary checks have been gone through if they were observed,
though.  Anyways, I redid the missed boundary checks.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/e...
File native_client_sdk/src/examples/demo/nacl_io_demo/nacl_io_demo.c (right):

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io_demo/nacl_io_demo.c:394: if
(char_written < 0 || char_written >= (signed int) sizeof(buffer)) {
On 2016/08/30 01:39:34, binji wrote:
> this new check is not necessary, snprintf will truncate the output if it won't
> fit in the buffer.

Done.  Another very good comment of yours.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc
(right):

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:40:
GetDentsHelper helper(HashPath(Path(".")), HashPath(Path("..")));
On 2016/08/30 01:39:35, binji wrote:
> On 2016/08/29 17:14:04, chanpatorikku wrote:
> > The code handles the TODO of html5_fs_node.cc:
> > 
> >   // TODO(binji): Better handling of ino numbers.
> >   const ino_t kCurDirIno = -1;
> >   const ino_t kParentDirIno = -2;
> >   GetDentsHelper helper(kCurDirIno, kParentDirIno);
> > 
> > Could you please reply if you would like the TODO to be done in a different
> CL,
> > as the handling of ino numbers looks to be out of scope of GoogleDriveFs?
> 
> This is fine.

Acknowledged.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:104:
assert(attr.offs + count <= UINT_MAX);
On 2016/08/30 01:39:35, binji wrote:
> I don't think an assertion is correct here; the user can pass a count that is
>
> UINT_MAX.

The boundary values of the variables have been rechecked.
Code has been rewritten.

The bits of the types 

off_t (which can be 32 bits, 64 bits, or some other values)
size_t (which can be 32 bits, 64 bits, or some other values)
int (which can be 16 bits, 32 bits, 64 bits, or some other values)

are unknown.

The bits of the types are assumed below, and code working with any combination
of the assumed bits has been rewritten.

32 bits and 64 bits of off_t,
32 bits and 64 bits of size_t, and
32 bits and 64 bits of int.

size_t is assumed to be >= 32 bits because SIZE_MAX seems to be defined in
stdint.h as

chromium/src/out/pepper_55/toolchain/linux_pnacl/lib/clang/3.7.0/include/stdint.h:
#define    SIZE_MAX __UINTN_MAX(__SIZE_WIDTH__)

and __SIZE_WIDTH__ is either 32 or 64.

int is assumed to be >= 32 bits because LP32 data models seem not very usual to
be used.

Could you please reply your opinion about if the coverage is good enough?



Your comment was uncovering a buffer overrun in udp_node.cc.

chromium/src/native_client_sdk/src/libraries/nacl_io/socket/udp_node.cc:

    // len and packet->len() are in size_t
    // Assume int is 32 bit, len = 1, and packet->len() = UINT_MAX,
    // capped_len becomes -1.  capped_len as size_t becomes SIZE_MAX.
    int capped_len = static_cast<int32_t>(std::min<int>(len, packet->len()));
    memcpy(buf, packet->buffer(), capped_len);

If you would like to fix the buffer overrun, could you please reply if you would
like to fix in another CL as the fix is out of scope of GoogleDriveFs?

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:121:
strncpy(&file_buffer[0] + attr.offs, (char*)buf, count);
On 2016/08/30 01:39:34, binji wrote:
> don't use C-style casts

Done.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:151:
assert(length <= UINT_MAX);
On 2016/08/30 01:39:34, binji wrote:
> I don't think an assertion is correct here; the user could pass a value to
> FTruncate larger than UINT_MAX.

The reply of this comment's going to be the same as that of the other comment

"I don't think an assertion is correct here; the user can pass a count that is >
UINT_MAX."

Done for rewriting the code.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:188:
return 0;
On 2016/08/30 01:39:35, binji wrote:
> shouldn't this return an error?

I answered a similar question in patch set 2.
The case is the scenario when the file read range low >= file size.
I'm doing the same as I was doing in patch set 3.

Google Drive API v3, HttpFs, and Html5Fs do not show an error when the file read
range low and high has a low value >= file size, so ReadHelper(..) does not
return an error either.

When low value >= the file size, Google Drive API v3 returns an empty response
body in addition to the status code STATUSCODE_REQUESTED_RANGE_NOT_SATISFIABLE. 
When Google Drive API v3 returns an error, the response body is non empty and
shows an error description.  Since an empty response body but not an error
description is returned, it can be concluded that Google Drive API v3 does not
return an error.

Moreover, when low value >= the file size, HttpFsNode::Read(..) and
Html5FsNode::Read(..), are designed to return no error.  There are test cases in
http_fs_test.cc and html5_fs_test.cc testing the scenario.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:199:
ReadHelper(attr.offs, read_end, (char*)buf, (uint32_t*)out_bytes);
On 2016/08/30 01:39:34, binji wrote:
> don't use C-style casts

Done.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:199:
ReadHelper(attr.offs, read_end, (char*)buf, (uint32_t*)out_bytes);
On 2016/08/30 01:39:34, binji wrote:
> don't use C-style casts

Done.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:349:
char* out_buffer,
On 2016/08/30 01:39:34, binji wrote:
> if you make the other changes below, you can just treat this as a raw buffer,
> and make this type void*

Done.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:351:
out_buffer[0] = '\0';
On 2016/08/30 01:39:35, binji wrote:
> This buffer is not a string, just raw data.

Done.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:372:
char_written >= (signed int)sizeof(range_value_buffer)) {
On 2016/08/30 01:39:34, binji wrote:
> no need to check this, it's impossible for the length to be greater than 1024

Done.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:396:
strncpy(out_buffer, &output[0], output.size());
On 2016/08/30 01:39:34, binji wrote:
> use memcpy, this data could include \0

Done.  Thank you so much for the catch. 

Other filesystems contain unit tests for simple strings with English words,
spaces, and trailing zeros, and they do not have unit tests for zeros in the
middle of a string.  Zeros in the middle of a string are probably too rare as a
use case, and unit tests for them are not added in GoogleDriveFs.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc
(right):

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc:257:
std::string string_buffer(1024, '\0');
On 2016/08/30 01:39:35, binji wrote:
> Use std::vector instead of std::string for raw data.

std::string can handle raw data.

There're cases when the type uint8_t is better than the type char for storing an
element of raw data, but for GoogleDriveFs such cases do not arise.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc:347:
NACL_ARRAY_SIZE_UNSAFE(attribute_values),
On 2016/08/30 01:39:35, binji wrote:
> On 2016/08/29 17:14:04, chanpatorikku wrote:
> > NACL_ARRAY_SIZE_UNSAFE(arr) borrows from
> > native_client/src/include/nacl_macros.h.
> > 
> > In native_client/src/include/nacl_macros.h, the safe version of
> NACL_ARRAY_SIZE
> > is defined in a complicated way and depends on _MSC_VER and __GNUC__.
> > 
> > If you would like the complete macro specified in nacl_macros.h to be copied
> to,
> > for example, native_client_sdk/src/libraries/sdk_util/macros.h, please let
me
> > know.
> 
> This definition is fine. Also, don't use the NACL_ prefix, it would be better
to
> use NACL_IO_ or move it into the .cc file if possible and remove the prefix.

Done.

Moved the macro into the .cc file if possible and removed the prefix.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/t...
File native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_util.cc (right):

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_util.cc:66: int32_t
IntegerToString(size_t integer, std::string* out_string) {
On 2016/08/30 01:39:35, binji wrote:
> On 2016/08/29 18:10:39, chanpatorikku wrote:
> > Sorry. The function was not meant to be in the patch set.
> > 
> > I am going to make sure when the CL is landed, the function is not there.
> 
> You can just upload a new patch with these removed.

Done.  Some more confidence about not having code intended to be deleted on this
new patch upload, as there's a pre-check on the diff at the online code review.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc
(right):

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:197:
if (count == 0) {
Return 0 when count == 0 so the later code:

  Error error = ReadHelper(attr.offs, attr.offs + bytes_to_read - 1, buf,
                           &read_helper_out_bytes);

is not going to become ReadHelper(0, max of off64_t, ...) and overflows int32_t,
the type of the 2nd argument of ReadHelper, when attr.offs = 0 and off_t is 64
bit.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
File native_client_sdk/src/tests/nacl_io_test/googledrivefs_test.cc (right):

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/tests/nacl_io_test/googledrivefs_test.cc:35:
GoogleDriveFsForTesting(const StringMap_t& string_map,
Changed

StringMap_t& 

to

const StringMap_t& 

for the style guide.

https://google.github.io/styleguide/cppguide.html#Reference_Arguments

[binji, 2016-09-20 01:22:05]

More reviews.

I suggested a few cases where separating this one large change into smaller
simpler CLs would speed things up. I know this CL has taken a while, so perhaps
getting some of the more straightforward stuff landed will make the iteration
smoother here.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc
(right):

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc:257:
std::string string_buffer(1024, '\0');
On 2016/09/06 14:49:38, chanpatorikku wrote:
> On 2016/08/30 01:39:35, binji wrote:
> > Use std::vector instead of std::string for raw data.
> 
> std::string can handle raw data.
> 
> There're cases when the type uint8_t is better than the type char for storing
an
> element of raw data, but for GoogleDriveFs such cases do not arise.

Yes, you can use std::string; it's not guaranteed to be contiguous, but in
practice it is. But why? std::string is designed to handle text, but it seems
you are using it to read and manipulate bytes. Using std::vector better conveys
the intent.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc
(right):

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:104:
// a Google Drive file has a max size of max of int32_t by design
This comment doesn't make much sense to me. Really all you need to say here is
that Google Drive has a maximum file size of INT32_MAX (i.e.
std::numeric_limits<int32_t>::max()).

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:123:
int32_t read_helper_out_bytes;
this isn't used? Is it not possible for ReadHelper to return fewer bytes without
an error?

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:125:
ReadHelper(0, file_size - 1, &file_buffer[0], &read_helper_out_bytes);
what if the file size changes between the call to GetSize above and the call to
ReadHelper?

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:125:
ReadHelper(0, file_size - 1, &file_buffer[0], &read_helper_out_bytes);
This looks a bit strange since the end is inclusive, not exclusive. Does the
http mount do the same thing? If not, I'd say it's clearer to make the end
exclusive instead.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:131:
memcpy(&file_buffer[0] + attr.offs, buf, bytes_to_write);
Can you add an assertion before this memcpy that: attr.offs + bytes_to_write <=
file_buffer_size

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:282:
*out_size = atoi(size_value.c_str());
you should validate this string. Try using strtoul instead.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:376:
memset(out_buffer, 0, 1);
why?

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:522:
AddHeaders("Content-type", "multipart/related; boundary=" + BOUNDARY_VALUE,
I think a nicer API would be:
p.AddHeaders(...);

same for AddBody.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.h
(right):

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.h:17:
static const char FOLDER_MIME_TYPE[] = "application/vnd.google-apps.folder";
All of these values and functions has relatively generic names for being
GoogleDriveFS-specific. I'd suggest moving it into a class, or wrapping it in a
namespace.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.h:17:
static const char FOLDER_MIME_TYPE[] = "application/vnd.google-apps.folder";
you shouldn't define static variables in a header file; they'll be duplicated in
every translation unit that includes the header. Instead do this:

something.h:
extern const char DRIVE_URL[];

something.cc
const char DRIVE_URL[] = "...";

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.h:35:
int ExtractYearFromRFC3339(const std::string& date_time);
are these ever used independently from ConvertDateTimeToEpochTime? If not, I'd
suggest not exposing these functions as part of the API.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.h:49:
void AddUrlPath(const std::string& path, std::string* out_url);
As mentioned before, a nicer API would be to make RequestUrlParams a class and
have these functions use it directly (without having to pass in the URL as an
out param).

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.h:77:
Error RequestItemIdAndItemType(const std::string& parent_dir_id,
why aren't these functions on GoogleFilesystemFS? Having them pass in a
Filesystem* is confusing as it won't work with any other filesystem.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
File native_client_sdk/src/libraries/nacl_io/hash.h (right):

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/hash.h:13: ino_t HashPathSegment(ino_t
hash, const char* str, size_t len);
this is a good change to move into another CL

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
File native_client_sdk/src/libraries/nacl_io/pepper/all_interfaces.h (right):

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/pepper/all_interfaces.h:169:
METHOD2(URLLoaderInterface, int32_t, FinishStreamingToFile, PP_Resource,
adding this new functionality could also be separated into its own CL

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
File
native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_file_io_interface.h
(right):

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_file_io_interface.h:5:
#ifndef LIBRARIES_NACL_IO_TEST_FAKE_FILE_IO_INTERFACE_H_
this refactoring would be a good change to move into another CL

[chanpatorikku, 2016-09-21 15:15:34]

Patchset #5 (id:120001) has been deleted

[chanpatorikku, 2016-09-21 16:41:10]

Work still in question.

I explained the case combinations that could not be easily seen from the code. 
Time and the code changes were also discussed.

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc
(right):

https://codereview.chromium.org/2156503002/diff/60001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.cc:257:
std::string string_buffer(1024, '\0');
On 2016/09/20 01:22:04, binji wrote:
> On 2016/09/06 14:49:38, chanpatorikku wrote:
> > On 2016/08/30 01:39:35, binji wrote:
> > > Use std::vector instead of std::string for raw data.
> > 
> > std::string can handle raw data.
> > 
> > There're cases when the type uint8_t is better than the type char for
storing
> an
> > element of raw data, but for GoogleDriveFs such cases do not arise.
> 
> Yes, you can use std::string; it's not guaranteed to be contiguous, but in
> practice it is. But why? std::string is designed to handle text, but it seems
> you are using it to read and manipulate bytes. Using std::vector better
conveys
> the intent.

Code not changing.

The reason is that the buffer argument of PPB_FileIO::Read is char*.

int32_t(* PPB_FileIO::Read)(PP_Resource file_io, int64_t offset, char *buffer,
int32_t bytes_to_read, struct PP_CompletionCallback callback)

std::string can be rewritten as std::vector<char>, but std::vector<char> and
std::string are very similar.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc
(right):

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:104:
// a Google Drive file has a max size of max of int32_t by design
On 2016/09/20 01:22:05, binji wrote:
> This comment doesn't make much sense to me. Really all you need to say here is
> that Google Drive has a maximum file size of INT32_MAX (i.e.
> std::numeric_limits<int32_t>::max()).

Quick fix on the comments.

Less time is going to be spent on going through the comments for the code
readers.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:123:
int32_t read_helper_out_bytes;
On 2016/09/20 01:22:04, binji wrote:
> this isn't used? Is it not possible for ReadHelper to return fewer bytes
without
> an error?

This isn't used here, but it is possible for ReadHelper to return fewer bytes
without an error.

GoogleDriveFsNode::Read(..) calls ReadHelper(attr.offs, attr.offs +
bytes_to_read - 1, buf, &read_helper_out_bytes).
When attr.offs == 6 && file_size == 4 && count == 2, bytes_to_read = 2.
ReadHelper(..) is going to return the status code 416, and read_helper_out_bytes
= 0.

There are many things to look to have a correct function.

The various variables to view are:

# of bits of off_t (32, 64), 
# of bits of size_t (32, 64), 
# of bits of int (32, 64), 
count range (0, 0 < count <= INT32_MAX, count > INT32_MAX), 
file_size range (0, 0 < file_size <= INT32_MAX), and 
offs range (0, 0 < offs <= INT32_MAX - 1, offs >= INT32_MAX)

The number of cases in the truth table is going to be 2 * 2 * 2 * 3 * 2 * 3,
about the number of lines of the function.

Moreover, there're two other cases to check: offs + count <= file_size, offs +
count > file_size.

I can send you the truth table and the checked result, but sending you hundreds
of more lines for all the functions for review looks to be giving you even more
things to review.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:125:
ReadHelper(0, file_size - 1, &file_buffer[0], &read_helper_out_bytes);
On 2016/09/20 01:22:04, binji wrote:
> This looks a bit strange since the end is inclusive, not exclusive. Does the
> http mount do the same thing? If not, I'd say it's clearer to make the end
> exclusive instead.

No, http mount does not do the same thing.

The end was made exclusive.

ReadHelper(..) is called in GoogleDriveFsNode::Write(..),
GoogleDriveFsNode::Read(..) and GoogleDriveFsNode::FTruncate(..).  Four
functions to be checked for the change.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:125:
ReadHelper(0, file_size - 1, &file_buffer[0], &read_helper_out_bytes);
On 2016/09/20 01:22:05, binji wrote:
> what if the file size changes between the call to GetSize above and the call
to
> ReadHelper?

GoogleDriveFs is not going to correctly work.

There is not a Google Drive API v3 call for locking.

Please let me know if documentations should be written for GoogleDriveFs
assuming the related Google Drive account is not accessed by others.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:131:
memcpy(&file_buffer[0] + attr.offs, buf, bytes_to_write);
On 2016/09/20 01:22:04, binji wrote:
> Can you add an assertion before this memcpy that: attr.offs + bytes_to_write
<=
> file_buffer_size

Yes.  An assertion, however, does not have to be added.

There're codes in nacl_io/socket/tcp_node.cc that does not add an assertion:

    int copy_bytes = std::min(value_len, *len);
    memcpy(optval, &value, copy_bytes);

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:282:
*out_size = atoi(size_value.c_str());
On 2016/09/20 01:22:04, binji wrote:
> you should validate this string. Try using strtoul instead.

strtoul(..) does not always validate the input correctly.

  std::string str = "3d2";

  unsigned long ul = strtoul(str.c_str(), NULL, 10);

  std::cout << ul << std::endl;  // 3 is printed.

Google Drive API v3 response is assumed to stay constant.
If Google Drive API v3 response does not stay constant, the interpretation of
the response does not stay constant, either.  GoogleDriveFs code has to be
modified for the changing response.
size_value is always between 0 and INT32_MAX when Google Drive API v3 response
is constant.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:376:
memset(out_buffer, 0, 1);
On 2016/09/20 01:22:05, binji wrote:
> why?

The out variables of many functions in the nacl_io directory are set to the
default output values in the first lines of the functions.

out_buffer default output value is '\0', so the code memset(out_buffer, 0, 1);
is there.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:522:
AddHeaders("Content-type", "multipart/related; boundary=" + BOUNDARY_VALUE,
On 2016/09/20 01:22:04, binji wrote:
> I think a nicer API would be:
> p.AddHeaders(...);
> 
> same for AddBody.

AddHeaders, AddBody, RequestUrlParams, and so on are similar to the Drive
example's design of sending requests to Google Drive API (Drive example is using
v2 but GoogleDriveFs is using v3).
Codes can be understood and maintained in the current design.

Code changes increase review time, not just coding time.

Would you still like to have a nicer API?

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.h
(right):

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.h:17:
static const char FOLDER_MIME_TYPE[] = "application/vnd.google-apps.folder";
On 2016/09/20 01:22:05, binji wrote:
> you shouldn't define static variables in a header file; they'll be duplicated
in
> every translation unit that includes the header. Instead do this:
> 
> something.h:
> extern const char DRIVE_URL[];
> 
> something.cc
> const char DRIVE_URL[] = "...";

~250 bytes are going to be duplicated.  
Code changes may have to be there for the hundreds of bytes, so the static
variables are changed to extern variables.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.h:17:
static const char FOLDER_MIME_TYPE[] = "application/vnd.google-apps.folder";
On 2016/09/20 01:22:05, binji wrote:
> All of these values and functions has relatively generic names for being
> GoogleDriveFS-specific. I'd suggest moving it into a class, or wrapping it in
a
> namespace.

There're variables specific to a class in other files in the nacl_io directory.

socket/socket_node.h:

static const size_t MAX_SOCK_TRANSFER = 65536;

Would you still like to change the code?

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.h:35:
int ExtractYearFromRFC3339(const std::string& date_time);
On 2016/09/20 01:22:05, binji wrote:
> are these ever used independently from ConvertDateTimeToEpochTime? If not, I'd
> suggest not exposing these functions as part of the API.

No, these are not ever used independently from ConvertDateTimeToEpochTime.
Further, ConvertDateTimeToEpochTime is only used in
GoogleDriveFsNode::GetModifiedTime.

When only one source file uses a function, some of the functions are written in
an unnamed namespace.
When more than one source file uses the function, however, the functions
probably are refactored to shared code.
The functions can be exposed, so exposing them rather than not exposing them and
sharing them later seems to be another idea.

Moreover, review time is increased when there're more code changes.

Would you still like not to expose the functions?

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.h:49:
void AddUrlPath(const std::string& path, std::string* out_url);
On 2016/09/20 01:22:05, binji wrote:
> As mentioned before, a nicer API would be to make RequestUrlParams a class and
> have these functions use it directly (without having to pass in the URL as an
> out param).

The reply of this message was done for another message of the same aim.
Please look at the other reply.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_util.h:77:
Error RequestItemIdAndItemType(const std::string& parent_dir_id,
On 2016/09/20 01:22:05, binji wrote:
> why aren't these functions on GoogleFilesystemFS? Having them pass in a
> Filesystem* is confusing as it won't work with any other filesystem.

If what you mean is that why aren't these functions defined as static function
on a class, the reason is that the calls of static functions of a class and the
calls of a global function in a certain namespace are made in the same way.

Filesystem* changed to GoogleDriveFs* since Filesystem* is confusing.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
File native_client_sdk/src/libraries/nacl_io/hash.h (right):

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/hash.h:13: ino_t HashPathSegment(ino_t
hash, const char* str, size_t len);
On 2016/09/20 01:22:05, binji wrote:
> this is a good change to move into another CL

Acknowledged.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
File native_client_sdk/src/libraries/nacl_io/pepper/all_interfaces.h (right):

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/pepper/all_interfaces.h:169:
METHOD2(URLLoaderInterface, int32_t, FinishStreamingToFile, PP_Resource,
On 2016/09/20 01:22:05, binji wrote:
> adding this new functionality could also be separated into its own CL

Acknowledged.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
File
native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_file_io_interface.h
(right):

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_file_io_interface.h:5:
#ifndef LIBRARIES_NACL_IO_TEST_FAKE_FILE_IO_INTERFACE_H_
On 2016/09/20 01:22:05, binji wrote:
> this refactoring would be a good change to move into another CL

Acknowledged.

https://codereview.chromium.org/2156503002/diff/140001/native_client_sdk/src/...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc
(right):

https://codereview.chromium.org/2156503002/diff/140001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:193:
if (attr.offs >= std::numeric_limits<int32_t>::max()) {
In patch set 2 and patch set 3, I wrote:
“Google Drive API v3, HttpFs, and Html5Fs do not show an error when the file
read range low and high has a low value >= file size, so ReadHelper(..) does not
return an error either. 
When low value >= the file size, Google Drive API v3 returns an empty response
body in addition to the status code STATUSCODE_REQUESTED_RANGE_NOT_SATISFIABLE. 

...“

Google Drive API v3 returns an error message for the status code
STATUSCODE_REQUESTED_RANGE_NOT_SATISFIABLE. I just found the behavior out. My
apology. HttpFs and Html5Fs were being checked before typing that they did not
show an error, but somehow for Google Drive API v3, the behavior was not
checked.
GoogleDriveFs follows the design of HttpFs and Html5Fs. And there's not any code
change.

https://codereview.chromium.org/2156503002/diff/140001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:383:
end - 1);
bytes=%i-%i

can become

bytes=0--1

when start = 0 and end = 0.

Google Drive API v3 can support the above case, although normally the low and
the high of bytes=<low>-<high> are non-negative numbers.

ReadHelper(..) is following Google Drive API v3 to read all the file contents
when low > high.
When low > high, start == end, and start == end + 1.
LoadUrl(..) gets all the file contents, but ReadResponseBody(.., end - start,
..) does not read any file contents and returns a 0-byte output to be set to
out_buffer.
May be good to check start == end in the beginning of the function for
performance reasons.

Please let me know if you would like a comment or a portion of code

if (start == end) {
  return 0;
}

in the beginning of the function for clarity and/or performance reasons.

https://codereview.chromium.org/2156503002/diff/140001/native_client_sdk/src/...
File
native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_googledrivefs.cc
(right):

https://codereview.chromium.org/2156503002/diff/140001/native_client_sdk/src/...
native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_googledrivefs.cc:671:
if (lo > hi) {
Found the behavior of Google Drive API v3 returns STATUSCODE_OK and returns the
entire file contents when low > high and low >= file size.  Although the case is
a rare use case, my apology for not catching the case.  Probably it's too fast
in patch set 2. Anyways, I compared the behavior of Google Drive API v3 and
FakeGoogleDriveServer on all the requests and rechecked that they behaved the
same.

[binji, 2016-09-26 19:11:12]

I understand that you are hesitant to do additional work because it will
increase the time for review. But I am finding it more and more difficult to
review this CL because there are many interacting parts, all combined together.
That is why I suggested breaking this CL into simpler CLs. I apologize that I
didn't ask you to do this earlier, it would have saved us both a lot of time.

I'm also finding it difficult to review because we are often disagreeing on the
direction the CL should go. You have often advocated for keeping the code a
certain way, because there are other examples of the behavior in nacl_io. That
is a reasonable choice to make without any other information, but when I am
reviewing the code, I am making suggestions having read the code as it exists.
Consistency with the file is more important than consistency with the other
files, IMO.

And now, there is concern that further changes may slow down the review process
even more. I understand that concern, but I will mention that the reason that I
am making some of these suggestions is because I am uncomfortable landing this
change as-is.

So here is my suggestion: let's take a break from this CL and try to land some
of the simpler refactorings that I mentioned before. Then let's try to work
through the GoogleDriveFS parts in smaller chunks. For example, it would be
great to have an initial CL that just wires up the basic class structures, but
doesn't do anything. Then we can add in the additional functionality
piece-by-piece, and the review process can be much simpler. For a change of this
size, it would be best for us to work together beforehand, discussing the best
process forward before writing any code. It may be a bit late for that now, but
I believe we can incrementally come to a solution that we're both happy with,
that also doesn't require you to throw away the code you've already written.

One of the reasons that I have not been quick to review is that reviewing this
CL takes a very long time. I often will have to read through the code again and
again, trying to see what was different from the last time. The last time I
reviewed the code it took me a few hours. That is way too long for me to be
spending on something that is not my primary work. I'm sure that if we can work
through the CLs in smaller chunks, I can review them more quickly, so the
turnaround time for you won't be as long.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
File native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc
(right):

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:131:
memcpy(&file_buffer[0] + attr.offs, buf, bytes_to_write);
On 2016/09/21 16:41:09, chanpatorikku wrote:
> On 2016/09/20 01:22:04, binji wrote:
> > Can you add an assertion before this memcpy that: attr.offs + bytes_to_write
> <=
> > file_buffer_size
> 
> Yes.  An assertion, however, does not have to be added.
> 
> There're codes in nacl_io/socket/tcp_node.cc that does not add an assertion:
> 
>     int copy_bytes = std::min(value_len, *len);
>     memcpy(optval, &value, copy_bytes);

Yes, but in this case it was not as clear to me that this was true.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:282:
*out_size = atoi(size_value.c_str());
On 2016/09/21 16:41:09, chanpatorikku wrote:
> On 2016/09/20 01:22:04, binji wrote:
> > you should validate this string. Try using strtoul instead.
> 
> strtoul(..) does not always validate the input correctly.
> 
>   std::string str = "3d2";
> 
>   unsigned long ul = strtoul(str.c_str(), NULL, 10);
> 
>   std::cout << ul << std::endl;  // 3 is printed.
> 
> Google Drive API v3 response is assumed to stay constant.
> If Google Drive API v3 response does not stay constant, the interpretation of
> the response does not stay constant, either.  GoogleDriveFs code has to be
> modified for the changing response.
> size_value is always between 0 and INT32_MAX when Google Drive API v3 response
> is constant.

No, strtoul doesn't validate but you can tell where it stopped parsing the int,
and you can check whether a value is returned out of range. atoi doesn't allow
for this.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:376:
memset(out_buffer, 0, 1);
On 2016/09/21 16:41:09, chanpatorikku wrote:
> On 2016/09/20 01:22:05, binji wrote:
> > why?
> 
> The out variables of many functions in the nacl_io directory are set to the
> default output values in the first lines of the functions.
> 
> out_buffer default output value is '\0', so the code memset(out_buffer, 0, 1);
> is there.

OK, but it seems strange to null-terminate out_buffer but then set out_bytes to
0. Also, out_buffer is void*, so it looks odd to null-terminate it.

https://codereview.chromium.org/2156503002/diff/100001/native_client_sdk/src/...
native_client_sdk/src/libraries/nacl_io/googledrivefs/googledrivefs_node.cc:522:
AddHeaders("Content-type", "multipart/related; boundary=" + BOUNDARY_VALUE,
On 2016/09/21 16:41:09, chanpatorikku wrote:
> On 2016/09/20 01:22:04, binji wrote:
> > I think a nicer API would be:
> > p.AddHeaders(...);
> > 
> > same for AddBody.
> 
> AddHeaders, AddBody, RequestUrlParams, and so on are similar to the Drive
> example's design of sending requests to Google Drive API (Drive example is
using
> v2 but GoogleDriveFs is using v3).
> Codes can be understood and maintained in the current design.
> 
> Code changes increase review time, not just coding time.
> 
> Would you still like to have a nicer API?

No, this isn't necessary, just mentioned it because I noticed it.

[chanpatorikku, 2017-06-15 13:08:06]

Make the review to become Closed.

PNaCl is on a path to deprecation, and GoogleDriveFs is compiled from PNaCl, so
comments are added to the implementation code to inform that GoogleDriveFs is
not going to be further developed.

Comments are added from the review below:
https://codereview.chromium.org/2937083003/