shill: Framework for asynchronous service side RPC calls.
Use the framework to switch EnterPIN to return asynchronously.
Also, added a few try/catch clauses around DBus proxy calls to ease testing and
debugging. They should go away as we transition to asynchronous proxy calls.
BUG=chromium-os:17263
TEST=unit tests, tested on device
Change-Id: I4177c5b91e23c31838b03689de729932c859a936
Reviewed-on: https://gerrit.chromium.org/gerrit/12541
Tested-by: Darin Petkov <petkov@chromium.org>
Reviewed-by: mukesh agrawal <quiche@chromium.org>
Reviewed-by: Eric Shienbrood <ers@chromium.org>
Commit-Ready: Darin Petkov <petkov@chromium.org>
diff --git a/adaptor_interfaces.h b/adaptor_interfaces.h
index 640053f..70098fe 100644
--- a/adaptor_interfaces.h
+++ b/adaptor_interfaces.h
@@ -14,6 +14,8 @@
namespace shill {
+class Error;
+
// These are the functions that a Device adaptor must support
class DeviceAdaptorInterface {
public:
@@ -113,5 +115,18 @@
const std::string &value) = 0;
};
+// A ReturnerInterface instance (along with its ownership) is passed by the
+// adaptor to the method handler. The handler releases ownership and initiates
+// an RPC return by calling one of the Return* methods.
+class ReturnerInterface {
+ public:
+ virtual void Return() = 0;
+ virtual void ReturnError(const Error &error) = 0;
+
+ protected:
+ // Destruction happens through the Return* methods.
+ virtual ~ReturnerInterface() {}
+};
+
} // namespace shill
#endif // SHILL_ADAPTOR_INTERFACES_
diff --git a/cellular.cc b/cellular.cc
index e5d8070..3158f4b 100644
--- a/cellular.cc
+++ b/cellular.cc
@@ -201,7 +201,11 @@
void Cellular::EnableModem() {
CHECK_EQ(kStateDisabled, state_);
// TODO(petkov): Switch to asynchronous calls (crosbug.com/17583).
- proxy_->Enable(true);
+ try {
+ proxy_->Enable(true);
+ } catch (const DBus::Error e) {
+ LOG(WARNING) << "Enable failed: " << e.what();
+ }
SetState(kStateEnabled);
}
@@ -239,8 +243,9 @@
capability_->RequirePIN(pin, require, error);
}
-void Cellular::EnterPIN(const string &pin, Error *error) {
- capability_->EnterPIN(pin, error);
+void Cellular::EnterPIN(const string &pin, ReturnerInterface *returner) {
+ VLOG(2) << __func__ << "(" << returner << ")";
+ capability_->EnterPIN(pin, returner);
}
void Cellular::UnblockPIN(
diff --git a/cellular.h b/cellular.h
index cf55455..1ec4231 100644
--- a/cellular.h
+++ b/cellular.h
@@ -165,7 +165,7 @@
virtual void Scan(Error *error);
virtual void RegisterOnNetwork(const std::string &network_id, Error *error);
virtual void RequirePIN(const std::string &pin, bool require, Error *error);
- virtual void EnterPIN(const std::string &pin, Error *error);
+ virtual void EnterPIN(const std::string &pin, ReturnerInterface *returner);
virtual void UnblockPIN(const std::string &unblock_code,
const std::string &pin,
Error *error);
diff --git a/cellular_capability.cc b/cellular_capability.cc
index 27658d7..4c97166 100644
--- a/cellular_capability.cc
+++ b/cellular_capability.cc
@@ -4,6 +4,7 @@
#include "shill/cellular_capability.h"
+#include "shill/adaptor_interfaces.h"
#include "shill/cellular.h"
#include "shill/error.h"
@@ -41,8 +42,12 @@
error, Error::kNotSupported, "RequirePIN not supported.");
}
-void CellularCapability::EnterPIN(const string &/*pin*/, Error *error) {
- Error::PopulateAndLog(error, Error::kNotSupported, "EnterPIN not supported.");
+void CellularCapability::EnterPIN(const string &/*pin*/,
+ ReturnerInterface *returner) {
+ Error error;
+ Error::PopulateAndLog(
+ &error, Error::kNotSupported, "EnterPIN not supported.");
+ returner->ReturnError(error);
}
void CellularCapability::UnblockPIN(const string &/*unblock_code*/,
diff --git a/cellular_capability.h b/cellular_capability.h
index 31f954d..2842d22 100644
--- a/cellular_capability.h
+++ b/cellular_capability.h
@@ -17,6 +17,7 @@
class Error;
class EventDispatcher;
class ProxyFactory;
+class ReturnerInterface;
// Cellular devices instantiate subclasses of CellularCapability that handle the
// specific modem technologies and capabilities.
@@ -62,7 +63,7 @@
// PIN management. The default implementation fails by populating |error|.
virtual void RequirePIN(const std::string &pin, bool require, Error *error);
- virtual void EnterPIN(const std::string &pin, Error *error);
+ virtual void EnterPIN(const std::string &pin, ReturnerInterface *returner);
virtual void UnblockPIN(const std::string &unblock_code,
const std::string &pin,
Error *error);
diff --git a/cellular_capability_gsm.cc b/cellular_capability_gsm.cc
index ac8c1a9..e226f76 100644
--- a/cellular_capability_gsm.cc
+++ b/cellular_capability_gsm.cc
@@ -117,12 +117,15 @@
if (cellular()->imei().empty()) {
// TODO(petkov): Switch to asynchronous calls (crosbug.com/17583).
cellular()->set_imei(card_proxy_->GetIMEI());
- VLOG(2) << "IMEI: " << cellular()->imei();
}
if (cellular()->imsi().empty()) {
// TODO(petkov): Switch to asynchronous calls (crosbug.com/17583).
- cellular()->set_imsi(card_proxy_->GetIMSI());
- VLOG(2) << "IMSI: " << cellular()->imsi();
+ try {
+ cellular()->set_imsi(card_proxy_->GetIMSI());
+ VLOG(2) << "IMSI: " << cellular()->imsi();
+ } catch (const DBus::Error e) {
+ LOG(WARNING) << "Unable to obtain IMSI: " << e.what();
+ }
}
if (spn_.empty()) {
// TODO(petkov): Switch to asynchronous calls (crosbug.com/17583).
@@ -332,18 +335,27 @@
card_proxy_->EnablePIN(pin, require);
}
-void CellularCapabilityGSM::EnterPIN(const string &pin, Error */*error*/) {
- VLOG(2) << __func__ << "(" << pin << ")";
+void CellularCapabilityGSM::EnterPIN(const string &pin,
+ ReturnerInterface *returner) {
+ VLOG(2) << __func__ << "(" << returner << ")";
// Defer because we may be in a dbus-c++ callback.
dispatcher()->PostTask(
task_factory_.NewRunnableMethod(
- &CellularCapabilityGSM::EnterPINTask, pin));
+ &CellularCapabilityGSM::EnterPINTask, pin, returner));
}
-void CellularCapabilityGSM::EnterPINTask(const string &pin) {
- VLOG(2) << __func__ << "(" << pin << ")";
+void CellularCapabilityGSM::EnterPINTask(const string &pin,
+ ReturnerInterface *returner) {
+ VLOG(2) << __func__ << "(" << returner << ")";
// TODO(petkov): Switch to asynchronous calls (crosbug.com/17583).
- card_proxy_->SendPIN(pin);
+ try {
+ card_proxy_->SendPIN(pin);
+ } catch (DBus::Error e) {
+ LOG(ERROR) << "EnterPIN failed: " << e.name() << "/" << e.message();
+ returner->ReturnError(Error(Error::kInternalError));
+ return;
+ }
+ returner->Return();
}
void CellularCapabilityGSM::UnblockPIN(
diff --git a/cellular_capability_gsm.h b/cellular_capability_gsm.h
index 80a72f4..1a81240 100644
--- a/cellular_capability_gsm.h
+++ b/cellular_capability_gsm.h
@@ -41,7 +41,7 @@
virtual void RegisterOnNetwork(const std::string &network_id, Error *error);
virtual bool IsRegistered();
virtual void RequirePIN(const std::string &pin, bool require, Error *error);
- virtual void EnterPIN(const std::string &pin, Error *error);
+ virtual void EnterPIN(const std::string &pin, ReturnerInterface *returner);
virtual void UnblockPIN(const std::string &unblock_code,
const std::string &pin,
Error *error);
@@ -95,7 +95,7 @@
void RegisterOnNetworkTask(const std::string &network_id);
void RequirePINTask(const std::string &pin, bool require);
- void EnterPINTask(const std::string &pin);
+ void EnterPINTask(const std::string &pin, ReturnerInterface *returner);
void UnblockPINTask(const std::string &unblock_code, const std::string &pin);
void ChangePINTask(const std::string &old_pin, const std::string &new_pin);
void ScanTask();
diff --git a/cellular_capability_gsm_unittest.cc b/cellular_capability_gsm_unittest.cc
index a8476a0..83ec2e1 100644
--- a/cellular_capability_gsm_unittest.cc
+++ b/cellular_capability_gsm_unittest.cc
@@ -18,6 +18,7 @@
#include "shill/mock_modem_gsm_network_proxy.h"
#include "shill/nice_mock_control.h"
+using testing::_;
using testing::NiceMock;
using testing::Return;
@@ -186,8 +187,10 @@
TEST_F(CellularCapabilityGSMTest, EnterPIN) {
Error error;
EXPECT_CALL(*card_proxy_, SendPIN(kPIN));
- capability_->EnterPIN(kPIN, &error);
- EXPECT_TRUE(error.IsSuccess());
+ MockReturner returner;
+ EXPECT_CALL(returner, Return());
+ EXPECT_CALL(returner, ReturnError(_)).Times(0);
+ capability_->EnterPIN(kPIN, &returner);
SetCardProxy();
dispatcher_.DispatchPendingEvents();
}
diff --git a/dbus_adaptor.cc b/dbus_adaptor.cc
index ea1ada9..5d71d95 100644
--- a/dbus_adaptor.cc
+++ b/dbus_adaptor.cc
@@ -349,4 +349,91 @@
return signature == ::DBus::type<uint32>::sig();
}
+// static
+DBusAdaptor::Returner *DBusAdaptor::Returner::Create(DBusAdaptor *adaptor) {
+ return new Returner(adaptor);
+}
+
+DBusAdaptor::Returner::Returner(DBusAdaptor *adaptor)
+ : adaptor_(adaptor),
+ state_(kStateInitialized) {
+ VLOG(2) << __func__ << " @ " << this;
+}
+
+DBusAdaptor::Returner::~Returner() {
+ CHECK(state_ != kStateDestroyed);
+ VLOG(2) << "Destroying returner @ " << this << " state: " << state_;
+ adaptor_ = NULL;
+ state_ = kStateDestroyed;
+}
+
+void DBusAdaptor::Returner::Return() {
+ VLOG(2) << __func__ << " @ " << this << " state: " << state_;
+ switch (state_) {
+ case kStateInitialized:
+ // Service method is returning right away, without any continuation.
+ state_ = kStateReturned;
+ return;
+ case kStateDelayed: {
+ // This return happens in the continuation.
+ DBus::ObjectAdaptor::Continuation *cont =
+ adaptor_->find_continuation(this);
+ CHECK(cont);
+ adaptor_->return_now(cont);
+ delete this;
+ return;
+ }
+ default:
+ NOTREACHED() << "Unexpected state: " << state_;
+ break;
+ }
+}
+
+void DBusAdaptor::Returner::ReturnError(const Error &error) {
+ VLOG(2) << __func__ << " @ " << this << " state: " << state_;
+ switch (state_) {
+ case kStateInitialized:
+ // Service method is returning right away, without any continuation.
+ error_.CopyFrom(error);
+ state_ = kStateReturned;
+ return;
+ case kStateDelayed: {
+ // This return happens in the continuation.
+ DBus::Error dbus_error;
+ error.ToDBusError(&dbus_error);
+ DBus::ObjectAdaptor::Continuation *cont =
+ adaptor_->find_continuation(this);
+ CHECK(cont);
+ adaptor_->return_error(cont, dbus_error);
+ delete this;
+ return;
+ }
+ default:
+ NOTREACHED() << "Unexpected state: " << state_;
+ break;
+ }
+}
+
+void DBusAdaptor::Returner::DelayOrReturn(DBus::Error *error) {
+ VLOG(2) << __func__ << " @ " << this << " state: " << state_;
+ switch (state_) {
+ case kStateInitialized:
+ // Service method needs continuation so delay the return.
+ state_ = kStateDelayed;
+
+ // return_later does not return. It unwinds the stack up to the dbus-c++
+ // message handler by throwing an exception.
+ adaptor_->return_later(this);
+ return;
+ case kStateReturned:
+ // Service method has returned right away, without any continuation.
+ error_.ToDBusError(error);
+ delete this;
+ return;
+ default:
+ NOTREACHED() << "Unexpected state: " << state_;
+ break;
+ }
+}
+
} // namespace shill
diff --git a/dbus_adaptor.h b/dbus_adaptor.h
index 2386f0d..109c095 100644
--- a/dbus_adaptor.h
+++ b/dbus_adaptor.h
@@ -13,13 +13,14 @@
#include <dbus-c++/dbus.h>
#include "shill/accessor_interface.h"
+#include "shill/adaptor_interfaces.h"
+#include "shill/error.h"
namespace shill {
#define SHILL_INTERFACE "org.chromium.flimflam"
#define SHILL_PATH "/org/chromium/flimflam"
-class Error;
class KeyValueStore;
class PropertyStore;
@@ -72,12 +73,57 @@
static bool IsUint16(::DBus::Signature signature);
static bool IsUint32(::DBus::Signature signature);
+ protected:
+ class Returner : public DBus::Tag,
+ public ReturnerInterface {
+ public:
+ // Creates a new returner instance associated with |adaptor|.
+ static Returner *Create(DBusAdaptor *adaptor);
+
+ // Used by the adaptor to initiate or delay the return, depending on the
+ // state of the returner. A call to this method should be the last statement
+ // in the adaptor method. If none of the interface Return* methods has been
+ // called yet, DelayOrReturn exits to the dbus-c++ message handler by
+ // throwing an exception. Otherwise, it initializes |error|, completes the
+ // RPC call right away and destroys |this|.
+ void DelayOrReturn(DBus::Error *error);
+
+ // Inherited from ReturnerInterface. These methods complete the RPC call
+ // right away and destroy the object if DelayOrReturn has been called
+ // already. Otherwise, they allow DelayOrReturn to complete the call.
+ virtual void Return();
+ virtual void ReturnError(const Error &error);
+
+ private:
+ // The returner transitions through the following states:
+ //
+ // Initialized -> [Delayed|Returned] -> Destroyed.
+ enum State {
+ kStateInitialized, // No *Return* methods called yet.
+ kStateDelayed, // DelayOrReturn called, Return* not.
+ kStateReturned, // Return* called, DelayOrReturn not.
+ kStateDestroyed // Return complete, returner destroyed.
+ };
+
+ explicit Returner(DBusAdaptor *adaptor);
+
+ // Destruction happens through the *Return* methods.
+ virtual ~Returner();
+
+ DBusAdaptor *adaptor_;
+ Error error_;
+ State state_;
+
+ DISALLOW_COPY_AND_ASSIGN(Returner);
+ };
+
private:
static const char kByteArraysSig[];
static const char kPathArraySig[];
static const char kStringmapSig[];
static const char kStringmapsSig[];
static const char kStringsSig[];
+
DISALLOW_COPY_AND_ASSIGN(DBusAdaptor);
};
diff --git a/device.cc b/device.cc
index 87a9a95..5281909 100644
--- a/device.cc
+++ b/device.cc
@@ -179,9 +179,12 @@
"Device doesn't support RequirePIN.");
}
-void Device::EnterPIN(const string &/*pin*/, Error *error) {
- Error::PopulateAndLog(error, Error::kNotSupported,
+void Device::EnterPIN(const string &/*pin*/, ReturnerInterface *returner) {
+ VLOG(2) << __func__;
+ Error error;
+ Error::PopulateAndLog(&error, Error::kNotSupported,
"Device doesn't support EnterPIN.");
+ returner->ReturnError(error);
}
void Device::UnblockPIN(const string &/*unblock_code*/,
diff --git a/device.h b/device.h
index 8ae93dc..c10d6d0 100644
--- a/device.h
+++ b/device.h
@@ -31,6 +31,7 @@
class EventDispatcher;
class Manager;
class RTNLHandler;
+class ReturnerInterface;
// Device superclass. Individual network interfaces types will inherit from
// this class.
@@ -67,7 +68,7 @@
virtual void Scan(Error *error);
virtual void RegisterOnNetwork(const std::string &network_id, Error *error);
virtual void RequirePIN(const std::string &pin, bool require, Error *error);
- virtual void EnterPIN(const std::string &pin, Error *error);
+ virtual void EnterPIN(const std::string &pin, ReturnerInterface *returner);
virtual void UnblockPIN(const std::string &unblock_code,
const std::string &pin,
Error *error);
diff --git a/device_dbus_adaptor.cc b/device_dbus_adaptor.cc
index b3c2270..87cedb2 100644
--- a/device_dbus_adaptor.cc
+++ b/device_dbus_adaptor.cc
@@ -106,9 +106,10 @@
}
void DeviceDBusAdaptor::EnterPin(const string &pin, ::DBus::Error &error) {
- Error e;
- device_->EnterPIN(pin, &e);
- e.ToDBusError(&error);
+ VLOG(2) << __func__;
+ Returner *returner = Returner::Create(this);
+ device_->EnterPIN(pin, returner);
+ returner->DelayOrReturn(&error);
}
void DeviceDBusAdaptor::UnblockPin(const string &unblock_code,
diff --git a/error.cc b/error.cc
index 5fd4ec4..b1d5dea 100644
--- a/error.cc
+++ b/error.cc
@@ -69,6 +69,10 @@
Populate(kSuccess);
}
+void Error::CopyFrom(const Error &error) {
+ Populate(error.type_, error.message_);
+}
+
bool Error::ToDBusError(::DBus::Error *error) const {
if (IsFailure()) {
error->set(GetName(type_).c_str(), message_.c_str());
diff --git a/error.h b/error.h
index bee2465..3c01047 100644
--- a/error.h
+++ b/error.h
@@ -51,6 +51,8 @@
void Reset();
+ void CopyFrom(const Error &error);
+
// Sets the DBus |error| and returns true if Error represents failure.
// Leaves |error| unchanged, and returns false, otherwise.
bool ToDBusError(::DBus::Error *error) const;
diff --git a/error_unittest.cc b/error_unittest.cc
index fa67b29..7736e38 100644
--- a/error_unittest.cc
+++ b/error_unittest.cc
@@ -56,6 +56,14 @@
EXPECT_EQ(kMessage, e.message());
}
+TEST_F(ErrorTest, CopyFrom) {
+ Error e(Error::kInvalidArguments, "Some message");
+ Error copy;
+ copy.CopyFrom(e);
+ EXPECT_EQ(e.type(), copy.type());
+ EXPECT_EQ(e.message(), copy.message());
+}
+
TEST_F(ErrorTest, ToDBusError) {
DBus::Error dbus_error;
ASSERT_FALSE(dbus_error.is_set());
diff --git a/mock_adaptors.cc b/mock_adaptors.cc
index 61881e5..7634dfb 100644
--- a/mock_adaptors.cc
+++ b/mock_adaptors.cc
@@ -61,4 +61,9 @@
ServiceMockAdaptor::~ServiceMockAdaptor() {}
const std::string &ServiceMockAdaptor::GetRpcIdentifier() { return rpc_id; }
+
+MockReturner::MockReturner() {}
+
+MockReturner::~MockReturner() {}
+
} // namespace shill
diff --git a/mock_adaptors.h b/mock_adaptors.h
index 84819c6..cbaf55d 100644
--- a/mock_adaptors.h
+++ b/mock_adaptors.h
@@ -10,6 +10,7 @@
#include <gmock/gmock.h>
#include "shill/adaptor_interfaces.h"
+#include "shill/error.h"
namespace shill {
@@ -117,5 +118,14 @@
const std::string rpc_id;
};
+class MockReturner : public ReturnerInterface {
+ public:
+ MockReturner();
+ virtual ~MockReturner();
+
+ MOCK_METHOD0(Return, void());
+ MOCK_METHOD1(ReturnError, void(const Error &error));
+};
+
} // namespace shill
#endif // SHILL_MOCK_ADAPTORS_