implement things needed to run net/disk_cache/backend_unittest

implement things needed to run unittests of disk_cache backend

[please use my chromium address, 2008-09-11 19:34:39]

After this net_unittests run 216 tests on Linux (previously: 180).

Hopefully I guessed reviewers correctly.

[Mark Mentovai, 2008-09-11 20:27:06]

http://codereview.chromium.org/2441/diff/1/4
File base/file_util_linux.cc (right):

http://codereview.chromium.org/2441/diff/1/4#newcode74
Line 74: bool CopyDirectory(const std::wstring& from_path, const std::wstring&
to_path,
It's unspecified as to whether this routine should copy file attributes and
metadata or just data.  The header is silent on the matter.  It appears to me,
although I'm not positive, that the Windows implementation here does copy
attributes, but the Windows implementation of CopyFile does not.  I don't really
know what the right thing to do in this case is.  We should at least be explicit
that the different platform implementations do different things by tacking
comments above each platform's implementation of these two functions, if we're
not willing to define a proper cross-platform behavior.

You might want to ask around about this.

Maybe Erik has thoughts in this area too.

http://codereview.chromium.org/2441/diff/1/4#newcode76
Line 76: if (!PathExists(to_path)) {
Call CreateDirectory instead of doing this.

http://codereview.chromium.org/2441/diff/1/4#newcode77
Line 77: if (mkdir(WideToUTF8(to_path).c_str(), 0755) != 0) {
(moot if you're using CreateDirectory) this should be 0777 as we do elsewhere,
to let the umask do its job.  If we decide to back off of that mode, we should
do it globally, there's no reason for this routine to be an outlier.

http://codereview.chromium.org/2441/diff/1/4#newcode78
Line 78: return false;
This function is defined in the header as:

// Copies the given path, and optionally all subdirectories and their contents
// as well.
// If there are files existing under to_path, always overwrite.
// Returns true if successful, false otherwise.
// Dont't use wildcards on the names, it may stop working without notice.

Ignoring the last line (because it's total nonsense outside of Windows), it
seems like we've missed the "if there are files existing" bit here by returning
early if the directory already exists.  You'd hit the same problem if you call
CreateDirectory and check its return value.

I'm not convinced that the documented behavior is the correct behavior.  You
might want to ask around to find out more about that, or again, maybe Erik has
something to say.

http://codereview.chromium.org/2441/diff/1/4#newcode81
Line 81: FileEnumerator enumerator(from_path, false, FileEnumerator::FILES);
I'd add a TODO here about gracefully handling symlinks.  It doesn't look like
the rest of the code is up to snuff on that front yet either.

http://codereview.chromium.org/2441/diff/1/4#newcode86
Line 86: from.append(L"/");
Could this be AppendToPath?

http://codereview.chromium.org/2441/diff/1/4#newcode90
Line 90: to.append(L"/");
This too?

http://codereview.chromium.org/2441/diff/1/4#newcode94
Line 94: // there will be leftover files if some have been successfully copied
Complete proper sentence with capitalization and a period, please.

http://codereview.chromium.org/2441/diff/1/4#newcode98
Line 98: if (recursive) {
I'd like to see a little vertical whitespace before this block.  Makes it clear
that we're getting into a new section of code.  One blank line is plenty.

Now that I think about it and look back above, I'd leave a blank line right
before you instantiate the first FileEnumerator too, for the same reason.

http://codereview.chromium.org/2441/diff/1/4#newcode99
Line 99: FileEnumerator enumerator(from_path, false,
FileEnumerator::DIRECTORIES);
Does this enumerator give us entries for . and ..?

http://codereview.chromium.org/2441/diff/1/4#newcode102
Line 102: path = GetFilenameFromPath(path); // somewhat hacky
Style nit: 2 spaces between code and comment on the same line. 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem...

How is this hacky when done here?

http://codereview.chromium.org/2441/diff/1/4#newcode104
Line 104: from.append(L"/");
AppendToPath?

http://codereview.chromium.org/2441/diff/1/4#newcode108
Line 108: to.append(L"/");
AppendToPath?

http://codereview.chromium.org/2441/diff/1/4#newcode112
Line 112: // there will be leftovers if some have been successfully copied
leftover whats?

http://codereview.chromium.org/2441/diff/1/4#newcode117
Line 117: return true;
...and for completeness, a blank line above this one.

http://codereview.chromium.org/2441/diff/1/3
File base/file_util_unittest.cc (left):

http://codereview.chromium.org/2441/diff/1/3#oldcode403
Line 403: #if defined(OS_WIN)
Can you leave this at #if !defined(OS_MACOSX) until I can do a Mac
implementation?  You'll break the Mac build otherwise, since your patch doesn't
add a CopyDirectory implementation to it.

(It would have been possible to implement your CopyDirectory function in
file_util_posix.cc, but we can use copyfile to handle directory copies on the
Mac too, and I'd rather just do that, in much the same way that I'd rather use
copyfile for the single-file copy case.)

[please use my chromium address, 2008-09-12 15:45:40]

On 2008/09/11 20:27:06, Mark Mentovai wrote:
> http://codereview.chromium.org/2441/diff/1/4
> File base/file_util_linux.cc (right):
> 
> http://codereview.chromium.org/2441/diff/1/4#newcode74
> Line 74: bool CopyDirectory(const std::wstring& from_path, const std::wstring&
> to_path,
> It's unspecified as to whether this routine should copy file attributes and
> metadata or just data.  The header is silent on the matter.  It appears to me,
> although I'm not positive, that the Windows implementation here does copy
> attributes, but the Windows implementation of CopyFile does not.  I don't
really
> know what the right thing to do in this case is.  We should at least be
explicit
> that the different platform implementations do different things by tacking
> comments above each platform's implementation of these two functions, if we're
> not willing to define a proper cross-platform behavior.
> 
> You might want to ask around about this.
> 
> Maybe Erik has thoughts in this area too.

I thought about that. Added comments in file_util.h and specific in
file_util_linux.cc.

> http://codereview.chromium.org/2441/diff/1/4#newcode76
> Line 76: if (!PathExists(to_path)) {
> Call CreateDirectory instead of doing this.

Done.

> http://codereview.chromium.org/2441/diff/1/4#newcode78
> Line 78: return false;
> This function is defined in the header as:
> 
> // Copies the given path, and optionally all subdirectories and their contents
> // as well.
> // If there are files existing under to_path, always overwrite.
> // Returns true if successful, false otherwise.
> // Dont't use wildcards on the names, it may stop working without notice.
> 
> Ignoring the last line (because it's total nonsense outside of Windows), it
> seems like we've missed the "if there are files existing" bit here by
returning
> early if the directory already exists.  You'd hit the same problem if you call
> CreateDirectory and check its return value.
> 

Comment in file_util.h says that CreateDirectory will return true when target
exists. Should be ok.

> http://codereview.chromium.org/2441/diff/1/4#newcode81
> Line 81: FileEnumerator enumerator(from_path, false, FileEnumerator::FILES);
> I'd add a TODO here about gracefully handling symlinks.  It doesn't look like
> the rest of the code is up to snuff on that front yet either.

That's true. Done.

> http://codereview.chromium.org/2441/diff/1/4#newcode99
> Line 99: FileEnumerator enumerator(from_path, false,
> FileEnumerator::DIRECTORIES);
> Does this enumerator give us entries for . and ..?

No, I checked with DLOG.

> http://codereview.chromium.org/2441/diff/1/4#newcode102
> Line 102: path = GetFilenameFromPath(path); // somewhat hacky
> Style nit: 2 spaces between code and comment on the same line. 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem...
> 
> How is this hacky when done here?

I removed comment, but basically I use GetFilenameFromPath (note the "Filename")
here to get last component of path (which is directory, not file). So the
function name could be slightly misleading.

Other issues fixed as well. New changes in net/SConscript are there because gcl
picked up results of gclient sync. I think it's ok.

[Erik does not do reviews, 2008-09-12 16:20:02]

Unfortunately, there's one big gotcha in this implementation.  The header file
doesn't specify this (sigh), but the Windows version of the code uses
SHFileOperation which supports MS-DOS wildcards in the filename position of
from_path:
http://msdn.microsoft.com/en-us/library/bb759795(VS.85).aspx
"Standard Microsoft MS-DOS wildcard characters, such as "*", are permitted only
in the file-name position. Using a wildcard character elsewhere in the string
will lead to unpredictable results."

Also unfortunately, at least some of our code depends on this behavior (see
chrome/browser/importer_unittest.cc - although this one is dumb and can be
removed).

Of course all of this is made worse by the fact that file_util_unittest.cc
doesn't exercise this behavior itself.

We have a couple of options here:
(1) Fully support the behavior of the windows platform as-is on Linux and Mac. 
Extend the unit tests to exercise this, update the header files for clarity.
(2) Track down the users of CopyDirectory, remove any use of wildcards and
change the Windows impl to detect wildcards and return an error if they're used
(there doesn't appear to be a flag to SHFileOperation to enforce this, so we'd
have to manually check).

(1) doesn't look particularly hard, but I'm not convinced we want / need to
support this feature in the API.
(2) might not be that bad either since it looks like there aren't that many
consumers of the API.  However, it would definitely require a fair amount of
testing to convince ourselves that we didn't break anything.

Opinions?

[rvargas (doing something else), 2008-09-15 17:44:17]

Actually, there is a comment on the header warning users against using wildcard
characters when using this API. At some point I fixed all calls that were using
wildcards so fixing new ones (if any) should be easy.

Maybe we could add code on debug mode to verify this if we want to prevent
further appearances of that pattern.

As a somewhat related note, it would be nice if all implementations do  more or
less the same thing regarding metadata. As far as I remember, the windows
implementation copies as much metadata as reasonable: the result of a copy is
after all a new file, so the owner (and security related) information of the new
file reflects the user that is actually performing the copy (as opposed to a
move operation, when all security info is preserved).

On 2008/09/12 16:20:02, Erik Kay wrote:
> Unfortunately, there's one big gotcha in this implementation.  The header file
> doesn't specify this (sigh), but the Windows version of the code uses
> SHFileOperation which supports MS-DOS wildcards in the filename position of
> from_path:
> http://msdn.microsoft.com/en-us/library/bb759795(VS.85).aspx
> "Standard Microsoft MS-DOS wildcard characters, such as "*", are permitted
only
> in the file-name position. Using a wildcard character elsewhere in the string
> will lead to unpredictable results."
> 
> Also unfortunately, at least some of our code depends on this behavior (see
> chrome/browser/importer_unittest.cc - although this one is dumb and can be
> removed).
> 
> Of course all of this is made worse by the fact that file_util_unittest.cc
> doesn't exercise this behavior itself.
> 
> We have a couple of options here:
> (1) Fully support the behavior of the windows platform as-is on Linux and Mac.

> Extend the unit tests to exercise this, update the header files for clarity.
> (2) Track down the users of CopyDirectory, remove any use of wildcards and
> change the Windows impl to detect wildcards and return an error if they're
used
> (there doesn't appear to be a flag to SHFileOperation to enforce this, so we'd
> have to manually check).
> 
> (1) doesn't look particularly hard, but I'm not convinced we want / need to
> support this feature in the API.
> (2) might not be that bad either since it looks like there aren't that many
> consumers of the API.  However, it would definitely require a fair amount of
> testing to convince ourselves that we didn't break anything.
> 
> Opinions?