Create ScopedTempDir, implement it on POSIX, and use it where appropriate.

Create ScopedTempDir, implement it on POSIX, and use it where appropriate.

R=mark@chromium.org
TEST=util_test --gtest_filter=ScopedTempDir.*

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/1cdb7c1d04abb4137678189f8bd15bd28ffbb3a8

[Robert Sesek, 2014-12-31 00:14:02]

Patchset #1 (id:1) has been deleted

[Robert Sesek, 2014-12-31 00:18:31]

Patchset #1 (id:20001) has been deleted

[Mark Mentovai, 2014-12-31 05:51:10]

Less code than I anticipated, nice!

https://codereview.chromium.org/834693002/diff/40001/util/mac/xattr_test.cc
File util/mac/xattr_test.cc (right):

https://codereview.chromium.org/834693002/diff/40001/util/mac/xattr_test.cc#n...
util/mac/xattr_test.cc:49: base::FilePath path() { return path_; }
Missed this on the last one: it can be const, and it can actually return a
const& too. And it can be protected.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_dir.h
File util/test/scoped_temp_dir.h (right):

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir.h:37: base::FilePath path() { return path_; }
This should be const and can return a const&.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir.h:42: base::FilePath CreateTemporaryDirectory();
This should be static. Remember to add “// static” to the .cc file.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir.h:47: //! Failures should be recorded as gtest
expectations.
These are comments for the caller, not the implementer, so:

should → are.
as → by (or via).

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir.h:49: //! \param[in] path The path and its contents to
delete. This must reference
“The path and its contents to delete” doesn’t make great sense. “The path to
delete, along with its contents” may be better.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir.h:51: void RecursivelyDeleteTemporaryDirectory(const
base::FilePath& path);
So can this.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir.h:53: base::FilePath path_;
Even this can be const.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
File util/test/scoped_temp_dir_posix.cc (right):

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_posix.cc:28: char dir_tempalate[] =
"/tmp/com.googlecode.crashpad.temp_dir.XXXXXX";
Test-specific things so far have used com.googlecode.crashpad.test as the
prefix.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_posix.cc:29: CHECK(mkdtemp(dir_tempalate)) <<
ErrnoMessage("mkdtemp");
PCHECK(), not CHECK(). Then you can lose the ErrnoMessage and just have <<
"mkdtemp".

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_posix.cc:29: CHECK(mkdtemp(dir_tempalate)) <<
ErrnoMessage("mkdtemp");
Also: add << " " << dir_template?

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_posix.cc:34: const base::FilePath& path) {
dir_path might further disambiguate from entry_path (and adjust the .h too,
then).

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_posix.cc:35: DIR* dir = opendir(path.value().c_str());
You ought to be able to make a scoper for this out of a ScopedGeneric really
easily. It can be local to this file for now.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_posix.cc:38: struct dirent* entry;
Can you just call this dirent* without the struct tag as we do elsewhere in
Crashpad C++?

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_posix.cc:40: base::FilePath entry_path =
path.Append(entry->d_name);
No point in doing this until after you’ve checked for "." and "..".

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_posix.cc:41: if (strcmp(entry->d_name, ".") == 0 ||
strcmp(entry->d_name, "..") == 0) {
#include <string.h>.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_posix.cc:48: EXPECT_EQ(0,
unlink(entry_path.value().c_str())) << ErrnoMessage("unlink")
The message would be easier to read if you put the entire thing on a single
continuation line. Same for the closedir() and rmdir().

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
File util/test/scoped_temp_dir_test_posix.cc (right):

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_test_posix.cc:1: // Copyright 2014 The Crashpad
Authors. All rights reserved.
I see why you named it this way, but I think that it should be
scoped_temp_dir_posix_test.cc, otherwise it’s hard to identify as a test by the
pattern *_test.*.

Alternatively, just call it scoped_temp_dir_test.cc, and abstract the mkdir()
and creat()/close() into something like CreateDirectory() and CreateFile() which
you implement for POSIX, alongside FileExists().

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_test_posix.cc:33: int rv = stat(path.value().c_str(),
&st);
I think it’s good to be in the habit of calling lstat() unless you’re really
sure you want to follow symbolic links. Here, you’re only concerned with
directory entries, so lstat() is more appropriate.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_test_posix.cc:35: EXPECT_EQ(ENOENT, errno) << "stat
expected ENOENT, got " << strerror(errno);
The message kinda repeats what the code says, except for the addition of the
strerror bit. Maybe just stream ErrnoMessage("stat") to the EXPECT.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_test_posix.cc:60: int fd =
HANDLE_EINTR(creat(file1.value().c_str(), 0644));
The ASSERT_GE(fd, 0) << ErrnoMessage("creat") is missing. But wait…

This is actually another reason to abstract this into to CreateFile(): you’ve
got this two-liner creat() and close() in four places now, and I’m talking about
turning it into a three-liner or more (if you EXPECT on close()’s return value
too, or if CreateFile() returns bool instead of void).

It’s good to can properly interpret the return value from creat() and only call
close() if the creat() succeeded. More importantly, it can also log a better
message with ErrnoMessage() if creat() fails. That will be more useful than just
letting FileExists() fail without any indication that creat() didn’t work or why
it didn’t work. You did the logging on the mkdir() below to handle the new
directory, you should do the same for the new files.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_test_posix.cc:62: ASSERT_TRUE(FileExists(file1));
This can be EXPECT_TRUE() since it isn’t a precondition for anything that
follows. Same for the check for file2.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_test_posix.cc:64: file2 = parent.Append("test 2");
Spaces! Happy to see that.

You should also test with a dot-file.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_test_posix.cc:86: ASSERT_TRUE(FileExists(file1));
Same here and on line 96. Line 91 needs to stay an ASSERT.

[Mark Mentovai, 2014-12-31 06:07:28]

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
File util/test/scoped_temp_dir_posix.cc (right):

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_posix.cc:46:
RecursivelyDeleteTemporaryDirectory(entry_path);
You should set entry to nullptr to affirmatively state that it may be unusable
once you make this call. That might also be worth an inline comment.

The other alternative is to use readdir_r() here, but I don’t think that’s
necessary at all.

The code you wrote isn’t wrong in any way, but this is a readdir() gotcha and
it’s good to protect against someone in the future trying to use entry after
it’s become bogus. (The compiler will optimize the nullptr assignment into
oblivion.)

[Mark Mentovai, 2015-01-01 06:40:20]

Happy new year!

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
File util/test/scoped_temp_dir_posix.cc (right):

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_posix.cc:46:
RecursivelyDeleteTemporaryDirectory(entry_path);
I wrote:
> You should set entry to nullptr to affirmatively state that it may be unusable
> once you make this call. That might also be worth an inline comment.
> 
> The other alternative is to use readdir_r() here, but I don’t think that’s
> necessary at all.
> 
> The code you wrote isn’t wrong in any way, but this is a readdir() gotcha and
> it’s good to protect against someone in the future trying to use entry after
> it’s become bogus. (The compiler will optimize the nullptr assignment into
> oblivion.)

Disregard per post-ball-drop discussion.

[Robert Sesek, 2015-01-02 17:34:09]

https://codereview.chromium.org/834693002/diff/40001/util/mac/xattr_test.cc
File util/mac/xattr_test.cc (right):

https://codereview.chromium.org/834693002/diff/40001/util/mac/xattr_test.cc#n...
util/mac/xattr_test.cc:49: base::FilePath path() { return path_; }
On 2014/12/31 05:51:09, Mark Mentovai wrote:
> Missed this on the last one: it can be const, and it can actually return a
> const& too. And it can be protected.

Done.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_dir.h
File util/test/scoped_temp_dir.h (right):

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir.h:37: base::FilePath path() { return path_; }
On 2014/12/31 05:51:09, Mark Mentovai wrote:
> This should be const and can return a const&.

Done.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir.h:42: base::FilePath CreateTemporaryDirectory();
On 2014/12/31 05:51:09, Mark Mentovai wrote:
> This should be static. Remember to add “// static” to the .cc file.

Done.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir.h:47: //! Failures should be recorded as gtest
expectations.
On 2014/12/31 05:51:09, Mark Mentovai wrote:
> These are comments for the caller, not the implementer, so:
> 
> should → are.
> as → by (or via).

Done.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir.h:49: //! \param[in] path The path and its contents to
delete. This must reference
On 2014/12/31 05:51:09, Mark Mentovai wrote:
> “The path and its contents to delete” doesn’t make great sense. “The path
to
> delete, along with its contents” may be better.

Done.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir.h:51: void RecursivelyDeleteTemporaryDirectory(const
base::FilePath& path);
On 2014/12/31 05:51:09, Mark Mentovai wrote:
> So can this.

Done.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir.h:53: base::FilePath path_;
On 2014/12/31 05:51:09, Mark Mentovai wrote:
> Even this can be const.

Done.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
File util/test/scoped_temp_dir_posix.cc (right):

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_posix.cc:28: char dir_tempalate[] =
"/tmp/com.googlecode.crashpad.temp_dir.XXXXXX";
On 2014/12/31 05:51:09, Mark Mentovai wrote:
> Test-specific things so far have used com.googlecode.crashpad.test as the
> prefix.

Done.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_posix.cc:29: CHECK(mkdtemp(dir_tempalate)) <<
ErrnoMessage("mkdtemp");
On 2014/12/31 05:51:09, Mark Mentovai wrote:
> Also: add << " " << dir_template?

Done.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_posix.cc:29: CHECK(mkdtemp(dir_tempalate)) <<
ErrnoMessage("mkdtemp");
On 2014/12/31 05:51:09, Mark Mentovai wrote:
> PCHECK(), not CHECK(). Then you can lose the ErrnoMessage and just have <<
> "mkdtemp".

Done.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_posix.cc:35: DIR* dir = opendir(path.value().c_str());
On 2014/12/31 05:51:10, Mark Mentovai wrote:
> You ought to be able to make a scoper for this out of a ScopedGeneric really
> easily. It can be local to this file for now.

I tried this, but it's more code and I don't think it helps in any significant
way.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_posix.cc:38: struct dirent* entry;
On 2014/12/31 05:51:10, Mark Mentovai wrote:
> Can you just call this dirent* without the struct tag as we do elsewhere in
> Crashpad C++?

Done, though my preference is to keep "struct" when referencing C structures
from C++.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_posix.cc:40: base::FilePath entry_path =
path.Append(entry->d_name);
On 2014/12/31 05:51:09, Mark Mentovai wrote:
> No point in doing this until after you’ve checked for "." and "..".

Done.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_posix.cc:41: if (strcmp(entry->d_name, ".") == 0 ||
strcmp(entry->d_name, "..") == 0) {
On 2014/12/31 05:51:09, Mark Mentovai wrote:
> #include <string.h>.

Done.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_posix.cc:48: EXPECT_EQ(0,
unlink(entry_path.value().c_str())) << ErrnoMessage("unlink")
On 2014/12/31 05:51:09, Mark Mentovai wrote:
> The message would be easier to read if you put the entire thing on a single
> continuation line. Same for the closedir() and rmdir().

Done.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
File util/test/scoped_temp_dir_test_posix.cc (right):

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_test_posix.cc:1: // Copyright 2014 The Crashpad
Authors. All rights reserved.
On 2014/12/31 05:51:10, Mark Mentovai wrote:
> I see why you named it this way, but I think that it should be
> scoped_temp_dir_posix_test.cc, otherwise it’s hard to identify as a test by
the
> pattern *_test.*.
> 
> Alternatively, just call it scoped_temp_dir_test.cc, and abstract the mkdir()
> and creat()/close() into something like CreateDirectory() and CreateFile()
which
> you implement for POSIX, alongside FileExists().

Done.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_test_posix.cc:33: int rv = stat(path.value().c_str(),
&st);
On 2014/12/31 05:51:10, Mark Mentovai wrote:
> I think it’s good to be in the habit of calling lstat() unless you’re really
> sure you want to follow symbolic links. Here, you’re only concerned with
> directory entries, so lstat() is more appropriate.

Done.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_test_posix.cc:35: EXPECT_EQ(ENOENT, errno) << "stat
expected ENOENT, got " << strerror(errno);
On 2014/12/31 05:51:10, Mark Mentovai wrote:
> The message kinda repeats what the code says, except for the addition of the
> strerror bit. Maybe just stream ErrnoMessage("stat") to the EXPECT.

Done.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_test_posix.cc:60: int fd =
HANDLE_EINTR(creat(file1.value().c_str(), 0644));
On 2014/12/31 05:51:10, Mark Mentovai wrote:
> The ASSERT_GE(fd, 0) << ErrnoMessage("creat") is missing. But wait…
> 
> This is actually another reason to abstract this into to CreateFile(): you’ve
> got this two-liner creat() and close() in four places now, and I’m talking
about
> turning it into a three-liner or more (if you EXPECT on close()’s return value
> too, or if CreateFile() returns bool instead of void).
> 
> It’s good to can properly interpret the return value from creat() and only
call
> close() if the creat() succeeded. More importantly, it can also log a better
> message with ErrnoMessage() if creat() fails. That will be more useful than
just
> letting FileExists() fail without any indication that creat() didn’t work or
why
> it didn’t work. You did the logging on the mkdir() below to handle the new
> directory, you should do the same for the new files.

Done.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_test_posix.cc:62: ASSERT_TRUE(FileExists(file1));
On 2014/12/31 05:51:10, Mark Mentovai wrote:
> This can be EXPECT_TRUE() since it isn’t a precondition for anything that
> follows. Same for the check for file2.

Done.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_test_posix.cc:64: file2 = parent.Append("test 2");
On 2014/12/31 05:51:10, Mark Mentovai wrote:
> Spaces! Happy to see that.
> 
> You should also test with a dot-file.

Done.

https://codereview.chromium.org/834693002/diff/40001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_test_posix.cc:86: ASSERT_TRUE(FileExists(file1));
On 2014/12/31 05:51:10, Mark Mentovai wrote:
> Same here and on line 96. Line 91 needs to stay an ASSERT.

Done.

[Mark Mentovai, 2015-01-02 17:49:55]

LGTM

https://codereview.chromium.org/834693002/diff/60001/util/mac/xattr_test.cc
File util/mac/xattr_test.cc (right):

https://codereview.chromium.org/834693002/diff/60001/util/mac/xattr_test.cc#n...
util/mac/xattr_test.cc:38: void SetUp() override {
SetUp() and TearDown() can be protected too.

https://codereview.chromium.org/834693002/diff/60001/util/test/scoped_temp_di...
File util/test/scoped_temp_dir_posix.cc (right):

https://codereview.chromium.org/834693002/diff/60001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_posix.cc:51: EXPECT_EQ(0,
unlink(entry_path.value().c_str())) << ErrnoMessage("unlink")
Put everything from << to the ; onto its own continuation line. Reads better
that way.

https://codereview.chromium.org/834693002/diff/60001/util/test/scoped_temp_di...
File util/test/scoped_temp_dir_test.cc (right):

https://codereview.chromium.org/834693002/diff/60001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_test.cc:36: EXPECT_EQ(ENOENT, errno) <<
ErrnoMessage("stat") << " " << path.value();
lstat

[Robert Sesek, 2015-01-02 18:51:13]

https://codereview.chromium.org/834693002/diff/60001/util/mac/xattr_test.cc
File util/mac/xattr_test.cc (right):

https://codereview.chromium.org/834693002/diff/60001/util/mac/xattr_test.cc#n...
util/mac/xattr_test.cc:38: void SetUp() override {
On 2015/01/02 17:49:54, Mark Mentovai wrote:
> SetUp() and TearDown() can be protected too.

Done.

https://codereview.chromium.org/834693002/diff/60001/util/test/scoped_temp_di...
File util/test/scoped_temp_dir_posix.cc (right):

https://codereview.chromium.org/834693002/diff/60001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_posix.cc:51: EXPECT_EQ(0,
unlink(entry_path.value().c_str())) << ErrnoMessage("unlink")
On 2015/01/02 17:49:54, Mark Mentovai wrote:
> Put everything from << to the ; onto its own continuation line. Reads better
> that way.

Done.

https://codereview.chromium.org/834693002/diff/60001/util/test/scoped_temp_di...
File util/test/scoped_temp_dir_test.cc (right):

https://codereview.chromium.org/834693002/diff/60001/util/test/scoped_temp_di...
util/test/scoped_temp_dir_test.cc:36: EXPECT_EQ(ENOENT, errno) <<
ErrnoMessage("stat") << " " << path.value();
On 2015/01/02 17:49:55, Mark Mentovai wrote:
> lstat

Done.

[Robert Sesek, 2015-01-02 18:51:51]

Committed patchset #3 (id:80001) manually as
1cdb7c1d04abb4137678189f8bd15bd28ffbb3a8 (presubmit successful).