Prototype of Files service.

Prototype of Files service.

Defines two important interfaces, Directory and File. Provides a basic,
single-threaded implementation of the latter (with a few omissions), an
extremely incomplete implementation of the former (more complete
implementation to follow soon, with tests), and a root Files
interface/application.

Limitations:
* Directory isn't really implemented.
* Makes no security guarantees -- i.e., definitely insecure (doesn't
  prevent path traversal "out of" a "file system").
* Not implemented: file streaming (read/write), file mapping, file
  re-opening.
* Various other flags not implemented (e.g., recursive delete).
* Theoretical implementation: many things as yet totally untested.
* Single-threaded. Should be easy enough to move to a thread pool (but
  note that operations still need to be sequenced within each
  "object"/message pipe).
* Still not specified (but definitely desired): directory streaming.
* Still need more error codes.
* Still needs more comments and documentation.

Changes over previous iterations:
* Temporarily removed most of (totally untested) Directory
  implementation.
* "file manager" -> "files".
* Introduced a Timespec struct (and nanosecond resolution).
* Turned persistent, shared "user" directory into a "debug" directory.
* Various other requested features added/changes made.

Open questions:
* There's some duplication between Directory and File (e.g., they both
  have Stat()). Perhaps it'd be more POSIX-y to combine them?
* Semantics for "chroot"-type operations and "re-open" operations.
* (Lots of thinking with respect to security in general.)

R=qsr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/2a394496d9f098564e3a52cc62c937801dd1b1f0

[abarth-chromium, 2015-02-06 04:38:34]

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

[abarth-chromium, 2015-02-06 04:38:35]

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/fi...
File services/file_manager/file.mojom (right):

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/fi...
services/file_manager/file.mojom:43: => (Error error);
I imagine we'll want to be able to pass Files to other apps with the idea that
they'll be limited in their access to that file.  Going from "read-only" to
"read-write" seems a bit scary in that situation.

[Aaron Boodman, 2015-02-06 04:41:57]

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

[Aaron Boodman, 2015-02-06 04:41:58]

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/di...
File services/file_manager/directory.mojom (right):

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/di...
services/file_manager/directory.mojom:29: // TODO(vtl): directory "streaming"?
Yeah, seems like for large directories, returning everything as an array would
not be great.

[jamesr, 2015-02-06 05:15:43]

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/di...
File services/file_manager/directory.mojom (right):

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/di...
services/file_manager/directory.mojom:16: Read() => (Error error,
array<FileInformation>? directory_contents);
in types.mojom FileInformation appears to only have size and FileTimes.  did you
mean to return array of |DirectoryEntry|s here?

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/di...
services/file_manager/directory.mojom:17: Change(string path) => (Error error);
what does 'Change' do? does it modify what this |Directory| object points to?
how's it different from OpenDirectory().. ?

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/di...
services/file_manager/directory.mojom:21: OpenFile(string path, File& file,
uint32 access_flags, uint32 open_flags)
if you want to model accessing existing filesystems then |string| as a type
won't work since many filesystems support components that aren't in unicode.  if
we want to say that this is only for manipulating filesystems created with this
API or only for "sane" filesystems then we can stick with string

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/di...
services/file_manager/directory.mojom:23: OpenDirectory(string path, Directory&
directory) => (Error error);
is |path| only relative to this? can it contain multiple components, or can you
only traverse one segment at a time? is there '..' in this?

[viettrungluu, 2015-02-06 06:20:41]

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/di...
File services/file_manager/directory.mojom (right):

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/di...
services/file_manager/directory.mojom:16: Read() => (Error error,
array<FileInformation>? directory_contents);
On 2015/02/06 05:15:43, jamesr wrote:
> in types.mojom FileInformation appears to only have size and FileTimes.  did
you
> mean to return array of |DirectoryEntry|s here?

Oops, yes.

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/di...
services/file_manager/directory.mojom:17: Change(string path) => (Error error);
On 2015/02/06 05:15:43, jamesr wrote:
> what does 'Change' do? does it modify what this |Directory| object points to?

