Issue 963093004: Files: Add basic implementation of Directory and some basic tests.

Issue 963093004: Files: Add basic implementation of Directory and some basic tests. (Closed)

Created:
5 years, 9 months ago by viettrungluu

Modified:
5 years, 9 months ago

Reviewers:
qsr

CC:
mojo-reviews_chromium.org, qsr+mojo_chromium.org, yzshen+watch_chromium.org

Base URL:
https://github.com/domokit/mojo.git@file_man

Target Ref:
refs/heads/master

Project:
mojo

Visibility:
Public.

More Reviews

Description

Files: Add basic implementation of Directory and some basic tests. R=qsr@chromium.org Committed: https://chromium.googlesource.com/external/mojo/+/451ed4192f8561ecaa2a930c5a67ef00f0bca2f8

Patch Set 6 : review comments #

Patch Set 7 : oops #

Created: 5 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+508 lines, -89 lines)			Patch
M	services/files/BUILD.gn	View	1 2 3 4 5	2 chunks	+3 lines, -0 lines	0 comments	Download
M	services/files/directory_impl.cc	View	1 2 3 4 5	7 chunks	+179 lines, -18 lines	0 comments	Download
M	services/files/file_impl.cc	View	1 2 3 4 5	4 chunks	+3 lines, -43 lines	0 comments	Download
M	services/files/files_apptest.cc	View	1 2 3 4 5	7 chunks	+214 lines, -28 lines	0 comments	Download
A	services/files/shared_impl.h	View	1 2 3 4 5	1 chunk	+29 lines, -0 lines	0 comments	Download
A	services/files/shared_impl.cc	View	1 2 3 4 5 6	1 chunk	+80 lines, -0 lines	0 comments	Download

Messages

Total messages: 5 (1 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

qsr

LGTM with nits. https://codereview.chromium.org/963093004/diff/80001/services/files/directory_impl.cc File services/files/directory_impl.cc (right): https://codereview.chromium.org/963093004/diff/80001/services/files/directory_impl.cc#newcode133 services/files/directory_impl.cc:133: // enough), but it's fine for ...

5 years, 9 months ago (2015-03-06 13:01:05 UTC) #3

viettrungluu

Thanks. https://codereview.chromium.org/963093004/diff/80001/services/files/directory_impl.cc File services/files/directory_impl.cc (right): https://codereview.chromium.org/963093004/diff/80001/services/files/directory_impl.cc#newcode133 services/files/directory_impl.cc:133: // enough), but it's fine for Linux. On ...

5 years, 9 months ago (2015-03-09 17:59:29 UTC) #4

Thanks.

https://codereview.chromium.org/963093004/diff/80001/services/files/directory...
File services/files/directory_impl.cc (right):

https://codereview.chromium.org/963093004/diff/80001/services/files/directory...
services/files/directory_impl.cc:133: // enough), but it's fine for Linux.
On 2015/03/06 13:01:04, qsr wrote:
> Do you want to put a static assert on OS_ANDROID || OS_LINUX do that we know
> when we are not safe anymore?

I'll use an #if !defined(...) and #error.

https://codereview.chromium.org/963093004/diff/80001/services/files/directory...
services/files/directory_impl.cc:162: e->type = FILE_TYPE_UNKNOWN;
On 2015/03/06 13:01:04, qsr wrote:
> I wonder if we should not just hide all of those entries instead.

It's tempting, but then it'd be odd if someone tried to create a file and failed
since something of that name already exists (or tried to remove an
apparently-empty directory and failed).

https://codereview.chromium.org/963093004/diff/80001/services/files/directory...
services/files/directory_impl.cc:176: // TODO(vtl): The code below is copied
from |FileImpl::Stat()|.
On 2015/03/06 13:01:04, qsr wrote:
> Could you extract it in a helper function then?

Done.

https://codereview.chromium.org/963093004/diff/80001/services/files/directory...
services/files/directory_impl.cc:207: // TODO(vtl): The code below is copied
from |FileImpl::Stat()|.
On 2015/03/06 13:01:04, qsr wrote:
> "

Done.

https://codereview.chromium.org/963093004/diff/80001/services/files/directory...
services/files/directory_impl.cc:299: // Allow |EEXIST| if |kOpenFlagExclusive|
is set. Note, however, that it
On 2015/03/06 13:01:04, qsr wrote:
> is not set?

Done.

https://codereview.chromium.org/963093004/diff/80001/services/files/directory...
services/files/directory_impl.cc:351: // TODO(vtl): Should we check that |path|
is nonempty?
On 2015/03/06 13:01:04, qsr wrote:
> You would need more than non-empty. foo/.. is not better than ""
> 
> We will probably need some notion of resolving path sometime I guess.

Errrr, right. I guess this falls under the "see TODOs about |path| in
OpenFile()" below.

https://codereview.chromium.org/963093004/diff/80001/services/files/directory...
services/files/directory_impl.cc:371: // First try deleting it as a file, unless
we're told to do directory-only.
On 2015/03/06 13:01:04, qsr wrote:
> You do not use fstatat because of the race?

Right.

> 
> Also, do we allow to delete file on unknown type? Especially if
> kDeleteFlagFileOnly is set?

Hmmm, I guess kDeleteFlagFileOnly is meant to mimic the behavior of unlink() (or
unlinkat() without AT_REMOVEDIR). Possibly, it should be renamed to
|kDeleteFlagNotDirectory|, or something like that (name suggestions welcome).

(Or we could say that anything that's not a directory is a (possibly-special)
"file".)

https://codereview.chromium.org/963093004/diff/80001/services/files/files_app...
File services/files/files_apptest.cc (right):

https://codereview.chromium.org/963093004/diff/80001/services/files/files_app...
services/files/files_apptest.cc:612: // Rename my_file to my_new_file.
On 2015/03/06 13:01:04, qsr wrote:
> Not certain you want to test this, but you could pass a file request on the
> previous call, move the file, write on the returned file, and check that what
> you wrote is visible when you open the new file -> mainly checking that if you
> have a file, the physical one can move without impacting you.

I'll add that to the TODO list. :)

viettrungluu

5 years, 9 months ago (2015-03-09 18:11:18 UTC) #5

Message was sent while issue was closed.

Committed patchset #7 (id:120001) manually as
451ed4192f8561ecaa2a930c5a67ef00f0bca2f8 (presubmit successful).

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages