shill: cleanups and logging
Fix a few small issues I noticed while working on suspend-resume
issues with PPP dongles, and add some logging that helps understand
the suspend/resume process.
More specifically:
- Add some override annotations to cellular.h
- Fix a typo in device.h
- Update a stale comment in device.h
- Add some logging for termination actions. This uses scope Manager
for HookTable. It's conceivable that some other component will
use HookTable as well. But let's worry about that when it happens.
- Remove obsolete headers (FRIEND_TEST_ALL_PREFIXES)
- Add comment for Cellular::ModemState
BUG=None
TEST=unit tests
Change-Id: I94761c2e5e8df304d1d32794bbf7069c359a1cda
Reviewed-on: https://chromium-review.googlesource.com/173755
Reviewed-by: mukesh agrawal <quiche@chromium.org>
Commit-Queue: mukesh agrawal <quiche@chromium.org>
Tested-by: mukesh agrawal <quiche@chromium.org>
diff --git a/cellular.h b/cellular.h
index 8adfe17..56ecf21 100644
--- a/cellular.h
+++ b/cellular.h
@@ -10,7 +10,6 @@
#include <base/basictypes.h>
#include <base/memory/weak_ptr.h>
-#include <base/gtest_prod_util.h> // for FRIEND_TEST_ALL_PREFIXES
#include <gtest/gtest_prod.h> // for FRIEND_TEST
#include "shill/dbus_properties.h"
@@ -59,6 +58,7 @@
kStateLinked,
};
+ // This enum must be kept in sync with ModemManager's MMModemState enum.
enum ModemState {
kModemStateFailed = -1,
kModemStateUnknown = 0,
@@ -176,27 +176,32 @@
const std::vector<std::string> &invalidated_properties);
// Inherited from Device.
- virtual void Start(Error *error, const EnabledStateChangedCallback &callback);
- virtual void Stop(Error *error, const EnabledStateChangedCallback &callback);
- virtual void LinkEvent(unsigned int flags, unsigned int change);
+ virtual void Start(Error *error, const EnabledStateChangedCallback &callback)
+ override;
+ virtual void Stop(Error *error, const EnabledStateChangedCallback &callback)
+ override;
+ virtual void LinkEvent(unsigned int flags, unsigned int change) override;
virtual void Scan(ScanType /*scan_type*/, Error *error,
- const std::string &/*reason*/);
+ const std::string &/*reason*/) override;
virtual void RegisterOnNetwork(const std::string &network_id,
Error *error,
- const ResultCallback &callback);
+ const ResultCallback &callback) override;
virtual void RequirePIN(const std::string &pin, bool require,
- Error *error, const ResultCallback &callback);
+ Error *error, const ResultCallback &callback)
+ override;
virtual void EnterPIN(const std::string &pin,
- Error *error, const ResultCallback &callback);
+ Error *error, const ResultCallback &callback) override;
virtual void UnblockPIN(const std::string &unblock_code,
const std::string &pin,
- Error *error, const ResultCallback &callback);
+ Error *error, const ResultCallback &callback)
+ override;
virtual void ChangePIN(const std::string &old_pin,
const std::string &new_pin,
- Error *error, const ResultCallback &callback);
- virtual void Reset(Error *error, const ResultCallback &callback);
+ Error *error, const ResultCallback &callback) override;
+ virtual void Reset(Error *error, const ResultCallback &callback) override;
virtual void SetCarrier(const std::string &carrier,
- Error *error, const ResultCallback &callback);
+ Error *error, const ResultCallback &callback)
+ override;
virtual void DropConnection() override;
virtual void SetServiceState(Service::ConnectState state) override;
virtual void SetServiceFailure(Service::ConnectFailure failure_state)
diff --git a/cellular_capability_classic.h b/cellular_capability_classic.h
index ff6cfb1..58b3152 100644
--- a/cellular_capability_classic.h
+++ b/cellular_capability_classic.h
@@ -12,7 +12,6 @@
#include <base/callback.h>
#include <base/memory/scoped_ptr.h>
#include <base/memory/weak_ptr.h>
-#include <base/gtest_prod_util.h> // for FRIEND_TEST_ALL_PREFIXES
#include <gtest/gtest_prod.h> // for FRIEND_TEST
#include "shill/cellular.h"
diff --git a/device.h b/device.h
index cda523f..d6f89ca 100644
--- a/device.h
+++ b/device.h
@@ -69,7 +69,7 @@
// cases where we want to SetEnabledNonPersistent, but don't care
// about the results.
virtual void SetEnabled(bool enable);
- // Enable or disable the device. Unlikely SetEnabledPersistent, it does not
+ // Enable or disable the device. Unlike SetEnabledPersistent, it does not
// save the setting in the profile.
//
// TODO(quiche): Replace both of the next two methods with calls to
@@ -224,12 +224,12 @@
}
// Suspend event handler. Called by Manager before the system
- // suspends. For this callback to be useful, the device must specify
- // a suspend delay. Otherwise, there is no guarantee that the device
- // will have time to complete its suspend actions, before the system
- // is suspended.
+ // suspends. This handler, along with any other suspect handlers,
+ // will have Manager::kTerminationActionsTimeoutMilliseconds to
+ // execute before the system enters the suspend state.
//
- // TODO(quiche): Add support for suspend delays. crbug.com/215582
+ // Code that needs to run on exit, as well as on suspend, should use
+ // Manager::AddTerminationAction, rather than OnBeforeSuspend.
virtual void OnBeforeSuspend();
// Resume event handler. Called by Manager as the system resumes.
diff --git a/hook_table.cc b/hook_table.cc
index d04842b..6d23569 100644
--- a/hook_table.cc
+++ b/hook_table.cc
@@ -12,6 +12,7 @@
#include "shill/error.h"
#include "shill/event_dispatcher.h"
+#include "shill/logging.h"
using base::Bind;
using base::Callback;
@@ -43,6 +44,7 @@
}
void HookTable::ActionComplete(const std::string &name) {
+ SLOG(Manager, 2) << __func__ << ": " << name;
HookTableMap::iterator it = hook_table_.find(name);
if (it != hook_table_.end()) {
HookAction *action = &it->second;
@@ -59,6 +61,7 @@
void HookTable::Run(int timeout_ms,
const Callback<void(const Error &)> &done) {
+ SLOG(Manager, 2) << __func__;
if (hook_table_.empty()) {
done.Run(Error(Error::kSuccess));
return;
@@ -85,6 +88,7 @@
}
bool HookTable::AllActionsComplete() {
+ SLOG(Manager, 2) << __func__;
for (HookTableMap::const_iterator it = hook_table_.begin();
it != hook_table_.end(); ++it) {
const HookAction &action = it->second;
diff --git a/manager.cc b/manager.cc
index 922f7e4..7d0cc47 100644
--- a/manager.cc
+++ b/manager.cc
@@ -1090,16 +1090,19 @@
}
void Manager::TerminationActionComplete(const string &name) {
+ SLOG(Manager, 2) << __func__;
termination_actions_.ActionComplete(name);
}
void Manager::RemoveTerminationAction(const string &name) {
+ SLOG(Manager, 2) << __func__;
if (termination_actions_.IsEmpty()) {
return;
}
termination_actions_.Remove(name);
if (termination_actions_.IsEmpty() && power_manager_.get()) {
if (suspend_delay_registered_) {
+ SLOG(Manager, 2) << "Unregistering suspend delay.";
power_manager_->UnregisterSuspendDelay(suspend_delay_id_);
suspend_delay_registered_ = false;
suspend_delay_id_ = 0;
diff --git a/manager.h b/manager.h
index 6d07dec..d268553 100644
--- a/manager.h
+++ b/manager.h
@@ -15,7 +15,6 @@
#include <base/memory/scoped_ptr.h>
#include <base/memory/weak_ptr.h>
#include <chromeos/dbus/service_constants.h>
-#include <base/gtest_prod_util.h> // for FRIEND_TEST_ALL_PREFIXES
#include <gtest/gtest_prod.h> // for FRIEND_TEST
#include "shill/crypto_util_proxy.h"