Yes (i.e., it's chdir). I thought it was confusing too. Maybe I should call it
ChangeDirectory.

> how's it different from OpenDirectory().. ?

OpenDirectory gives you a new Directory.

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/di...
services/file_manager/directory.mojom:21: OpenFile(string path, File& file,
uint32 access_flags, uint32 open_flags)
On 2015/02/06 05:15:43, jamesr wrote:
> if you want to model accessing existing filesystems then |string| as a type
> won't work since many filesystems support components that aren't in unicode. 
if
> we want to say that this is only for manipulating filesystems created with
this
> API or only for "sane" filesystems then we can stick with string

For the purposes of Mojo apps, the latter seems better. Otherwise it's very hard
for apps. (I'm sure we "incorrectly" -- though what's the alternative? -- assume
that filenames are Unicode in many places.)

Existing filesystems are a mess (arbitrary byte strings, minus / and \0, being
the Unix-y thing; various other filesystems having explicit or implicit
character encodings; NTFS is arbitrary uint16_t strings, minus 0) ... and not
really being able to display filenames is no good.

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/di...
services/file_manager/directory.mojom:23: OpenDirectory(string path, Directory&
directory) => (Error error);
On 2015/02/06 05:15:43, jamesr wrote:
> is |path| only relative to this? can it contain multiple components, or can
you
> only traverse one segment at a time? is there '..' in this?

I imagine that it should be allowed to contain multiple components and .. should
be allowed. (Possibly it should also allow "absolute" paths, for some meaning of
"absolute" too.)

Some "chroot"-like facility, taking a Directory and declaring it the root for
all subsequent operations originating from that Directory (including other
Directory's opened from it, etc.) and preventing escape, seems like it would be
useful. (See my TODO below about "make root".)

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/fi...
File services/file_manager/file.mojom (right):

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/fi...
services/file_manager/file.mojom:43: => (Error error);
On 2015/02/06 04:38:35, abarth wrote:
> I imagine we'll want to be able to pass Files to other apps with the idea that
> they'll be limited in their access to that file.  Going from "read-only" to
> "read-write" seems a bit scary in that situation.

Right. The more confusing ones are "append", etc.

(The reason I want "reopen" in addition to "dup" is that "dup" shares the
position in the file (the "file description", in POSIX parlance), which is
pretty inconvenient for passing Files to other apps. Maybe I should just combine
them and distinguish them using a flag though.)

[qsr, 2015-02-06 10:54:54]

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

[qsr, 2015-02-06 10:54:55]

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/fi...
File services/file_manager/file.mojom (right):

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/fi...
services/file_manager/file.mojom:23: ReadStream(handle<data_pipe_producer>
source, int64 offset, Whence whence)
When does those returns in a non error case? When the pipe is closed?

Do we want a size? I might not want to read the full file.

[sky, 2015-02-06 15:54:44]

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

[sky, 2015-02-06 15:54:46]

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/di...
File services/file_manager/directory.mojom (right):

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/di...
services/file_manager/directory.mojom:5: module mojo.files;
nit: I would have expected files to match your directory name. Maybe you should
rename one to match?

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/di...
services/file_manager/directory.mojom:17: Change(string path) => (Error error);
On 2015/02/06 06:20:41, viettrungluu wrote:
> On 2015/02/06 05:15:43, jamesr wrote:
> > what does 'Change' do? does it modify what this |Directory| object points
to?
> 
> Yes (i.e., it's chdir). I thought it was confusing too. Maybe I should call it
> ChangeDirectory.
> 
> > how's it different from OpenDirectory().. ?
> 
> OpenDirectory gives you a new Directory.

Seems less confusing to me if a directory only ever represents one path and
can't be changed. Is there a reason this needed instead of forcing callers to
use OpenDirectory?

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/di...
services/file_manager/directory.mojom:25: Rename(string path, string new_path)
=> (Error error);
Raw strings for paths is always error prone. Maybe it should be a struct with
the array of path elements? Or maybe this is better served by a client library
on top?

[viettrungluu, 2015-02-06 16:57:36]

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/di...
File services/file_manager/directory.mojom (right):

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/di...
services/file_manager/directory.mojom:5: module mojo.files;
On 2015/02/06 15:54:46, sky wrote:
> nit: I would have expected files to match your directory name. Maybe you
should
> rename one to match?

It's extremely natural for there to be other implementors of File and Directory;
"file manager" just happens to be one of them. Possibly FileManager should not
be in the "files" namespace/module though. Possibly we should also split this
directory into two (file_manager and files -- the latter might also contain
client libraries and such).

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/di...
services/file_manager/directory.mojom:25: Rename(string path, string new_path)
=> (Error error);
On 2015/02/06 15:54:45, sky wrote:
> Raw strings for paths is always error prone. Maybe it should be a struct with
> the array of path elements? Or maybe this is better served by a client library
> on top?

I could go either way on this, though the reality is that real-life OS APIs take
single strings. (If I were king, they'd all take arrays of components, and you
might also allow /'s in components.) Another argument for strings is that paths
are passed in a lot of existing code as strings, including in mixed contexts
where the strings may not always be paths (e.g., argv).

Either way, I imagine there being various client libraries. (Note that a
particularly important client library would be a <stdio.h>-type library[*],
which would take strings for paths.)

[*] Of course, in a (P)NaCl app, such a client library would be *the*
implementation of <stdio.h>.

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/fi...
File services/file_manager/file.mojom (right):

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/fi...
services/file_manager/file.mojom:23: ReadStream(handle<data_pipe_producer>
source, int64 offset, Whence whence)
On 2015/02/06 10:54:55, qsr wrote:
> When does those returns in a non error case? When the pipe is closed?

Hmmm, that's a good question.

I was imagining that it'd return as soon as reading "started" (for some weak
definition of "start"), but maybe that's not right.

(A usage pattern I was imagining was that someone might want to call this
"synchronously", end up with a valid data pipe to read from on success. Then
they might "synchronously" loop and consume from that data pipe.)

> 
> Do we want a size? I might not want to read the full file.

That's a good idea.

[viettrungluu, 2015-02-06 17:01:23]

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/di...
File services/file_manager/directory.mojom (right):

https://codereview.chromium.org/875643004/diff/40001/services/file_manager/di...
services/file_manager/directory.mojom:17: Change(string path) => (Error error);
On 2015/02/06 15:54:46, sky wrote:
> On 2015/02/06 06:20:41, viettrungluu wrote:
> > On 2015/02/06 05:15:43, jamesr wrote:
> > > what does 'Change' do? does it modify what this |Directory| object points
> to?
> > 
> > Yes (i.e., it's chdir). I thought it was confusing too. Maybe I should call
it
> > ChangeDirectory.
> > 
> > > how's it different from OpenDirectory().. ?
> > 
> > OpenDirectory gives you a new Directory.
> 
> Seems less confusing to me if a directory only ever represents one path and
> can't be changed. Is there a reason this needed instead of forcing callers to
> use OpenDirectory?

The idea was to make it easy to implement chdir() and notions like CWD for an
app, but you might right....

[darin (slow to review), 2015-02-06 23:36:09]

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

[darin (slow to review), 2015-02-06 23:36:11]

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/di...
File services/file_manager/directory.mojom (right):

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/di...
services/file_manager/directory.mojom:17: Read() => (Error error,
array<DirectoryEntry>? directory_contents);
note, this kind of API does not handle large directories very well. you may at
some point wish to have a way to stream DirectoryEntry objects.

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/di...
services/file_manager/directory.mojom:35: Delete(string path) => (Error error);
not sure if you want to include this, but a recursive delete can be quite handy.
the alternative is a lot of expensive directory enumeration.

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/fi...
File services/file_manager/file.mojom (right):

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/fi...
services/file_manager/file.mojom:23: ReadStream(handle<data_pipe_producer>
source, int64 offset, Whence whence)
nit: maybe this should be called ReadToStream? shouldn't the first parameter be
a data_pipe_consumer here? it is the intended recipient for the data that was
read from the file, right? or, is this really a function that reads from the
data_pipe_consumer and copies (writes) to the file? maybe naming consistency
with the Read/Write methods is valuable.

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/fi...
services/file_manager/file.mojom:28: Tell() => (Error error, int64 position);
is the point of these to enable some read-ahead optimizations for the Read
method? alternatively, the client could just maintain their own concept of a
cursor.

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/fi...
services/file_manager/file.mojom:47: AsBuffer() => (Error error,
handle<shared_buffer>? buffer);
how about Read/Write variants that take a SHM handle as a parameter? that way
the caller can efficiently ask to have data copied from the file into SHM
without having to map the entire file.

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/fi...
File services/file_manager/file_manager.mojom (right):

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/fi...
services/file_manager/file_manager.mojom:12: USER,
maybe for starters, there should only be temporary storage.

[vtl, 2015-02-06 23:51:48]

vtl@google.com changed reviewers:
+ vtl@google.com

[vtl, 2015-02-06 23:51:50]

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/di...
File services/file_manager/directory.mojom (right):

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/di...
services/file_manager/directory.mojom:17: Read() => (Error error,
array<DirectoryEntry>? directory_contents);
On 2015/02/06 23:36:11, darin (slow to review) wrote:
> note, this kind of API does not handle large directories very well. you may at
> some point wish to have a way to stream DirectoryEntry objects.

Yep (see TODO below).

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/di...
services/file_manager/directory.mojom:35: Delete(string path) => (Error error);
On 2015/02/06 23:36:11, darin (slow to review) wrote:
> not sure if you want to include this, but a recursive delete can be quite
handy.
> the alternative is a lot of expensive directory enumeration.

Maybe this should take flags.

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/fi...
File services/file_manager/file.mojom (right):

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/fi...
services/file_manager/file.mojom:23: ReadStream(handle<data_pipe_producer>
source, int64 offset, Whence whence)
On 2015/02/06 23:36:11, darin (slow to review) wrote:
> nit: maybe this should be called ReadToStream?

Maybe?

> shouldn't the first parameter be
> a data_pipe_consumer here?

Nope.

> it is the intended recipient for the data that was
> read from the file, right?

Yes, which is why the receiver (e.g., the file manager) needs the producer
handle; the caller is the consumer.

> or, is this really a function that reads from the
> data_pipe_consumer and copies (writes) to the file? maybe naming consistency
> with the Read/Write methods is valuable.

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/fi...
services/file_manager/file.mojom:28: Tell() => (Error error, int64 position);
On 2015/02/06 23:36:11, darin (slow to review) wrote:
> is the point of these to enable some read-ahead optimizations for the Read
> method? alternatively, the client could just maintain their own concept of a
> cursor.

If you want to support POSIX-ish/standard C library-ish functionality, you need
a shared cursor.

(Admittedly, you may only want those semantics within a single app. But then
you'd be forced to wrap everything extremely thoroughly and multiplex "fake FDs"
to Files.)

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/fi...
services/file_manager/file.mojom:47: AsBuffer() => (Error error,
handle<shared_buffer>? buffer);
On 2015/02/06 23:36:11, darin (slow to review) wrote:
> how about Read/Write variants that take a SHM handle as a parameter? that way
> the caller can efficiently ask to have data copied from the file into SHM
> without having to map the entire file.

They don't have to map the entire file (buffer handles support arbitrary
mapping).

Moreover, if you're doing that sort of thing (setting up/tearing down shmem on a
per read/write basis), you'd probably be better off just sending/copying the
data most of the time anyway.

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/fi...
File services/file_manager/file_manager.mojom (right):

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/fi...
services/file_manager/file_manager.mojom:12: USER,
On 2015/02/06 23:36:11, darin (slow to review) wrote:
> maybe for starters, there should only be temporary storage.

Yup (see my message in mojo-dev).

(On the other hand, for development/debugging purposes, it's kind of handy to
have a known/stable place for files to go.)

[darin (slow to review), 2015-02-07 00:05:54]

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/fi...
File services/file_manager/file.mojom (right):

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/fi...
services/file_manager/file.mojom:23: ReadStream(handle<data_pipe_producer>
source, int64 offset, Whence whence)
On 2015/02/06 23:51:50, vtl wrote:
> On 2015/02/06 23:36:11, darin (slow to review) wrote:
> > nit: maybe this should be called ReadToStream?
> 
> Maybe?
> 
> > shouldn't the first parameter be
> > a data_pipe_consumer here?
> 
> Nope.
> 
> > it is the intended recipient for the data that was
> > read from the file, right?
> 
> Yes, which is why the receiver (e.g., the file manager) needs the producer
> handle; the caller is the consumer.

OK, yes... I got myself confused. The name ReadStream sounds like it means "read
the stream" or "read (from) stream" rather than "read data from file copying
output to stream". hence, the ReadToStream suggestion.

> 
> > or, is this really a function that reads from the
> > data_pipe_consumer and copies (writes) to the file? maybe naming consistency
> > with the Read/Write methods is valuable.

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/fi...
services/file_manager/file.mojom:28: Tell() => (Error error, int64 position);
On 2015/02/06 23:51:49, vtl wrote:
> On 2015/02/06 23:36:11, darin (slow to review) wrote:
> > is the point of these to enable some read-ahead optimizations for the Read
> > method? alternatively, the client could just maintain their own concept of a
> > cursor.
> 
> If you want to support POSIX-ish/standard C library-ish functionality, you
need
> a shared cursor.
> 
> (Admittedly, you may only want those semantics within a single app. But then
> you'd be forced to wrap everything extremely thoroughly and multiplex "fake
FDs"
> to Files.)

OK, I see.

https://codereview.chromium.org/875643004/diff/80001/services/file_manager/fi...
services/file_manager/file.mojom:47: AsBuffer() => (Error error,
handle<shared_buffer>? buffer);
On 2015/02/06 23:51:49, vtl wrote:
> On 2015/02/06 23:36:11, darin (slow to review) wrote:
> > how about Read/Write variants that take a SHM handle as a parameter? that
way
> > the caller can efficiently ask to have data copied from the file into SHM
> > without having to map the entire file.
> 
> They don't have to map the entire file (buffer handles support arbitrary
> mapping).

I see.

> 
> Moreover, if you're doing that sort of thing (setting up/tearing down shmem on
a
> per read/write basis), you'd probably be better off just sending/copying the
> data most of the time anyway.

