Crash reporter: collect suspend and resume info from power manager

unclean_shutdown_collector.cc

 // Copyright (c) 2010 The Chromium OS Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "crash-reporter/unclean_shutdown_collector.h"
 
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "crash-reporter/system_logging.h"
 
 static const char kUncleanShutdownFile[] =
     "/var/lib/crash_reporter/pending_clean_shutdown";
 
+// Files created by power manager used for crash reporting.
+static const FilePath kPowerdTracePath = FilePath("/var/lib/power_manager");

[tfarina, 2010/11/19 15:35:43]

Please, don't do this.
See
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

[tfarina, 2010/11/19 15:35:43]

Can you use FILE_PATH_LITERAL instead?

[kmixter1, 2010/11/19 20:02:02]

Fair enough, I retract my statement about doing this in power manager.

+// File with timestamp of last suspend.
+static const char kPowerdSuspendFile[] = "powerd_suspending";
+// File with timestamp of last resume.
+static const char kPowerdResumeFile[] = "powerd_resuming";
+// Presence of this file indicates that the battery is critically low.
+static const char kPowerdLowBatteryFile[] = "powerd_low_battery";
+
 UncleanShutdownCollector::UncleanShutdownCollector()
     : unclean_shutdown_file_(kUncleanShutdownFile) {
 }
 
 UncleanShutdownCollector::~UncleanShutdownCollector() {
 }
 
 bool UncleanShutdownCollector::Enable() {
   FilePath file_path(unclean_shutdown_file_);
   file_util::CreateDirectory(file_path.DirName());
@@ skipping 14 matching lines @@
 }
 
 bool UncleanShutdownCollector::Collect() {
   FilePath unclean_file_path(unclean_shutdown_file_);
   if (!file_util::PathExists(unclean_file_path)) {
     return false;
   }
   logger_->LogWarning("Last shutdown was not clean");
   DeleteUncleanShutdownFile();
 
+  // Check for unclean shutdown due to battery running out.

[tfarina, 2010/11/19 15:35:43]

This comment is good. I'd add it in the header file and remove it from here.

[kmixter1, 2010/11/19 20:02:02]

I'd name the function to indicate this: CheckForDeadBatteryUncleanShutdown (or
something similar)

+  CheckPowerdFiles();
+
   if (is_feedback_allowed_function_()) {
     count_crash_function_();
   }
   return true;
 }
 
 bool UncleanShutdownCollector::Disable() {
   logger_->LogInfo("Clean shutdown signalled");
+  file_util::Delete(kPowerdTracePath.Append(kPowerdSuspendFile), false);
+  file_util::Delete(kPowerdTracePath.Append(kPowerdResumeFile), false);
+  file_util::Delete(kPowerdTracePath.Append(kPowerdLowBatteryFile), false);
   return DeleteUncleanShutdownFile();
 }
+
+bool UncleanShutdownCollector::ReadModificationTime(const FilePath &file_path,
+                                                    base::Time *mod_time)
+{

[tfarina, 2010/11/19 15:35:43]

the { should be in the end of the previous line.

+  file_util::FileInfo file_info;

[tfarina, 2010/11/19 15:35:43]

Please, move this inside the first if, close where it's used.

+  base::Time::Exploded timestamp;

[tfarina, 2010/11/19 15:35:43]

Please, move this inside the if too.

+  // Initialize timestamp fields to prevent warnings during print.
+  timestamp.hour = -1;
+  timestamp.minute = -1;
+  timestamp.second = -1;
+  // Log the file modification time if it exists.
+  if (file_util::PathExists(file_path)) {

[tfarina, 2010/11/19 15:35:43]

You can return early. I think it's better.

if (!file_util::Path(file_path))
  return;

+    if (!GetFileInfo(file_path, &file_info))
+      return false;
+    file_info.last_modified.LocalExplode(&timestamp);
+    logger_->LogInfo("%s modified at %02d:%02d:%02d", file_path.value().c_str(),
+                     timestamp.hour, timestamp.minute, timestamp.second);
+    // Return the timestamp.
+    if (mod_time != NULL)
+      *mod_time = base::Time::FromLocalExploded(timestamp);
+    return true;
+  }
+  return false;
+}
+
+bool UncleanShutdownCollector::CheckPowerdFiles()
+{

[tfarina, 2010/11/19 15:35:43]

the { should be on the previous line.

+  logger_->LogInfo("Checking for Power Daemon files");

[kmixter1, 2010/11/19 20:02:02]

This seems pretty chatty as we already emit a few messages at every startup and
this one is not conditional.

+
+  base::Time suspend_time, resume_time, low_battery_time;
+  bool was_suspended = ReadModificationTime(kPowerdTracePath.
+                              Append(kPowerdSuspendFile), &suspend_time);

[tfarina, 2010/11/19 15:35:43]

wrong indentation. Indent 4 spaces. Better, if possible, align Append with
kPowerdTracePath, put the &suspend_time on the next line aligned with Append.
Same thing below.

+  bool was_resumed = ReadModificationTime(kPowerdTracePath.

[kmixter1, 2010/11/19 20:02:02]

I would move these down to where they're actually used.  Then you probably can
even remove this variable.

+                              Append(kPowerdResumeFile), &resume_time);
+  bool running_on_low_battery = ReadModificationTime(kPowerdTracePath.
+                              Append(kPowerdLowBatteryFile), &low_battery_time);
+  bool battery_ran_out = false;
+
+  if (was_suspended) {

[kmixter1, 2010/11/19 20:02:02]

Once the Delete calls are factored out you can make this be !was_suspended
return false.

+    // Check for case of battery running out while suspended.
+    if (!was_resumed || (was_resumed && resume_time < suspend_time)) {
+      logger_->LogInfo("Unclean shutdown occurred while suspended. The battery "
+                       "probably ran out.");
+      battery_ran_out = true;
+    // Check for case of battery running out after resuming from a low-battery
+    // suspend.
+    } else if (was_suspended && was_resumed && resume_time > suspend_time &&

[kmixter1, 2010/11/19 20:02:02]

I think the whole first line of this is redundant.  Do you need use (was_resumed
&& resume_time > suspend_time)?  It's required to be true by the else if.  Well,
sort of, it should be resume_time >= suspend_time but I assume that's a bug.

What about running_on_low_battery && suspend_time > low_battery_time &&
was_resumed && resume_time > suspend_time?  Doesn't that mean the battery ran
out while the device was resumed?  Why wouldn't we return true for that case.

And that that point, why wouldn't we just replace if (was_suspended) {...} with
return running_on_low_battery?

+               running_on_low_battery && low_battery_time <= suspend_time) {
+

[tfarina, 2010/11/19 15:35:43]

remove this blank line.

+      logger_->LogInfo("Unclean shutdown occurred while resumed after battery "
+                       "was critically low.  The battery probably ran out.");
+      battery_ran_out = true;
+    }
+  }
+
+  file_util::Delete(kPowerdTracePath.Append(kPowerdSuspendFile), false);

[kmixter1, 2010/11/19 20:02:02]

Please separate this out into a function with a name like
SignalPowerdTraceFileGeneration() and call it from Collect().  This helps avoid
a "Check/Get" function from having a side effect which is always nice.

Then you can also call this function instead of duplicating the code in
Disable(). ;)

+  file_util::Delete(kPowerdTracePath.Append(kPowerdResumeFile), false);
+  file_util::Delete(kPowerdTracePath.Append(kPowerdLowBatteryFile), false);
+
+  return battery_ran_out;
+}