Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1392653002:
Add functionality to prune old crash reports from the database. (Closed)
|
Created:
5 years, 2 months ago by Robert Sesek Modified:
5 years, 2 months ago CC:
crashpad-dev_chromium.org Base URL:
https://chromium.googlesource.com/crashpad/crashpad@master Target Ref:
refs/heads/master Project:
crashpad Visibility:
Public. |
DescriptionAdd functionality to prune old crash reports from the database.
BUG=crashpad:22
R=mark@chromium.org
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/f32ca63a91d9db18cc9751dd42ca015534d24afb
Patch Set 1 #
Total comments: 62
Patch Set 2 : Address comments #
Total comments: 2
Patch Set 3 : Additional comments #
Total comments: 10
Patch Set 4 : Round up to 1k #
Total comments: 4
Patch Set 5 : #
Messages
Total messages: 15 (2 generated)
I don't have a Windows box to work on, so this isn't tested there. But it's got an implementation.
scottmg@chromium.org changed reviewers: + scottmg@chromium.org
I didn't look at the code, but with the below fixed the tests pass on Windows. https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... client/crash_report_database_win.cc:378: return kNoError; This needs to be CrashReportDatabase::kNoError. https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... client/crash_report_database_win.cc:759: OperationStatus CrashReportDatabaseWin::DeleteReport(const UUID& uuid) { There's no declaration for this method. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.cc File client/prune_crash_reports.cc (right): https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.cc:113: } On Windows: d:\src\crashpad\crashpad\client\prune_crash_reports.cc(113) : warning C4715: 'crashpad::BinaryPruneCondition::ShouldPruneReport' : not all control paths return a value https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports_... File client/prune_crash_reports_test.cc (right): https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports_... client/prune_crash_reports_test.cc:143: string.push_back(i); warning converting int to char here on Windows. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports_... client/prune_crash_reports_test.cc:243: return arc4random() % rand_max; arc4random doesn't exist on Windows.
https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports_... File client/prune_crash_reports_test.cc (right): https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports_... client/prune_crash_reports_test.cc:243: return arc4random() % rand_max; On 2015/10/06 22:22:07, scottmg wrote: > arc4random doesn't exist on Windows. (Should have mentioned we have base/rand_util.h which is RtlGenRandom()-based on Windows.)
https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... File client/crash_report_database_mac.mm (right): https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... client/crash_report_database_mac.mm:492: lock.reset(); What’s the thinking behind releasing the lock *before* the unlink? Seems dangerous. Someone else can come in and take the lock and then this function unlinks the file out from under it. https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... File client/crash_report_database_test.cc (right): https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... client/crash_report_database_test.cc:540: EXPECT_TRUE(FileExistsAtPath(keep_pending.file_path)); FileExistsAtPath() is going away in https://codereview.chromium.org/1390023002/. You can change these to FileExists() after that lands, if that goes in before this. https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... client/crash_report_database_test.cc:571: } Make sure the deletes didn’t delete beyond their scopes: test that the you can still LookUpCrashReport() both of the “keep” reports, and that both of their files still exist. https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... client/crash_report_database_win.cc:243: CrashReportDatabase::Report* report); Returning a CrashReportDatabase::Report for something that you’re dumping the metadata for seems a little bogus. How about if this passed back just the FilePath that the caller will delete? https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... client/crash_report_database_win.cc:774: return CrashReportDatabase::kFileSystemError; You don’t need CrashReportDatabase:: here. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.cc File client/prune_crash_reports.cc (right): https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.cc:31: status = database->GetPendingReports(&temp); Why don’t you fetch this right into all_reports? Then you can avoid the insert here, avoid declaring temp until you use it to get completed reports below, and rename temp to completed_reports to make it more accurate. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.cc:33: LOG(ERROR) << "Database Pruning: Failed to get pending reports"; Local style is predominantly “PruneCrashReportDatabase: failed whatever blah” https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.cc:69: : oldest_report_time_(time(nullptr) - (max_age_in_days * 60 * 60 * 24)) {} mod this whole thing to a round UTC day? https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.cc:89: struct _stat statbuf; _stati64 for a full-sized st_size. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.cc:90: if (_wstat(report.file_path.value().c_str(), &statbuf) == 0) { _wstati64 https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.h File client/prune_crash_reports.h (right): https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:31: //! sorted in descending by CrashReportDatabase::Report::creation_time. This in descending what? https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:35: //! \param[in] condition The condition by which all reports in the database by → against? https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:40: //! \brief Returns a sensible default condition for removing obsolete crash This should actually say what the condition is in the body (doesn’t have to be in the \brief). https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:44: scoped_ptr<PruneCondition> GetDefaultDatabasePruneCondition(); Maybe make this a static function, PruneCondition::GetDefault()? https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:68: //! \brief Creates a PruneCondition based on Report::creation_time. CrashReportDatabase::Report::creation_time https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:78: time_t oldest_report_time_; const https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:89: //! \a max_size_in_bytes. Clarify that after that, it will prune. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:92: //! crash reports should consume. Duplicate “crash crash.” https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:93: explicit DatabaseSizePruneCondition(size_t max_size_in_bytes); “Bytes” is probably not the best choice for a unit here, it’s too easy to overflow (in 32-bit-land, at least). You used “days” for AgePruneCondition, how about kilobytes here? https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:93: explicit DatabaseSizePruneCondition(size_t max_size_in_bytes); <sys/types.h> for size_t. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:108: enum Operator { enum class is hot and should work just fine in this application. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:116: //! \param[in] op The logical operator to apply on \a lhs and \a rhs. Specify whether short-circuiting is implemented. It is. (Is that what you want? Seems like it could mess up tabulation of DatabaseSizePruneCondition::measured_size_. The exact behavior seems like something that you ought to have a test for, too.) https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:125: Operator op_; These data members can be const. Well, maybe lhs_ and rhs_ can’t be. But op_ can. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports_... File client/prune_crash_reports_test.cc (right): https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports_... client/prune_crash_reports_test.cc:31: class TestDatabase : public CrashReportDatabase { I don’t know if gmock would make the TestDatabase or this test any easier to deal with.
Thanks for testing this on Windows, Scott! https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... File client/crash_report_database_mac.mm (right): https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... client/crash_report_database_mac.mm:492: lock.reset(); On 2015/10/07 03:54:27, Mark Mentovai wrote: > What’s the thinking behind releasing the lock *before* the unlink? Seems > dangerous. Someone else can come in and take the lock and then this function > unlinks the file out from under it. I didn't expect to be able to unlink() with an flock on the file, but it looks like you can. https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... File client/crash_report_database_test.cc (right): https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... client/crash_report_database_test.cc:540: EXPECT_TRUE(FileExistsAtPath(keep_pending.file_path)); On 2015/10/07 03:54:27, Mark Mentovai wrote: > FileExistsAtPath() is going away in https://codereview.chromium.org/1390023002/. > You can change these to FileExists() after that lands, if that goes in before > this. Done. https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... client/crash_report_database_test.cc:571: } On 2015/10/07 03:54:27, Mark Mentovai wrote: > Make sure the deletes didn’t delete beyond their scopes: test that the you can > still LookUpCrashReport() both of the “keep” reports, and that both of their > files still exist. Done. https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... client/crash_report_database_win.cc:243: CrashReportDatabase::Report* report); On 2015/10/07 03:54:27, Mark Mentovai wrote: > Returning a CrashReportDatabase::Report for something that you’re dumping the > metadata for seems a little bogus. How about if this passed back just the > FilePath that the caller will delete? Done. https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... client/crash_report_database_win.cc:378: return kNoError; On 2015/10/06 22:22:07, scottmg wrote: > This needs to be CrashReportDatabase::kNoError. Done. https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... client/crash_report_database_win.cc:759: OperationStatus CrashReportDatabaseWin::DeleteReport(const UUID& uuid) { On 2015/10/06 22:22:07, scottmg wrote: > There's no declaration for this method. Done. https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... client/crash_report_database_win.cc:774: return CrashReportDatabase::kFileSystemError; On 2015/10/07 03:54:27, Mark Mentovai wrote: > You don’t need CrashReportDatabase:: here. Done. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.cc File client/prune_crash_reports.cc (right): https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.cc:31: status = database->GetPendingReports(&temp); On 2015/10/07 03:54:27, Mark Mentovai wrote: > Why don’t you fetch this right into all_reports? Then you can avoid the insert > here, avoid declaring temp until you use it to get completed reports below, and > rename temp to completed_reports to make it more accurate. Must be empty on entry. http://docs.crashpad.googlecode.com/git/doxygen/classcrashpad_1_1CrashReportD... https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.cc:33: LOG(ERROR) << "Database Pruning: Failed to get pending reports"; On 2015/10/07 03:54:27, Mark Mentovai wrote: > Local style is predominantly “PruneCrashReportDatabase: failed whatever blah” Done. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.cc:69: : oldest_report_time_(time(nullptr) - (max_age_in_days * 60 * 60 * 24)) {} On 2015/10/07 03:54:27, Mark Mentovai wrote: > mod this whole thing to a round UTC day? Done. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.cc:89: struct _stat statbuf; On 2015/10/07 03:54:27, Mark Mentovai wrote: > _stati64 for a full-sized st_size. Done. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.cc:90: if (_wstat(report.file_path.value().c_str(), &statbuf) == 0) { On 2015/10/07 03:54:27, Mark Mentovai wrote: > _wstati64 Done. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.cc:113: } On 2015/10/06 22:22:07, scottmg wrote: > On Windows: > > d:\src\crashpad\crashpad\client\prune_crash_reports.cc(113) : warning C4715: > 'crashpad::BinaryPruneCondition::ShouldPruneReport' : not all control paths > return a value Done. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.h File client/prune_crash_reports.h (right): https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:31: //! sorted in descending by CrashReportDatabase::Report::creation_time. This On 2015/10/07 03:54:28, Mark Mentovai wrote: > in descending what? Done. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:35: //! \param[in] condition The condition by which all reports in the database On 2015/10/07 03:54:28, Mark Mentovai wrote: > by → against? Done. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:40: //! \brief Returns a sensible default condition for removing obsolete crash On 2015/10/07 03:54:28, Mark Mentovai wrote: > This should actually say what the condition is in the body (doesn’t have to be > in the \brief). Didn't know if we wanted to define that in the API docs or not. Done. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:44: scoped_ptr<PruneCondition> GetDefaultDatabasePruneCondition(); On 2015/10/07 03:54:28, Mark Mentovai wrote: > Maybe make this a static function, PruneCondition::GetDefault()? Done. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:68: //! \brief Creates a PruneCondition based on Report::creation_time. On 2015/10/07 03:54:28, Mark Mentovai wrote: > CrashReportDatabase::Report::creation_time Done. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:78: time_t oldest_report_time_; On 2015/10/07 03:54:28, Mark Mentovai wrote: > const Done. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:89: //! \a max_size_in_bytes. On 2015/10/07 03:54:28, Mark Mentovai wrote: > Clarify that after that, it will prune. Done. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:92: //! crash reports should consume. On 2015/10/07 03:54:28, Mark Mentovai wrote: > Duplicate “crash crash.” Done. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:93: explicit DatabaseSizePruneCondition(size_t max_size_in_bytes); On 2015/10/07 03:54:28, Mark Mentovai wrote: > <sys/types.h> for size_t. Done. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:93: explicit DatabaseSizePruneCondition(size_t max_size_in_bytes); On 2015/10/07 03:54:28, Mark Mentovai wrote: > “Bytes” is probably not the best choice for a unit here, it’s too easy to > overflow (in 32-bit-land, at least). You used “days” for AgePruneCondition, how > about kilobytes here? Done. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:108: enum Operator { On 2015/10/07 03:54:28, Mark Mentovai wrote: > enum class is hot and should work just fine in this application. Sure, but I think it'd be more verbose than necessary here. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:116: //! \param[in] op The logical operator to apply on \a lhs and \a rhs. On 2015/10/07 03:54:28, Mark Mentovai wrote: > Specify whether short-circuiting is implemented. > > It is. (Is that what you want? Seems like it could mess up tabulation of > DatabaseSizePruneCondition::measured_size_. The exact behavior seems like > something that you ought to have a test for, too.) Done. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.h:125: Operator op_; On 2015/10/07 03:54:28, Mark Mentovai wrote: > These data members can be const. Well, maybe lhs_ and rhs_ can’t be. But op_ > can. Done. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports_... File client/prune_crash_reports_test.cc (right): https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports_... client/prune_crash_reports_test.cc:31: class TestDatabase : public CrashReportDatabase { On 2015/10/07 03:54:28, Mark Mentovai wrote: > I don’t know if gmock would make the TestDatabase or this test any easier to > deal with. Gmock is never easy to deal with. But it is less code. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports_... client/prune_crash_reports_test.cc:143: string.push_back(i); On 2015/10/06 22:22:07, scottmg wrote: > warning converting int to char here on Windows. Done. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports_... client/prune_crash_reports_test.cc:243: return arc4random() % rand_max; On 2015/10/06 22:35:29, scottmg wrote: > On 2015/10/06 22:22:07, scottmg wrote: > > arc4random doesn't exist on Windows. > > (Should have mentioned we have base/rand_util.h which is RtlGenRandom()-based on > Windows.) Done.
https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... File client/crash_report_database_mac.mm (right): https://codereview.chromium.org/1392653002/diff/1/client/crash_report_databas... client/crash_report_database_mac.mm:492: lock.reset(); Robert Sesek wrote: > I didn't expect to be able to unlink() with an flock on the file, but it looks > like you can. Yes. POSIX file semantics are cool. https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.cc File client/prune_crash_reports.cc (right): https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.cc:31: status = database->GetPendingReports(&temp); On 2015/10/07 16:24:34, Robert Sesek wrote: > On 2015/10/07 03:54:27, Mark Mentovai wrote: > > Why don’t you fetch this right into all_reports? Then you can avoid the insert > > here, avoid declaring temp until you use it to get completed reports below, > and > > rename temp to completed_reports to make it more accurate. > > Must be empty on entry. It will be. 1. Fetch pending into all_reports. 2. Fetch completed into completed_reports. 3. Insert completed_reports into all_reports. https://codereview.chromium.org/1392653002/diff/20001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/1392653002/diff/20001/client/crash_report_dat... client/crash_report_database_win.cc:591: // TODO(scottmg): When are completed reports pruned from disk? Delete here or Remove this comment.
https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.cc File client/prune_crash_reports.cc (right): https://codereview.chromium.org/1392653002/diff/1/client/prune_crash_reports.... client/prune_crash_reports.cc:31: status = database->GetPendingReports(&temp); On 2015/10/07 16:40:13, Mark Mentovai wrote: > On 2015/10/07 16:24:34, Robert Sesek wrote: > > On 2015/10/07 03:54:27, Mark Mentovai wrote: > > > Why don’t you fetch this right into all_reports? Then you can avoid the > insert > > > here, avoid declaring temp until you use it to get completed reports below, > > and > > > rename temp to completed_reports to make it more accurate. > > > > Must be empty on entry. > > It will be. > > 1. Fetch pending into all_reports. > 2. Fetch completed into completed_reports. > 3. Insert completed_reports into all_reports. Done. https://codereview.chromium.org/1392653002/diff/20001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/1392653002/diff/20001/client/crash_report_dat... client/crash_report_database_win.cc:591: // TODO(scottmg): When are completed reports pruned from disk? Delete here or On 2015/10/07 16:40:13, Mark Mentovai wrote: > Remove this comment. Done.
LGTM https://codereview.chromium.org/1392653002/diff/40001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/1392653002/diff/40001/client/crash_report_dat... client/crash_report_database_win.cc:1: // Copyright 2015 The Crashpad Authors. All rights reserved. Another thing that occurred to me, which can either be fixed under crashpad:22 or its own bug: If the metadata file gets trashed, we’ll have orphaned reports sitting around on Windows. We should have something dump all orphaned reports. https://codereview.chromium.org/1392653002/diff/40001/client/prune_crash_repo... File client/prune_crash_reports.cc (right): https://codereview.chromium.org/1392653002/diff/40001/client/prune_crash_repo... client/prune_crash_reports.cc:46: std::sort(all_reports.begin(), all_reports.end(), Just noting it here so that there’s evidence that we talked about it: I thought we might want to prefer pruning completed reports while there are pending reports still hanging around, possibly by sorting the sub-lists individually and then merging them. The rationale was “why should we prune something we might still try to use while there are still things we’re more certainly done with that we can toss?” Robert didn’t seem very compelled, and eventually I just shrugged. https://codereview.chromium.org/1392653002/diff/40001/client/prune_crash_repo... client/prune_crash_reports.cc:71: const time_t kSecondsInDay = 60 * 60 * 24; namespace {} https://codereview.chromium.org/1392653002/diff/40001/client/prune_crash_repo... client/prune_crash_reports.cc:103: return (measured_size_ / 1024.0f) > max_size_in_kb_; What’s with the floating-point domain? How about you switch to measured_size_in_kb_ (can be the same type as max_size_in_kb_), and you tabulate it as measured_size_in_kb_ += (statbuf.st_size + 1023) / 1024? (Any fractional kilobyte is rounded up to the next whole kilobyte.) https://codereview.chromium.org/1392653002/diff/40001/client/prune_crash_repo... File client/prune_crash_reports.h (right): https://codereview.chromium.org/1392653002/diff/40001/client/prune_crash_repo... client/prune_crash_reports.h:73: class AgePruneCondition : public PruneCondition { Age, DatabaseSize, and Binary PruneConditions can be final. StaticPruneCondition in the test, too.
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/1392653002/diff/40001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/1392653002/diff/40001/client/crash_report_dat... client/crash_report_database_win.cc:1: // Copyright 2015 The Crashpad Authors. All rights reserved. On 2015/10/07 19:13:05, Mark Mentovai wrote: > Another thing that occurred to me, which can either be fixed under crashpad:22 > or its own bug: > > If the metadata file gets trashed, we’ll have orphaned reports sitting around on > Windows. We should have something dump all orphaned reports. That seems like a separate bug. https://code.google.com/p/crashpad/issues/detail?id=66 https://codereview.chromium.org/1392653002/diff/40001/client/prune_crash_repo... File client/prune_crash_reports.cc (right): https://codereview.chromium.org/1392653002/diff/40001/client/prune_crash_repo... client/prune_crash_reports.cc:71: const time_t kSecondsInDay = 60 * 60 * 24; On 2015/10/07 19:13:05, Mark Mentovai wrote: > namespace {} static https://codereview.chromium.org/1392653002/diff/40001/client/prune_crash_repo... client/prune_crash_reports.cc:103: return (measured_size_ / 1024.0f) > max_size_in_kb_; On 2015/10/07 19:13:05, Mark Mentovai wrote: > What’s with the floating-point domain? > > How about you switch to measured_size_in_kb_ (can be the same type as > max_size_in_kb_), and you tabulate it as measured_size_in_kb_ += > (statbuf.st_size + 1023) / 1024? (Any fractional kilobyte is rounded up to the > next whole kilobyte.) Floating point was to promote to float before to round at the devision. Per extensive offline discussion about rounding, we'll just over-estimate to nearest 1k. https://codereview.chromium.org/1392653002/diff/40001/client/prune_crash_repo... File client/prune_crash_reports.h (right): https://codereview.chromium.org/1392653002/diff/40001/client/prune_crash_repo... client/prune_crash_reports.h:73: class AgePruneCondition : public PruneCondition { On 2015/10/07 19:13:05, Mark Mentovai wrote: > Age, DatabaseSize, and Binary PruneConditions can be final. StaticPruneCondition > in the test, too. Done.
FYI built + tested on Win10
LGTM https://codereview.chromium.org/1392653002/diff/40001/client/prune_crash_repo... File client/prune_crash_reports.cc (right): https://codereview.chromium.org/1392653002/diff/40001/client/prune_crash_repo... client/prune_crash_reports.cc:71: const time_t kSecondsInDay = 60 * 60 * 24; Robert Sesek wrote: > On 2015/10/07 19:13:05, Mark Mentovai wrote: > > namespace {} > > static That’s cool too. https://codereview.chromium.org/1392653002/diff/80001/client/prune_crash_repo... File client/prune_crash_reports.cc (right): https://codereview.chromium.org/1392653002/diff/80001/client/prune_crash_repo... client/prune_crash_reports.cc:61: } Lose a TODO, gain a TODO. Can you leave behind a breadcrumb pointing to your newly-filed https://code.google.com/p/crashpad/issues/detail?id=66? https://codereview.chromium.org/1392653002/diff/80001/client/prune_crash_repo... client/prune_crash_reports.cc:103: (static_cast<size_t>(statbuf.st_size) + 1023) / 1024; size_t may be narrower than decltype(statbuf.st_size). If you need to cast, can you do it after dividing?
https://codereview.chromium.org/1392653002/diff/80001/client/prune_crash_repo... File client/prune_crash_reports.cc (right): https://codereview.chromium.org/1392653002/diff/80001/client/prune_crash_repo... client/prune_crash_reports.cc:61: } On 2015/10/07 20:58:23, Mark Mentovai wrote: > Lose a TODO, gain a TODO. Can you leave behind a breadcrumb pointing to your > newly-filed https://code.google.com/p/crashpad/issues/detail?id=66? Done. https://codereview.chromium.org/1392653002/diff/80001/client/prune_crash_repo... client/prune_crash_reports.cc:103: (static_cast<size_t>(statbuf.st_size) + 1023) / 1024; On 2015/10/07 20:58:23, Mark Mentovai wrote: > size_t may be narrower than decltype(statbuf.st_size). If you need to cast, can > you do it after dividing? Done.
Message was sent while issue was closed.
Committed patchset #5 (id:100001) manually as f32ca63a91d9db18cc9751dd42ca015534d24afb (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