Oh, I was thinking about cases where someone might want to copy a range from a
file (or multiple files) into a piece of memory. Using SHM could be useful in
that case. Having to map the file and dereference to achieve the effect suffers
from blocking the client while the file is synchronously brought into memory.

[viettrungluu, 2015-02-27 18:37:31]

viettrungluu@chromium.org changed reviewers:
+ jamesr@chromium.org
- aa@chromium.org, abarth@chromium.org, darin@chromium.org, qsr@chromium.org,
sky@chromium.org, vtl@google.com

[viettrungluu, 2015-02-27 18:38:23]

OK, ready for initial review.

-> jamesr, who (I) "volunteered"; everyone else to CC

[qsr, 2015-03-02 12:46:38]

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

[qsr, 2015-03-02 12:46:40]

https://codereview.chromium.org/875643004/diff/530001/services/files/director...
File services/files/directory.mojom (right):

https://codereview.chromium.org/875643004/diff/530001/services/files/director...
services/files/directory.mojom:17: // TODO(vtl): Should we have a "close"
method?
Isn't closing the handle enough? What can you do with a connected close
directory?

https://codereview.chromium.org/875643004/diff/530001/services/files/director...
services/files/directory.mojom:20: Touch(TimespecOrNow? atime, TimespecOrNow?
mtime) => (Error error);
Do a null atime or mtime means I should not modify it? Can you specify this?

