Skip to content

Commit

Permalink
[Chromecast] Change LockFileHasDumps to HasDumps.
Browse files Browse the repository at this point in the history
We want derived classes to run DoWork when there are entries in the
lockfile OR if there are any extraneous files in the dump directory.

BUG=internal b/25147415

Review URL: https://codereview.chromium.org/1417143002

Cr-Commit-Position: refs/heads/master@{#355686}
  • Loading branch information
bcf authored and Commit bot committed Oct 23, 2015
1 parent fc3dec8 commit 87a3ba8
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 15 deletions.
23 changes: 20 additions & 3 deletions chromecast/crash/linux/synchronized_minidump_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <time.h>
#include <unistd.h>

#include "base/files/dir_reader_posix.h"
#include "base/files/file_util.h"
#include "base/logging.h"
#include "base/strings/string_split.h"
Expand Down Expand Up @@ -403,13 +404,29 @@ bool SynchronizedMinidumpManager::CanUploadDump() {
return period_dumps_count < kRatelimitPeriodMaxDumps;
}

bool SynchronizedMinidumpManager::LockFileHasDumps() {
bool SynchronizedMinidumpManager::HasDumps() {
// Check if lockfile has entries.
int64 size = 0;
if (!GetFileSize(base::FilePath(lockfile_path_), &size)) {
if (GetFileSize(base::FilePath(lockfile_path_), &size) && size > 0)
return true;

// Check if any files are in minidump directory
base::DirReaderPosix reader(dump_path_.value().c_str());
if (!reader.IsValid()) {
DLOG(FATAL) << "Could not open minidump dir: " << dump_path_.value();
return false;
}

return size > 0;
while (reader.Next()) {
if (strcmp(reader.name(), ".") == 0 || strcmp(reader.name(), "..") == 0)
continue;

const std::string file_path = dump_path_.Append(reader.name()).value();
if (file_path != lockfile_path_ && file_path != metadata_path_)
return true;
}

return false;
}

} // namespace chromecast
5 changes: 3 additions & 2 deletions chromecast/crash/linux/synchronized_minidump_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,10 @@ class SynchronizedMinidumpManager {
// elapsed.
bool CanUploadDump();

// Returns true when there are dumps in the lockfile, false otherwise.
// Returns true when there are dumps in the lockfile or extra files in the
// dump directory, false otherwise.
// Used to avoid unnecessary file locks in consumers.
bool LockFileHasDumps();
bool HasDumps();

// If true, the flock on the lockfile will be nonblocking.
bool non_blocking_;
Expand Down
32 changes: 22 additions & 10 deletions chromecast/crash/linux/synchronized_minidump_manager_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ class SynchronizedMinidumpManagerSimple : public SynchronizedMinidumpManager {
}

// Accessors for testing.
bool HasDumps() { return SynchronizedMinidumpManager::HasDumps(); }
const std::string& dump_path() { return dump_path_.value(); }
const std::string& lockfile_path() { return lockfile_path_; }
bool work_done() { return work_done_; }
Expand Down Expand Up @@ -100,11 +101,8 @@ class FakeSynchronizedMinidumpUploader : public SynchronizedMinidumpManager {
return 0;
}

bool LockFileHasDumps() {
return SynchronizedMinidumpManager::LockFileHasDumps();
}

// Accessors for testing.
bool HasDumps() { return SynchronizedMinidumpManager::HasDumps(); }
bool can_upload_return_val() { return can_upload_return_val_; }

private:
Expand Down Expand Up @@ -509,12 +507,12 @@ TEST_F(SynchronizedMinidumpManagerTest, UploadSucceedsAfterRateLimitPeriodEnd) {
consume_dumps(uploader, 1);
}

TEST_F(SynchronizedMinidumpManagerTest, LockFileHasDumpsWithoutDumps) {
TEST_F(SynchronizedMinidumpManagerTest, HasDumpsWithoutDumps) {
FakeSynchronizedMinidumpUploader uploader;
ASSERT_FALSE(uploader.LockFileHasDumps());
ASSERT_FALSE(uploader.HasDumps());
}

TEST_F(SynchronizedMinidumpManagerTest, LockFileHasDumpsWithDumps) {
TEST_F(SynchronizedMinidumpManagerTest, HasDumpsWithDumps) {
// Sample parameters.
time_t now = time(0);
MinidumpParams params;
Expand All @@ -529,15 +527,29 @@ TEST_F(SynchronizedMinidumpManagerTest, LockFileHasDumpsWithDumps) {
const int kNumDumps = 3;
for (int i = 0; i < kNumDumps; ++i) {
produce_dumps(producer, 1);
ASSERT_TRUE(uploader.LockFileHasDumps());
ASSERT_TRUE(uploader.HasDumps());
}

for (int i = 0; i < kNumDumps; ++i) {
ASSERT_TRUE(uploader.LockFileHasDumps());
ASSERT_TRUE(uploader.HasDumps());
consume_dumps(uploader, 1);
}

ASSERT_FALSE(uploader.LockFileHasDumps());
ASSERT_FALSE(uploader.HasDumps());
}

TEST_F(SynchronizedMinidumpManagerTest, HasDumpsNotInLockFile) {
SynchronizedMinidumpManagerSimple manager;
ASSERT_FALSE(manager.HasDumps());

// Create file in dump path.
const base::FilePath path =
base::FilePath(manager.dump_path()).Append("hello123");
const char kFileContents[] = "foobar";
ASSERT_EQ(static_cast<int>(sizeof(kFileContents)),
WriteFile(path, kFileContents, sizeof(kFileContents)));

ASSERT_TRUE(manager.HasDumps());
}

} // namespace chromecast

0 comments on commit 87a3ba8

Please sign in to comment.