[Chromecast] Remove usage of nonreentrant functions.

[Chromecast] Remove usage of nonreetrant functions.

Remove usage of dmtime & readdir_r
Replace usage of time_t with base::Time::ToDoubleT()
Replace int error codes with bool error codes
Remove unix headers in favor of chromium-equivalent functions
These changes are applied to files in directory crash/linux/

BUG=internal b/27252596
TEST=builds, cast_crash_unittests

Committed: https://crrev.com/062511105e551e0e48f4a56e89b68c5c7b84672e
Cr-Commit-Position: refs/heads/master@{#410441}

[ameyak, 2016-08-02 19:36:32]

Description was changed from

==========
[Chromecast] Remove usage of nonreetrant functions.

Remove usage of dmtime & readdir_r
Replace usage of time_t with base::Time::ToDoubleT()
Replace int error codes with bool error codes
Remove unix headers in favor of chromium-equivalent functions
These changes are applied to files in directory crash/linux/

BUG=internal b/27252596
TEST=builds, cast_crash_unittests
==========

to

==========
[Chromecast] Remove usage of nonreetrant functions.

Remove usage of dmtime & readdir_r
Replace usage of time_t with base::Time::ToDoubleT()
Replace int error codes with bool error codes
Remove unix headers in favor of chromium-equivalent functions
These changes are applied to files in directory crash/linux/

BUG=internal b/27252596
TEST=builds, cast_crash_unittests
==========

[ameyak, 2016-08-02 19:38:33]

ameyak@google.com changed reviewers:
+ bcf@chromium.org

[ameyak, 2016-08-02 21:55:05]

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

[bcf, 2016-08-03 02:40:24]

https://codereview.chromium.org/2203123003/diff/40001/chromecast/base/file_ut...
File chromecast/base/file_utils.cc (right):

https://codereview.chromium.org/2203123003/diff/40001/chromecast/base/file_ut...
chromecast/base/file_utils.cc:16: LOG(ERROR) << "Error finding " << file << "
error = " << strerror(errno);
Error opening . open may fail for other reasons too.

https://codereview.chromium.org/2203123003/diff/40001/chromecast/base/file_ut...
chromecast/base/file_utils.cc:20: flag = flock(fd, flag);
Handle EINTR with TEMP_FAILURE_RETRY
https://www.gnu.org/software/libc/manual/html_node/Interrupted-Primitives.html

https://codereview.chromium.org/2203123003/diff/40001/chromecast/base/file_ut...
File chromecast/base/file_utils.h (right):

https://codereview.chromium.org/2203123003/diff/40001/chromecast/base/file_ut...
chromecast/base/file_utils.h:12: // Used for locking/unlocking files. This is a
blocking request. If the methods
Super nit: These aren't technically methods since they aren't class members.

https://codereview.chromium.org/2203123003/diff/40001/chromecast/crash/linux/...
File chromecast/crash/linux/dump_info.cc (left):

https://codereview.chromium.org/2203123003/diff/40001/chromecast/crash/linux/...
chromecast/crash/linux/dump_info.cc:49: valid_(false) {
This is the only place valid_ is set to false, I think we can just drop it.

https://codereview.chromium.org/2203123003/diff/40001/chromecast/crash/linux/...
File chromecast/crash/linux/synchronized_minidump_manager.cc (left):

https://codereview.chromium.org/2203123003/diff/40001/chromecast/crash/linux/...
chromecast/crash/linux/synchronized_minidump_manager.cc:14: #include
<sys/stat.h>
Can usage of stat.h, types,h, unistd.h be removed?

https://codereview.chromium.org/2203123003/diff/40001/chromecast/crash/linux/...
File chromecast/crash/linux/synchronized_minidump_manager.cc (right):

https://codereview.chromium.org/2203123003/diff/40001/chromecast/crash/linux/...
chromecast/crash/linux/synchronized_minidump_manager.cc:76:
base::StringToInt64(period_start_str, &result);
Check return value.

https://codereview.chromium.org/2203123003/diff/40001/chromecast/crash/linux/...
chromecast/crash/linux/synchronized_minidump_manager.cc:146: initialized(false)
{}
How about instead call it "lock_held_"

https://codereview.chromium.org/2203123003/diff/40001/chromecast/crash/linux/...
chromecast/crash/linux/synchronized_minidump_manager.cc:200: if
(!CreateDirectory(dump_path_)) {
This uses mode 0700. Can you make sure this works?
https://cs.chromium.org/chromium/src/base/files/file_util_posix.cc?sq=package...

[ameyak, 2016-08-03 18:50:33]

https://codereview.chromium.org/2203123003/diff/40001/chromecast/base/file_ut...
File chromecast/base/file_utils.cc (right):

https://codereview.chromium.org/2203123003/diff/40001/chromecast/base/file_ut...
chromecast/base/file_utils.cc:16: LOG(ERROR) << "Error finding " << file << "
error = " << strerror(errno);
On 2016/08/03 02:40:23, bcf wrote:
> Error opening . open may fail for other reasons too.

Done.

https://codereview.chromium.org/2203123003/diff/40001/chromecast/base/file_ut...
chromecast/base/file_utils.cc:20: flag = flock(fd, flag);
On 2016/08/03 02:40:23, bcf wrote:
> Handle EINTR with TEMP_FAILURE_RETRY
> https://www.gnu.org/software/libc/manual/html_node/Interrupted-Primitives.html

Done.

https://codereview.chromium.org/2203123003/diff/40001/chromecast/base/file_ut...
File chromecast/base/file_utils.h (right):

https://codereview.chromium.org/2203123003/diff/40001/chromecast/base/file_ut...
chromecast/base/file_utils.h:12: // Used for locking/unlocking files. This is a
blocking request. If the methods
On 2016/08/03 02:40:23, bcf wrote:
> Super nit: These aren't technically methods since they aren't class members.

Done. Technically correct is the best kind of correct. :)

https://codereview.chromium.org/2203123003/diff/40001/chromecast/crash/linux/...
File chromecast/crash/linux/dump_info.cc (left):

https://codereview.chromium.org/2203123003/diff/40001/chromecast/crash/linux/...
chromecast/crash/linux/dump_info.cc:49: valid_(false) {
On 2016/08/03 02:40:23, bcf wrote:
> This is the only place valid_ is set to false, I think we can just drop it.

Acknowledged. Ignored as discussed.

https://codereview.chromium.org/2203123003/diff/40001/chromecast/crash/linux/...
File chromecast/crash/linux/synchronized_minidump_manager.cc (left):

https://codereview.chromium.org/2203123003/diff/40001/chromecast/crash/linux/...
chromecast/crash/linux/synchronized_minidump_manager.cc:14: #include
<sys/stat.h>
On 2016/08/03 02:40:24, bcf wrote:
> Can usage of stat.h, types,h, unistd.h be removed?

time_t/size_t are required and defined in sys/types.h. I have attempted to
remove time.h functions (time(1), difftime(2)) in lieu of base::Time

sys/stat.h is no longer necessary and has been removed
unistd.h is no longer necessary and has been removed

https://codereview.chromium.org/2203123003/diff/40001/chromecast/crash/linux/...
File chromecast/crash/linux/synchronized_minidump_manager.cc (right):

https://codereview.chromium.org/2203123003/diff/40001/chromecast/crash/linux/...
chromecast/crash/linux/synchronized_minidump_manager.cc:76:
base::StringToInt64(period_start_str, &result);
On 2016/08/03 02:40:23, bcf wrote:
> Check return value.

Done. Overlooked that result can be changed even on a failure.

https://codereview.chromium.org/2203123003/diff/40001/chromecast/crash/linux/...
chromecast/crash/linux/synchronized_minidump_manager.cc:146: initialized(false)
{}
On 2016/08/03 02:40:24, bcf wrote:
> How about instead call it "lock_held_"

Done.

https://codereview.chromium.org/2203123003/diff/40001/chromecast/crash/linux/...
chromecast/crash/linux/synchronized_minidump_manager.cc:200: if
(!CreateDirectory(dump_path_)) {
On 2016/08/03 02:40:24, bcf wrote:
> This uses mode 0700. Can you make sure this works?
>
https://cs.chromium.org/chromium/src/base/files/file_util_posix.cc?sq=package...

Acknowledged. Seems ok, as discussed.

[bcf, 2016-08-03 20:45:10]

https://codereview.chromium.org/2203123003/diff/40001/chromecast/crash/linux/...
File chromecast/crash/linux/synchronized_minidump_manager.cc (left):

https://codereview.chromium.org/2203123003/diff/40001/chromecast/crash/linux/...
chromecast/crash/linux/synchronized_minidump_manager.cc:14: #include
<sys/stat.h>
On 2016/08/03 18:50:32, ameyak wrote:
> On 2016/08/03 02:40:24, bcf wrote:
> > Can usage of stat.h, types,h, unistd.h be removed?
> 
> time_t/size_t are required and defined in sys/types.h. I have attempted to
> remove time.h functions (time(1), difftime(2)) in lieu of base::Time
> 
> sys/stat.h is no longer necessary and has been removed
> unistd.h is no longer necessary and has been removed
> 

Can you try to remove time_t usage?

https://codereview.chromium.org/2203123003/diff/60001/chromecast/base/file_ut...
File chromecast/base/file_utils.cc (right):

https://codereview.chromium.org/2203123003/diff/60001/chromecast/base/file_ut...
chromecast/base/file_utils.cc:13: bool AttemptLockOnFileWithFlag(const char*
file, int flag) {
nit: pass in const base::FilePath& so you only have to call .value().c_str()
once.

https://codereview.chromium.org/2203123003/diff/60001/chromecast/base/file_ut...
chromecast/base/file_utils.cc:20: flag = TEMP_FAILURE_RETRY(flock(fd, flag));
You shouldn't reuse this variable for another purpose. Just use

int ret = TEMP_FAILURE_RETRY(flock(fd, flag));

https://codereview.chromium.org/2203123003/diff/60001/chromecast/base/file_ut...
chromecast/base/file_utils.cc:20: flag = TEMP_FAILURE_RETRY(flock(fd, flag));
Should log on error.

[ameyak, 2016-08-04 15:25:31]

https://codereview.chromium.org/2203123003/diff/40001/chromecast/crash/linux/...
File chromecast/crash/linux/synchronized_minidump_manager.cc (left):

https://codereview.chromium.org/2203123003/diff/40001/chromecast/crash/linux/...
chromecast/crash/linux/synchronized_minidump_manager.cc:14: #include
<sys/stat.h>
On 2016/08/03 20:45:10, bcf wrote:
> On 2016/08/03 18:50:32, ameyak wrote:
> > On 2016/08/03 02:40:24, bcf wrote:
> > > Can usage of stat.h, types,h, unistd.h be removed?
> > 
> > time_t/size_t are required and defined in sys/types.h. I have attempted to
> > remove time.h functions (time(1), difftime(2)) in lieu of base::Time
> > 
> > sys/stat.h is no longer necessary and has been removed
> > unistd.h is no longer necessary and has been removed
> > 
> 
> Can you try to remove time_t usage?

Done.

https://codereview.chromium.org/2203123003/diff/60001/chromecast/base/file_ut...
File chromecast/base/file_utils.cc (right):

https://codereview.chromium.org/2203123003/diff/60001/chromecast/base/file_ut...
chromecast/base/file_utils.cc:13: bool AttemptLockOnFileWithFlag(const char*
file, int flag) {
On 2016/08/03 20:45:10, bcf wrote:
> nit: pass in const base::FilePath& so you only have to call .value().c_str()
> once.

Done.

https://codereview.chromium.org/2203123003/diff/60001/chromecast/base/file_ut...
chromecast/base/file_utils.cc:20: flag = TEMP_FAILURE_RETRY(flock(fd, flag));
On 2016/08/03 20:45:10, bcf wrote:
> Should log on error.

Done.

https://codereview.chromium.org/2203123003/diff/60001/chromecast/base/file_ut...
chromecast/base/file_utils.cc:20: flag = TEMP_FAILURE_RETRY(flock(fd, flag));
On 2016/08/03 20:45:10, bcf wrote:
> You shouldn't reuse this variable for another purpose. Just use
> 
> int ret = TEMP_FAILURE_RETRY(flock(fd, flag));

Done.

[bcf, 2016-08-05 03:33:24]

https://codereview.chromium.org/2203123003/diff/80001/chromecast/crash/linux/...
File chromecast/crash/linux/synchronized_minidump_manager.cc (right):

https://codereview.chromium.org/2203123003/diff/80001/chromecast/crash/linux/...
chromecast/crash/linux/synchronized_minidump_manager.cc:291:
ratelimit_fields->SetDouble(kLockfileRatelimitPeriodStartKey, 0);
nit: 0.0 for double.

https://codereview.chromium.org/2203123003/diff/120001/chromecast/crash/linux...
File chromecast/crash/linux/synchronized_minidump_manager.cc (right):

https://codereview.chromium.org/2203123003/diff/120001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager.cc:11: #include
<sys/types.h>
This is unused now right?

https://codereview.chromium.org/2203123003/diff/120001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager.cc:72: return seconds ?
base::Time::FromDoubleT(seconds) : base::Time::UnixEpoch();
Add comment why this is necessary:

// 0 means "not initialized", so we need a special case.

https://codereview.chromium.org/2203123003/diff/120001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager.cc:194: LOG(ERROR) <<
"mkdir for " << dump_path_.value() << " failed.";
Nit:

We're not using mkdir anymore.

"Failed to create directory: ". Also lets use CreateDirectoryAndGetError so we
can keep the error message.

https://codereview.chromium.org/2203123003/diff/120001/chromecast/crash/linux...
File chromecast/crash/linux/synchronized_minidump_manager.h (right):

https://codereview.chromium.org/2203123003/diff/120001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager.h:64: int
AcquireLockAndDoWork();
Lets change all the the error codes to bool to be consistent with the rest of
Chromium.

https://codereview.chromium.org/2203123003/diff/120001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager.h:70: virtual int DoWork()
= 0;
In this case, I don't think its good design to have the derived class determine
the meaning of the return value. So bool error code here too.

https://codereview.chromium.org/2203123003/diff/120001/chromecast/crash/linux...
File chromecast/crash/linux/synchronized_minidump_manager_unittest.cc (right):

https://codereview.chromium.org/2203123003/diff/120001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager_unittest.cc:287:
TEST_F(SynchronizedMinidumpManagerTest,
Why are we keeping this around?

https://codereview.chromium.org/2203123003/diff/120001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager_unittest.cc:356:
TEST_F(SynchronizedMinidumpManagerTest,
Ditto

[bcf, 2016-08-05 03:33:35]

bcf@chromium.org changed reviewers:
+ slan@chromium.org

[ameyak, 2016-08-05 18:45:07]

https://codereview.chromium.org/2203123003/diff/80001/chromecast/crash/linux/...
File chromecast/crash/linux/synchronized_minidump_manager.cc (right):

https://codereview.chromium.org/2203123003/diff/80001/chromecast/crash/linux/...
chromecast/crash/linux/synchronized_minidump_manager.cc:291:
ratelimit_fields->SetDouble(kLockfileRatelimitPeriodStartKey, 0);
On 2016/08/05 03:33:23, bcf wrote:
> nit: 0.0 for double.

Done.

https://codereview.chromium.org/2203123003/diff/120001/chromecast/crash/linux...
File chromecast/crash/linux/synchronized_minidump_manager.cc (right):

https://codereview.chromium.org/2203123003/diff/120001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager.cc:11: #include
<sys/types.h>
On 2016/08/05 03:33:23, bcf wrote:
> This is unused now right?

We still used size_t, but I have removed its usage and replaced it with
uint64_t.

https://codereview.chromium.org/2203123003/diff/120001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager.cc:72: return seconds ?
base::Time::FromDoubleT(seconds) : base::Time::UnixEpoch();
On 2016/08/05 03:33:23, bcf wrote:
> Add comment why this is necessary:
> 
> // 0 means "not initialized", so we need a special case.

Done.

https://codereview.chromium.org/2203123003/diff/120001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager.cc:194: LOG(ERROR) <<
"mkdir for " << dump_path_.value() << " failed.";
On 2016/08/05 03:33:23, bcf wrote:
> Nit:
> 
> We're not using mkdir anymore.
> 
> "Failed to create directory: ". Also lets use CreateDirectoryAndGetError so we
> can keep the error message.

Done.

https://codereview.chromium.org/2203123003/diff/120001/chromecast/crash/linux...
File chromecast/crash/linux/synchronized_minidump_manager.h (right):

https://codereview.chromium.org/2203123003/diff/120001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager.h:64: int
AcquireLockAndDoWork();
On 2016/08/05 03:33:23, bcf wrote:
> Lets change all the the error codes to bool to be consistent with the rest of
> Chromium.

Done.

https://codereview.chromium.org/2203123003/diff/120001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager.h:70: virtual int DoWork()
= 0;
On 2016/08/05 03:33:23, bcf wrote:
> In this case, I don't think its good design to have the derived class
determine
> the meaning of the return value. So bool error code here too.

Done.

https://codereview.chromium.org/2203123003/diff/120001/chromecast/crash/linux...
File chromecast/crash/linux/synchronized_minidump_manager_unittest.cc (right):

https://codereview.chromium.org/2203123003/diff/120001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager_unittest.cc:287:
TEST_F(SynchronizedMinidumpManagerTest,
On 2016/08/05 03:33:23, bcf wrote:
> Why are we keeping this around?

Initial thought was it would be cleaner to just disable, assuming we could
salvage anything in the test. Looking at the actual content, it seems otherwise.

https://codereview.chromium.org/2203123003/diff/120001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager_unittest.cc:356:
TEST_F(SynchronizedMinidumpManagerTest,
On 2016/08/05 03:33:23, bcf wrote:
> Ditto

See comment above.

[slan, 2016-08-08 16:37:11]

Looks good, just nits.

https://codereview.chromium.org/2203123003/diff/140001/chromecast/base/file_u...
File chromecast/base/file_utils.cc (right):

https://codereview.chromium.org/2203123003/diff/140001/chromecast/base/file_u...
chromecast/base/file_utils.cc:13: bool CallFlockOnFileWithFlag(const int fd, int
flag) {
nit: Add a comment explaining retval.

https://codereview.chromium.org/2203123003/diff/140001/chromecast/base/file_u...
chromecast/base/file_utils.cc:14: int ret;
nit: init this.

https://codereview.chromium.org/2203123003/diff/140001/chromecast/base/file_u...
chromecast/base/file_utils.cc:16: LOG(ERROR) << "Error locking " << fd << "
error = " << strerror(errno);
PLOG would be appropriate here:

https://cs.chromium.org/chromium/src/base/logging.h?rcl=0&l=122

https://codereview.chromium.org/2203123003/diff/140001/chromecast/base/file_u...
chromecast/base/file_utils.cc:28: int fd;
nit: init this.

https://codereview.chromium.org/2203123003/diff/140001/chromecast/base/file_u...
chromecast/base/file_utils.cc:31: if ((fd = open(file, O_RDONLY)) < 0) {
The comment on this function doesn't indicate that this file is opened
read-only. Maybe change the name of the function to OpenAndLockReadOnlyFile? Or
pass a param in to indicate write permissions.

https://codereview.chromium.org/2203123003/diff/140001/chromecast/base/file_u...
chromecast/base/file_utils.cc:32: LOG(ERROR) << "Error opening " << file << "
error = " << strerror(errno);
nit: PLOG

https://codereview.chromium.org/2203123003/diff/140001/chromecast/base/file_u...
File chromecast/base/file_utils.h (right):

https://codereview.chromium.org/2203123003/diff/140001/chromecast/base/file_u...
chromecast/base/file_utils.h:15: int OpenAndLockFile(const base::FilePath&
path);
What is the expectation if the file does not exist?

https://codereview.chromium.org/2203123003/diff/140001/chromecast/crash/linux...
File chromecast/crash/linux/synchronized_minidump_manager.cc (right):

https://codereview.chromium.org/2203123003/diff/140001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager.cc:66: double seconds;
nit: init

https://codereview.chromium.org/2203123003/diff/140001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager.cc:173: LOG(INFO) <<
"remove failed. error " << strerror(errno);
PLOG

https://codereview.chromium.org/2203123003/diff/140001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager.cc:211: if ((lockfile_fd_ =
OpenAndLockFile(lockfile_path_)) < 0) {
This sets lockfile_fd_ if it fails. That seems like it may cause some havoc.
Maybe use a temp.

https://codereview.chromium.org/2203123003/diff/140001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager.cc:327:
chromecast::UnlockAndCloseFile(lockfile_fd_);
nit: ns qualifier not needed.

[ameyak, 2016-08-08 18:00:44]

https://codereview.chromium.org/2203123003/diff/140001/chromecast/base/file_u...
File chromecast/base/file_utils.cc (right):

https://codereview.chromium.org/2203123003/diff/140001/chromecast/base/file_u...
chromecast/base/file_utils.cc:13: bool CallFlockOnFileWithFlag(const int fd, int
flag) {
On 2016/08/08 16:37:10, slan wrote:
> nit: Add a comment explaining retval.

Done.

https://codereview.chromium.org/2203123003/diff/140001/chromecast/base/file_u...
chromecast/base/file_utils.cc:14: int ret;
On 2016/08/08 16:37:10, slan wrote:
> nit: init this.

Done.

https://codereview.chromium.org/2203123003/diff/140001/chromecast/base/file_u...
chromecast/base/file_utils.cc:16: LOG(ERROR) << "Error locking " << fd << "
error = " << strerror(errno);
On 2016/08/08 16:37:10, slan wrote:
> PLOG would be appropriate here:
> 
> https://cs.chromium.org/chromium/src/base/logging.h?rcl=0&l=122

Done.

https://codereview.chromium.org/2203123003/diff/140001/chromecast/base/file_u...
chromecast/base/file_utils.cc:28: int fd;
On 2016/08/08 16:37:10, slan wrote:
> nit: init this.

Done.

https://codereview.chromium.org/2203123003/diff/140001/chromecast/base/file_u...
chromecast/base/file_utils.cc:31: if ((fd = open(file, O_RDONLY)) < 0) {
On 2016/08/08 16:37:10, slan wrote:
> The comment on this function doesn't indicate that this file is opened
> read-only. Maybe change the name of the function to OpenAndLockReadOnlyFile?
Or
> pass a param in to indicate write permissions.

Done.

https://codereview.chromium.org/2203123003/diff/140001/chromecast/base/file_u...
chromecast/base/file_utils.cc:32: LOG(ERROR) << "Error opening " << file << "
error = " << strerror(errno);
On 2016/08/08 16:37:10, slan wrote:
> nit: PLOG

Done.

https://codereview.chromium.org/2203123003/diff/140001/chromecast/base/file_u...
File chromecast/base/file_utils.h (right):

https://codereview.chromium.org/2203123003/diff/140001/chromecast/base/file_u...
chromecast/base/file_utils.h:15: int OpenAndLockFile(const base::FilePath&
path);
On 2016/08/08 16:37:10, slan wrote:
> What is the expectation if the file does not exist?

Returns invalid file descriptor (-1)

https://codereview.chromium.org/2203123003/diff/140001/chromecast/crash/linux...
File chromecast/crash/linux/synchronized_minidump_manager.cc (right):

https://codereview.chromium.org/2203123003/diff/140001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager.cc:66: double seconds;
On 2016/08/08 16:37:10, slan wrote:
> nit: init

Done.

https://codereview.chromium.org/2203123003/diff/140001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager.cc:173: LOG(INFO) <<
"remove failed. error " << strerror(errno);
On 2016/08/08 16:37:11, slan wrote:
> PLOG

Done.

https://codereview.chromium.org/2203123003/diff/140001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager.cc:211: if ((lockfile_fd_ =
OpenAndLockFile(lockfile_path_)) < 0) {
On 2016/08/08 16:37:11, slan wrote:
> This sets lockfile_fd_ if it fails. That seems like it may cause some havoc.
> Maybe use a temp.

OpenAndLockFile will return -1, which will assign to |lockfile_fd_|. This is
desired, as -1 is the invalid state that is DCHECKed on all lockfile-related
accesses.

https://codereview.chromium.org/2203123003/diff/140001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager.cc:327:
chromecast::UnlockAndCloseFile(lockfile_fd_);
On 2016/08/08 16:37:10, slan wrote:
> nit: ns qualifier not needed.

Done.

[slan, 2016-08-08 18:32:19]

LGTM; please wait for LGTM from bcf@ too.

https://codereview.chromium.org/2203123003/diff/140001/chromecast/base/file_u...
File chromecast/base/file_utils.h (right):

https://codereview.chromium.org/2203123003/diff/140001/chromecast/base/file_u...
chromecast/base/file_utils.h:15: int OpenAndLockFile(const base::FilePath&
path);
On 2016/08/08 18:00:43, ameyak wrote:
> On 2016/08/08 16:37:10, slan wrote:
> > What is the expectation if the file does not exist?
> 
> Returns invalid file descriptor (-1)

Oh, it's right there in the comment. 
https://i.ytimg.com/vi/2hlu0qd1Huc/hqdefault.jpg

https://codereview.chromium.org/2203123003/diff/140001/chromecast/crash/linux...
File chromecast/crash/linux/synchronized_minidump_manager.cc (right):

https://codereview.chromium.org/2203123003/diff/140001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager.cc:211: if ((lockfile_fd_ =
OpenAndLockFile(lockfile_path_)) < 0) {
On 2016/08/08 18:00:43, ameyak wrote:
> On 2016/08/08 16:37:11, slan wrote:
> > This sets lockfile_fd_ if it fails. That seems like it may cause some havoc.
> > Maybe use a temp.
> 
> OpenAndLockFile will return -1, which will assign to |lockfile_fd_|. This is
> desired, as -1 is the invalid state that is DCHECKed on all lockfile-related
> accesses.

LGTM

[slan, 2016-08-08 18:40:37]

On 2016/08/08 18:32:19, slan wrote:
> LGTM; please wait for LGTM from bcf@ too.
> 

bcf@ is OOO; good to submit.

[ameyak, 2016-08-08 18:46:30]

The CQ bit was checked by ameyak@google.com

[commit-bot: I haz the power, 2016-08-08 18:46:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-08 18:53:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-08 18:53:53]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[ameyak, 2016-08-08 20:05:01]

The CQ bit was checked by ameyak@google.com

[commit-bot: I haz the power, 2016-08-08 20:05:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-08 20:34:31]

Description was changed from

==========
[Chromecast] Remove usage of nonreetrant functions.

Remove usage of dmtime & readdir_r
Replace usage of time_t with base::Time::ToDoubleT()
Replace int error codes with bool error codes
Remove unix headers in favor of chromium-equivalent functions
These changes are applied to files in directory crash/linux/

BUG=internal b/27252596
TEST=builds, cast_crash_unittests
==========

to

==========
[Chromecast] Remove usage of nonreetrant functions.

Remove usage of dmtime & readdir_r
Replace usage of time_t with base::Time::ToDoubleT()
Replace int error codes with bool error codes
Remove unix headers in favor of chromium-equivalent functions
These changes are applied to files in directory crash/linux/

BUG=internal b/27252596
TEST=builds, cast_crash_unittests
==========

[commit-bot: I haz the power, 2016-08-08 20:34:32]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-08-08 20:36:41]

Description was changed from

==========
[Chromecast] Remove usage of nonreetrant functions.

Remove usage of dmtime & readdir_r
Replace usage of time_t with base::Time::ToDoubleT()
Replace int error codes with bool error codes
Remove unix headers in favor of chromium-equivalent functions
These changes are applied to files in directory crash/linux/

BUG=internal b/27252596
TEST=builds, cast_crash_unittests
==========

to

==========
[Chromecast] Remove usage of nonreetrant functions.

Remove usage of dmtime & readdir_r
Replace usage of time_t with base::Time::ToDoubleT()
Replace int error codes with bool error codes
Remove unix headers in favor of chromium-equivalent functions
These changes are applied to files in directory crash/linux/

BUG=internal b/27252596
TEST=builds, cast_crash_unittests

Committed: https://crrev.com/062511105e551e0e48f4a56e89b68c5c7b84672e
Cr-Commit-Position: refs/heads/master@{#410441}
==========

[commit-bot: I haz the power, 2016-08-08 20:36:45]

Patchset 9 (id:??) landed as
https://crrev.com/062511105e551e0e48f4a56e89b68c5c7b84672e
Cr-Commit-Position: refs/heads/master@{#410441}