https://codereview.chromium.org/875643004/diff/530001/services/files/file.mojom
File services/files/file.mojom (right):

https://codereview.chromium.org/875643004/diff/530001/services/files/file.moj...
services/files/file.mojom:16: Close() => (Error err);
What can you do with a connected closed file? If nothing, should we get rid of
this and just use closing the handle as the right signal?

https://codereview.chromium.org/875643004/diff/530001/services/files/file.moj...
services/files/file.mojom:27: // appended) modes, and how would those be
signalled?
Do you also need a special value of no limit?

https://codereview.chromium.org/875643004/diff/530001/services/files/file.moj...
services/files/file.mojom:40: // TODO(vtl): null |times| means "now" (for both
atime and mtime).
Isn't the comment outdated? You have way to specify now in your data structure,
so you can use null |times| to mean do not modify.

https://codereview.chromium.org/875643004/diff/530001/services/files/file_imp...
File services/files/file_impl.cc (right):

https://codereview.chromium.org/875643004/diff/530001/services/files/file_imp...
services/files/file_impl.cc:73: // multithreaded.) Maybe we should do an
|ftell()| and always use |pread()|.
Do you plan to have multiple call to the same file be in different thread in
your thread pool? That would be kind of weird, as if I do multiple read, I
expect that to work. So, we probably should make sure that multiple call to the
same file ends up on the same thread, so you should never have this issue. Or am
I missing something?

https://codereview.chromium.org/875643004/diff/530001/services/files/files.mojom
File services/files/files.mojom (right):

https://codereview.chromium.org/875643004/diff/530001/services/files/files.mo...
services/files/files.mojom:16: // exist) only available in Debug builds.
What is ~/MojoDebug on Android?

https://codereview.chromium.org/875643004/diff/530001/services/files/files_im...
File services/files/files_impl.cc (right):

https://codereview.chromium.org/875643004/diff/530001/services/files/files_im...
services/files/files_impl.cc:76: LOG(WARNING) << "~/MojoDebug only available in
Debug builds";
You should call the callback, otherwise client will not be notified.

https://codereview.chromium.org/875643004/diff/530001/services/files/types.mojom
File services/files/types.mojom (right):

https://codereview.chromium.org/875643004/diff/530001/services/files/types.mo...
services/files/types.mojom:42: // TODO(vtl): Use a union instead, when that
becomes possible.
I am missing something obvious, but why do you need the boolean at all?

https://codereview.chromium.org/875643004/diff/530001/services/files/types.mo...
services/files/types.mojom:53: int64 size;
What does size mean for a directory?

[viettrungluu, 2015-03-02 18:11:00]

https://codereview.chromium.org/875643004/diff/530001/services/files/director...
File services/files/directory.mojom (right):

https://codereview.chromium.org/875643004/diff/530001/services/files/director...
services/files/directory.mojom:17: // TODO(vtl): Should we have a "close"
method?
On 2015/03/02 12:46:40, qsr wrote:
> Isn't closing the handle enough? What can you do with a connected close
> directory?

See my answer for files. Having an explicit close for directories is a little
less important, but possibly it'd be good for consistency.

https://codereview.chromium.org/875643004/diff/530001/services/files/director...
services/files/directory.mojom:20: Touch(TimespecOrNow? atime, TimespecOrNow?
mtime) => (Error error);
On 2015/03/02 12:46:40, qsr wrote:
> Do a null atime or mtime means I should not modify it? Can you specify this?

See TODO at line #13. Lots of comments are obviously needed.

https://codereview.chromium.org/875643004/diff/530001/services/files/file.mojom
File services/files/file.mojom (right):

https://codereview.chromium.org/875643004/diff/530001/services/files/file.moj...
services/files/file.mojom:16: Close() => (Error err);
On 2015/03/02 12:46:40, qsr wrote:
> What can you do with a connected closed file?

Not very much.

> If nothing, should we get rid of
> this and just use closing the handle as the right signal?

There are two reasons to have an explicit close:

* It acts as an explicit flush with acknowledgement.
* It gives a chance for more errors to be reported. (close(2) says: "Not
checking the return value of close() is a common  but  nevertheless serious 
programming error.  It is quite possible that errors on a previous write(2)
operation are first reported at the final close()." Of course, it then goes to
undermine itself somewhat in the following paragraph.)

https://codereview.chromium.org/875643004/diff/530001/services/files/file.moj...
services/files/file.mojom:27: // appended) modes, and how would those be
signalled?
On 2015/03/02 12:46:40, qsr wrote:
> Do you also need a special value of no limit?

See TODO above -- we need TWO special values!

https://codereview.chromium.org/875643004/diff/530001/services/files/file.moj...
services/files/file.mojom:40: // TODO(vtl): null |times| means "now" (for both
atime and mtime).
On 2015/03/02 12:46:40, qsr wrote:
> Isn't the comment outdated? You have way to specify now in your data
structure,
> so you can use null |times| to mean do not modify.

Done.

https://codereview.chromium.org/875643004/diff/530001/services/files/file_imp...
File services/files/file_impl.cc (right):

https://codereview.chromium.org/875643004/diff/530001/services/files/file_imp...
services/files/file_impl.cc:73: // multithreaded.) Maybe we should do an
|ftell()| and always use |pread()|.
On 2015/03/02 12:46:40, qsr wrote:
> Do you plan to have multiple call to the same file be in different thread in
> your thread pool?

Obviously, calls to the same (Mojo) handle must be serialized.

> That would be kind of weird, as if I do multiple read, I
> expect that to work. So, we probably should make sure that multiple call to
the
> same file ends up on the same thread, so you should never have this issue.

It's quite hard/costly to guarantee that multiple Mojo handles referencing the
same file are serialized, and not obviously desirable.

For files that may be read by lots of things, this would reduce concurrency and
play badly with priorities/scheduling.

> Or am
> I missing something?

Probably the right thing to do would be to make it so that things sharing the
same file *description* (i.e., are all dup()ed from the same original FD) are
serialized.

https://codereview.chromium.org/875643004/diff/530001/services/files/files.mojom
File services/files/files.mojom (right):

https://codereview.chromium.org/875643004/diff/530001/services/files/files.mo...
services/files/files.mojom:16: // exist) only available in Debug builds.
On 2015/03/02 12:46:40, qsr wrote:
> What is ~/MojoDebug on Android?

It probably doesn't work, but feel free to suggest an alternative.

https://codereview.chromium.org/875643004/diff/530001/services/files/types.mojom
File services/files/types.mojom (right):

https://codereview.chromium.org/875643004/diff/530001/services/files/types.mo...
services/files/types.mojom:42: // TODO(vtl): Use a union instead, when that
becomes possible.
On 2015/03/02 12:46:40, qsr wrote:
> I am missing something obvious, but why do you need the boolean at all?

Really, this should be turned into a union, once those are ready for prime time,
so I'd prefer to keep the explicit bool.

https://codereview.chromium.org/875643004/diff/530001/services/files/types.mo...
services/files/types.mojom:53: int64 size;
On 2015/03/02 12:46:40, qsr wrote:
> What does size mean for a directory?

That's a good question (to be pondered). stat(2) doesn't really say, so it's
probably undefined according to POSIX. Maybe we should define it to be zero.

[viettrungluu, 2015-03-03 05:30:26]

qsr -- do you want to review this (instead of jamesr)? (Keep in mind that this
is far from a finished product....)

[qsr, 2015-03-03 11:56:45]

https://codereview.chromium.org/875643004/diff/530001/services/files/file.mojom
File services/files/file.mojom (right):

https://codereview.chromium.org/875643004/diff/530001/services/files/file.moj...
services/files/file.mojom:16: Close() => (Error err);
On 2015/03/02 18:10:59, viettrungluu wrote:
> On 2015/03/02 12:46:40, qsr wrote:
> > What can you do with a connected closed file?
> 
> Not very much.
> 
> > If nothing, should we get rid of
> > this and just use closing the handle as the right signal?
> 
> There are two reasons to have an explicit close:

 Ok

> * It acts as an explicit flush with acknowledgement.

 Hum. Should we have a flush method? I don't see any documentation on the
different write method relative to flushing.

> * It gives a chance for more errors to be reported. (close(2) says: "Not
> checking the return value of close() is a common  but  nevertheless serious 
> programming error.  It is quite possible that errors on a previous write(2)
> operation are first reported at the final close()." Of course, it then goes to
> undermine itself somewhat in the following paragraph.)

https://codereview.chromium.org/875643004/diff/530001/services/files/files.mojom
File services/files/files.mojom (right):

https://codereview.chromium.org/875643004/diff/530001/services/files/files.mo...
services/files/files.mojom:16: // exist) only available in Debug builds.
On 2015/03/02 18:10:59, viettrungluu wrote:
> On 2015/03/02 12:46:40, qsr wrote:
> > What is ~/MojoDebug on Android?
> 
> It probably doesn't work, but feel free to suggest an alternative.

I guess I can make sure that HOME has some kind of usable value on android. Then
this will work, as this is only for debug. But when we will want per app
storage, we will need something more...

https://codereview.chromium.org/875643004/diff/530001/services/files/files_im...
File services/files/files_impl.cc (right):

https://codereview.chromium.org/875643004/diff/530001/services/files/files_im...
services/files/files_impl.cc:76: LOG(WARNING) << "~/MojoDebug only available in
Debug builds";
On 2015/03/02 12:46:40, qsr wrote:
> You should call the callback, otherwise client will not be notified.

You didn't change this -> I really think this should be modified to call the
callback with an error here. As it is now, you fill create a Directory with a
null directory fd, but still return a ERROR_OK.

https://codereview.chromium.org/875643004/diff/530001/services/files/types.mojom
File services/files/types.mojom (right):

https://codereview.chromium.org/875643004/diff/530001/services/files/types.mo...
services/files/types.mojom:42: // TODO(vtl): Use a union instead, when that
becomes possible.
On 2015/03/02 18:10:59, viettrungluu wrote:
> On 2015/03/02 12:46:40, qsr wrote:
> > I am missing something obvious, but why do you need the boolean at all?
> 
> Really, this should be turned into a union, once those are ready for prime
time,
> so I'd prefer to keep the explicit bool.

What would the union looks like? The same as this? This is still kind of weird,
because you won't care about the value of the boolean. Isn't the following
struct exactly what you need:

struct TimespecOrNow {
  Timespec? timespec;
}

and you specify that a null timespec means Now.

You can then have the 3 states you need -> A null TimespecOrNow is no info, a
non-null TimespecOrNow with a null timespec is now, and a non-null timespec is
the value of the timespec.

I am not sure the union makes it clearer, unless you have another idea about
what the |now| will be.

https://codereview.chromium.org/875643004/diff/570001/services/files/director...
File services/files/directory.mojom (right):

https://codereview.chromium.org/875643004/diff/570001/services/files/director...
services/files/directory.mojom:11: // (currently not)
Can you specify somewhere that all |path| in the different operations are
relatives to "this" directory.

https://codereview.chromium.org/875643004/diff/570001/services/files/director...
services/files/directory.mojom:41: // this may be used as a simple "mkdir()"
with |kOpenFlagCreate|.
I guess "." is a valid path? Is this the way to duplicate a directory to be able
to send it to another application?

https://codereview.chromium.org/875643004/diff/570001/services/files/director...
services/files/directory.mojom:58: // "root")
Agreed on letting this as a TODO, but shouldn't this be the default (or even the
only mode of operation)? I don't think we should ever accept path that would get
out of the current directly. You use Directories to have a root, and each time
you cd, you change the root, if you want to have access to more, you keep the
original root.

https://codereview.chromium.org/875643004/diff/570001/services/files/director...
File services/files/directory_impl.cc (right):

https://codereview.chromium.org/875643004/diff/570001/services/files/director...
services/files/directory_impl.cc:51: if ((open_flags & (kOpenFlagAppend |
kOpenFlagTruncate)))
You have one pair of parenthesis too many.

https://codereview.chromium.org/875643004/diff/570001/services/files/director...
File services/files/directory_impl.h (right):

https://codereview.chromium.org/875643004/diff/570001/services/files/director...
services/files/directory_impl.h:52: base::FilePath owned_dir_name_;
It seems that you only use this with temporary directory. What do you think
about using base/files/scoped_temp_dir.h to not have to handle all of this
yourself?

https://codereview.chromium.org/875643004/diff/570001/services/files/file_imp...
File services/files/file_impl.cc (right):

https://codereview.chromium.org/875643004/diff/570001/services/files/file_imp...
services/files/file_impl.cc:28: const size_t kMaxReadSize = 4 * 1024 * 1024;
Any reason you want to limit this? Performance reason?

https://codereview.chromium.org/875643004/diff/570001/services/files/file_imp...
services/files/file_impl.cc:44: callback.Run(ERROR_OK);
Didn't you tell me that the reason we had close is to be able to send the right
error :) Maybe we should try doing this.

https://codereview.chromium.org/875643004/diff/570001/services/files/file_imp...
services/files/file_impl.cc:75: WhenceToStandardWhence(whence)) < 0) {
It is not clear from the documentation that the file offset will be changed by
this method, and if we look at pread/pwrite, they do not change it, so maybe we
shouldn't either.

https://codereview.chromium.org/875643004/diff/570001/services/files/file_imp...
services/files/file_impl.cc:85: // TODO(vtl): Decode errno.
Should you use ErrnoToError?

https://codereview.chromium.org/875643004/diff/570001/services/files/file_imp...
services/files/file_impl.cc:127: // |pwrite()|.
"

https://codereview.chromium.org/875643004/diff/570001/services/files/files_im...
File services/files/files_impl.cc (right):

https://codereview.chromium.org/875643004/diff/570001/services/files/files_im...
services/files/files_impl.cc:76: LOG(WARNING) << "~/MojoDebug only available in
Debug builds";
See my comment last time. You are not returning an error for this case.

[viettrungluu, 2015-03-03 18:50:36]

Thanks, PTAL.

https://codereview.chromium.org/875643004/diff/530001/services/files/file.mojom
File services/files/file.mojom (right):

https://codereview.chromium.org/875643004/diff/530001/services/files/file.moj...
services/files/file.mojom:16: Close() => (Error err);
On 2015/03/03 11:56:44, qsr wrote:
> On 2015/03/02 18:10:59, viettrungluu wrote:
> > On 2015/03/02 12:46:40, qsr wrote:
> > > What can you do with a connected closed file?
> > 
> > Not very much.
> > 
> > > If nothing, should we get rid of
> > > this and just use closing the handle as the right signal?
> > 
> > There are two reasons to have an explicit close:
> 
>  Ok
> 
> > * It acts as an explicit flush with acknowledgement.
> 
>  Hum. Should we have a flush method? I don't see any documentation on the
> different write method relative to flushing.

I meant that it's similar in effect to closing the message pipe, except with
acknowledgement. (Possibly we should have something analogous to fsync(2) also.
I added a TODO for this.)

In general, I'm a bit nervous about tying a potentially-important operation like
closing to the closure of the message pipe (it may be important since the
implementation may take it as a signal to, e.g., upload/synchronize data).

This is especially true for GCed languages, where releasing the last ref to an
object isn't sufficient to guarantee destruction and thus message pipe closure.
But maybe this means that MojoHandle wrapper objects for GCed languages need
explicit close methods. If that's the case, then we could remove this. WDYT?

> > * It gives a chance for more errors to be reported. (close(2) says: "Not
> > checking the return value of close() is a common  but  nevertheless serious 
> > programming error.  It is quite possible that errors on a previous write(2)
> > operation are first reported at the final close()." Of course, it then goes
to
> > undermine itself somewhat in the following paragraph.)
>

https://codereview.chromium.org/875643004/diff/530001/services/files/files.mojom
File services/files/files.mojom (right):

https://codereview.chromium.org/875643004/diff/530001/services/files/files.mo...
services/files/files.mojom:16: // exist) only available in Debug builds.
On 2015/03/03 11:56:44, qsr wrote:
> On 2015/03/02 18:10:59, viettrungluu wrote:
> > On 2015/03/02 12:46:40, qsr wrote:
> > > What is ~/MojoDebug on Android?
> > 
> > It probably doesn't work, but feel free to suggest an alternative.
> 
> I guess I can make sure that HOME has some kind of usable value on android.
Then
> this will work, as this is only for debug. But when we will want per app
> storage, we will need something more...

Right. We'll need to decide on per-app versus per-origin, and also figure out
persistence rules, quota, etc. (Probably we shouldn't encourage too much data to
be saved locally.)

https://codereview.chromium.org/875643004/diff/530001/services/files/files_im...
File services/files/files_impl.cc (right):

https://codereview.chromium.org/875643004/diff/530001/services/files/files_im...
services/files/files_impl.cc:76: LOG(WARNING) << "~/MojoDebug only available in
Debug builds";
On 2015/03/03 11:56:44, qsr wrote:
> On 2015/03/02 12:46:40, qsr wrote:
> > You should call the callback, otherwise client will not be notified.
> 
> You didn't change this -> I really think this should be modified to call the
> callback with an error here. As it is now, you fill create a Directory with a
> null directory fd, but still return a ERROR_OK.

Oops, missed your previous comment. Sorry.

I had meant to put the #endif after the dir_fd.reset(). Fixed.

https://codereview.chromium.org/875643004/diff/530001/services/files/types.mojom
File services/files/types.mojom (right):

https://codereview.chromium.org/875643004/diff/530001/services/files/types.mo...
services/files/types.mojom:42: // TODO(vtl): Use a union instead, when that
becomes possible.
On 2015/03/03 11:56:44, qsr wrote:
> On 2015/03/02 18:10:59, viettrungluu wrote:
> > On 2015/03/02 12:46:40, qsr wrote:
> > > I am missing something obvious, but why do you need the boolean at all?
> > 
> > Really, this should be turned into a union, once those are ready for prime
> time,
> > so I'd prefer to keep the explicit bool.
> 
> What would the union looks like? The same as this? This is still kind of
weird,
> because you won't care about the value of the boolean. Isn't the following
> struct exactly what you need:
> 
> struct TimespecOrNow {
>   Timespec? timespec;
> }
> 
> and you specify that a null timespec means Now.
> 
> You can then have the 3 states you need -> A null TimespecOrNow is no info, a
> non-null TimespecOrNow with a null timespec is now, and a non-null timespec is
> the value of the timespec.
> 
> I am not sure the union makes it clearer, unless you have another idea about
> what the |now| will be.

Yeah, I'm not super-happy about it. Really, what I want is a union with a unary
"now" choice. I don't like having a struct containing only a nullable struct for
this purpose -- it's a just a bit too weird/unclear.

I could make now a "unary enum" for the purposes of such a union, but our enums
are currently always extensible and only weakly typed.

https://codereview.chromium.org/875643004/diff/530001/services/files/types.mo...
services/files/types.mojom:53: int64 size;
On 2015/03/02 18:10:59, viettrungluu wrote:
> On 2015/03/02 12:46:40, qsr wrote:
> > What does size mean for a directory?
> 
> That's a good question (to be pondered). stat(2) doesn't really say, so it's
> probably undefined according to POSIX. Maybe we should define it to be zero.

(I defined it to be zero for directories.)

https://codereview.chromium.org/875643004/diff/570001/services/files/director...
File services/files/directory.mojom (right):

https://codereview.chromium.org/875643004/diff/570001/services/files/director...
services/files/directory.mojom:11: // (currently not)
On 2015/03/03 11:56:44, qsr wrote:
> Can you specify somewhere that all |path| in the different operations are
> relatives to "this" directory.

Done.

https://codereview.chromium.org/875643004/diff/570001/services/files/director...
services/files/directory.mojom:41: // this may be used as a simple "mkdir()"
with |kOpenFlagCreate|.
On 2015/03/03 11:56:44, qsr wrote:
> I guess "." is a valid path? Is this the way to duplicate a directory to be
able
> to send it to another application?

That's a good point. Possibly Directory should have Dup()/Reopen(), like File.
(Added TODO.)

https://codereview.chromium.org/875643004/diff/570001/services/files/director...
services/files/directory.mojom:58: // "root")
On 2015/03/03 11:56:44, qsr wrote:
> Agreed on letting this as a TODO, but shouldn't this be the default (or even
the
> only mode of operation)? I don't think we should ever accept path that would
get
> out of the current directly. You use Directories to have a root, and each time
> you cd, you change the root, if you want to have access to more, you keep the
> original root.

Being unable to chdir() out would make it difficult/impossible for people to
implement POSIX-style current working directories (which may even be
per-thread).

(To imitate it, an app would have to just keep the root Directory, and then keep
track of the CWD as a path. But this isn't a faithful imitation: "cd foo; mkdir
../bar; mv ../foo ../bar; cd .." probably wouldn't work right -- especially if
the mv were done by someone else.)

https://codereview.chromium.org/875643004/diff/570001/services/files/director...
File services/files/directory_impl.cc (right):

https://codereview.chromium.org/875643004/diff/570001/services/files/director...
services/files/directory_impl.cc:51: if ((open_flags & (kOpenFlagAppend |
kOpenFlagTruncate)))
On 2015/03/03 11:56:44, qsr wrote:
> You have one pair of parenthesis too many.

The extra pair of parentheses indicates that I really want to test the truth
value of a bitwise operation, i.e., that I actually mean & and not &&.

(Some compilers with some settings will warn about this, at least in some
situations. I agree that this case is fairly unambiguous, but I prefer to be
consistent and also don't feel like figuring out which of our many build
configurations may or may not complain about it.)

https://codereview.chromium.org/875643004/diff/570001/services/files/director...
File services/files/directory_impl.h (right):

https://codereview.chromium.org/875643004/diff/570001/services/files/director...
services/files/directory_impl.h:52: base::FilePath owned_dir_name_;
On 2015/03/03 11:56:44, qsr wrote:
> It seems that you only use this with temporary directory. What do you think
> about using base/files/scoped_temp_dir.h to not have to handle all of this
> yourself?

Done.

https://codereview.chromium.org/875643004/diff/570001/services/files/file_imp...
File services/files/file_impl.cc (right):

https://codereview.chromium.org/875643004/diff/570001/services/files/file_imp...
services/files/file_impl.cc:28: const size_t kMaxReadSize = 4 * 1024 * 1024;
On 2015/03/03 11:56:44, qsr wrote:
> Any reason you want to limit this? Performance reason? 

Performance among others (e.g., resource consumption). Also, we'll also run into
message size limitations (which exist for the same reasons).

It occurs to me that this is already too big, so I guess I'll have to reduce it.

https://codereview.chromium.org/875643004/diff/570001/services/files/file_imp...
services/files/file_impl.cc:44: callback.Run(ERROR_OK);
On 2015/03/03 11:56:44, qsr wrote:
> Didn't you tell me that the reason we had close is to be able to send the
right
> error :) Maybe we should try doing this.

Done, though when you see the code you may still decide that it's a terrible
idea.

(I still don't like having an "active destructor" though.)

https://codereview.chromium.org/875643004/diff/570001/services/files/file_imp...
services/files/file_impl.cc:75: WhenceToStandardWhence(whence)) < 0) {
On 2015/03/03 11:56:45, qsr wrote:
> It is not clear from the documentation that the file offset will be changed by
> this method, and if we look at pread/pwrite, they do not change it, so maybe
we
> shouldn't either.

I wonder if we should have a flag, or if we should only change the position if
offset != 0 || whence != WHENCE_FROM_CURRENT.

(But then implementing WHENCE_FROM_END is tougher, since it'll require a stat()
to compute the offset for pread().)

If you don't mind too much, I'll leave this as a TODO. (I'll also add a TODO to
file.mojom about this.)

https://codereview.chromium.org/875643004/diff/570001/services/files/file_imp...
services/files/file_impl.cc:85: // TODO(vtl): Decode errno.
On 2015/03/03 11:56:44, qsr wrote:
> Should you use ErrnoToError?

Done.

https://codereview.chromium.org/875643004/diff/570001/services/files/file_imp...
services/files/file_impl.cc:127: // |pwrite()|.
On 2015/03/03 11:56:45, qsr wrote:
> "

Acknowledged.

https://codereview.chromium.org/875643004/diff/570001/services/files/files_im...
File services/files/files_impl.cc (right):

https://codereview.chromium.org/875643004/diff/570001/services/files/files_im...
services/files/files_impl.cc:76: LOG(WARNING) << "~/MojoDebug only available in
Debug builds";
On 2015/03/03 11:56:45, qsr wrote:
> See my comment last time. You are not returning an error for this case.

Acknowledged.

[qsr, 2015-03-04 15:10:19]

lgtm

https://codereview.chromium.org/875643004/diff/570001/services/files/director...
File services/files/directory.mojom (right):

https://codereview.chromium.org/875643004/diff/570001/services/files/director...
services/files/directory.mojom:58: // "root")
On 2015/03/03 18:50:35, viettrungluu wrote:
> On 2015/03/03 11:56:44, qsr wrote:
> > Agreed on letting this as a TODO, but shouldn't this be the default (or even
> the
> > only mode of operation)? I don't think we should ever accept path that would
> get
> > out of the current directly. You use Directories to have a root, and each
time
> > you cd, you change the root, if you want to have access to more, you keep
the
> > original root.
> 
> Being unable to chdir() out would make it difficult/impossible for people to
> implement POSIX-style current working directories (which may even be
> per-thread).
> 
> (To imitate it, an app would have to just keep the root Directory, and then
keep
> track of the CWD as a path. But this isn't a faithful imitation: "cd foo;
mkdir
> ../bar; mv ../foo ../bar; cd .." probably wouldn't work right -- especially if
> the mv were done by someone else.)

 I see. But then, do you have any idea how to correctly implement rooting a
directory? If I have a rooted directory, I go one down to a sub-directory (so
the directory I opened is not rooted anymore, but it is expected I cannot go up
more than once), then this directory is moved outside of its parent. How would
we make sure that you cannot go up to some place you never were supposed to be
able to access?

 If we prevent any access to anything over the current directory, I think we do
not have this kind of problems.

https://codereview.chromium.org/875643004/diff/570001/services/files/file_imp...
File services/files/file_impl.cc (right):

https://codereview.chromium.org/875643004/diff/570001/services/files/file_imp...
services/files/file_impl.cc:44: callback.Run(ERROR_OK);
On 2015/03/03 18:50:36, viettrungluu wrote:
> On 2015/03/03 11:56:44, qsr wrote:
> > Didn't you tell me that the reason we had close is to be able to send the
> right
> > error :) Maybe we should try doing this.
> 
> Done, though when you see the code you may still decide that it's a terrible
> idea.
> 
> (I still don't like having an "active destructor" though.)

 The implementation is not as bad as you make it sound. We solved the EINTR
problem in base if I followed correctly the bug. On the other end, if we decide
we do not care about error, we should remove it from the signature.

About active destructor, I guess this is mostly your choice, but for info about
gc, on java you are suppose to close your proxy and not rely on the gc for this.

[viettrungluu, 2015-03-04 16:03:26]

Committed patchset #32 (id:610001) manually as
2a394496d9f098564e3a52cc62c937801dd1b1f0 (presubmit successful).