shill: don't run dhcpcd and pppd simultaneously, for the same Cellular
Some Cellular dongles expose both a network device (e.g. wwan0),
and a serial interface for ppp. Despite the presence of the network
device, they really want to be used as PPP devices.
Normally, we run dhcpcd on every network device, and terminate the
link if we are unable to acquire a DHCP lease. In the case of PPP
dongles, that causes us to tear down a totally functional link
after about 30 seconds. (The IP address is acquired using PPP
IPCP, and applied to the ppp0, or similar, interface.)
Avoid this problem, by a) not starting dhcpcd if pppd is already
running for the Cellular device, and b) stopping dhcpcd for the
Cellular device (if running), when we start pppd.
BUG=chromium:252516
TEST=new unit tests
Change-Id: I506413655ce44d36b3db0bd8e436340e5f5c20a9
Reviewed-on: https://gerrit.chromium.org/gerrit/60328
Reviewed-by: Ben Chan <benchan@chromium.org>
Commit-Queue: mukesh agrawal <quiche@chromium.org>
Tested-by: mukesh agrawal <quiche@chromium.org>
diff --git a/cellular.cc b/cellular.cc
index 3f31e30..678f75b 100644
--- a/cellular.cc
+++ b/cellular.cc
@@ -640,6 +640,11 @@
void Cellular::LinkEvent(unsigned int flags, unsigned int change) {
Device::LinkEvent(flags, change);
+ if (ppp_task_) {
+ LOG(INFO) << "Ignoring LinkEvent on device with PPP interface.";
+ return;
+ }
+
if ((flags & IFF_UP) != 0 && state_ == kStateConnected) {
LOG(INFO) << link_name() << " is up.";
SetState(kStateLinked);
@@ -763,6 +768,13 @@
}
void Cellular::StartPPP(const string &serial_device) {
+ // Some PPP dongles also expose an Ethernet-like network device,
+ // using either cdc_ether, or cdc_ncm. dhcpcd may be running on that
+ // interface. If so, it will eventually time out, and cause us
+ // terminate the cellular connection. Avoid terminating the connection,
+ // by cancelling any such DHCP process.
+ DestroyIPConfig();
+
base::Callback<void(pid_t, int)> death_callback(
Bind(&Cellular::OnPPPDied, weak_ptr_factory_.GetWeakPtr()));
vector<string> args;
diff --git a/cellular.h b/cellular.h
index a033022..5b9d6d2 100644
--- a/cellular.h
+++ b/cellular.h
@@ -286,6 +286,8 @@
FRIEND_TEST(CellularTest, EnableTrafficMonitor);
FRIEND_TEST(CellularTest,
HandleNewRegistrationStateForServiceRequiringActivation);
+ FRIEND_TEST(CellularTest, LinkEventUpWithPPP);
+ FRIEND_TEST(CellularTest, LinkEventUpWithoutPPP);
FRIEND_TEST(CellularTest, LinkEventWontDestroyService);
FRIEND_TEST(CellularTest, ModemStateChangeDisable);
FRIEND_TEST(CellularTest, ModemStateChangeEnable);
@@ -301,6 +303,7 @@
FRIEND_TEST(CellularTest, StartCDMARegister);
FRIEND_TEST(CellularTest, StartGSMRegister);
FRIEND_TEST(CellularTest, StartLinked);
+ FRIEND_TEST(CellularTest, StartPPP); // for |state_|
FRIEND_TEST(Modem1Test, CreateDeviceMM1);
// Names of properties in storage
diff --git a/cellular_unittest.cc b/cellular_unittest.cc
index fdf66cb..beed15d 100644
--- a/cellular_unittest.cc
+++ b/cellular_unittest.cc
@@ -22,6 +22,7 @@
#include "shill/mock_device_info.h"
#include "shill/mock_dhcp_config.h"
#include "shill/mock_dhcp_provider.h"
+#include "shill/mock_external_task.h"
#include "shill/mock_modem_cdma_proxy.h"
#include "shill/mock_modem_gsm_card_proxy.h"
#include "shill/mock_modem_gsm_network_proxy.h"
@@ -31,6 +32,7 @@
#include "shill/mock_rtnl_handler.h"
#include "shill/property_store_unittest.h"
#include "shill/proxy_factory.h"
+#include "shill/rpc_task.h" // for RpcTaskDelegate
// mm/mm-modem.h must be included after cellular_capability_universal.h
// in order to allow MM_MODEM_CDMA_* to be defined properly.
@@ -991,6 +993,58 @@
EXPECT_TRUE(device_->allow_roaming_);
}
+class TestRPCTaskDelegate :
+ public RPCTaskDelegate,
+ public base::SupportsWeakPtr<TestRPCTaskDelegate> {
+ public:
+ virtual void GetLogin(std::string *user, std::string *password) {}
+ virtual void Notify(const std::string &reason,
+ const std::map<std::string, std::string> &dict) {}
+};
+
+TEST_F(CellularTest, LinkEventUpWithPPP) {
+ // If PPP is running, don't run DHCP as well.
+ TestRPCTaskDelegate task_delegate;
+ base::Callback<void(pid_t, int)> death_callback;
+ scoped_ptr<NiceMock<MockExternalTask>> mock_task(
+ new NiceMock<MockExternalTask>(modem_info_.control_interface(),
+ modem_info_.glib(),
+ task_delegate.AsWeakPtr(),
+ death_callback));
+ EXPECT_CALL(*mock_task, OnDelete()).Times(AnyNumber());
+ device_->ppp_task_ = mock_task.Pass();
+ device_->state_ = Cellular::kStateConnected;
+ EXPECT_CALL(dhcp_provider_, CreateConfig(kTestDeviceName, _, _, _))
+ .Times(0);
+ EXPECT_CALL(*dhcp_config_, RequestIP()).Times(0);
+ device_->LinkEvent(IFF_UP, 0);
+}
+
+TEST_F(CellularTest, LinkEventUpWithoutPPP) {
+ // If PPP is not running, fire up DHCP.
+ device_->state_ = Cellular::kStateConnected;
+ EXPECT_CALL(dhcp_provider_, CreateConfig(kTestDeviceName, _, _, _))
+ .WillOnce(Return(dhcp_config_));
+ EXPECT_CALL(*dhcp_config_, RequestIP());
+ EXPECT_CALL(*dhcp_config_, ReleaseIP(_)).Times(AnyNumber());
+ device_->LinkEvent(IFF_UP, 0);
+}
+
+TEST_F(CellularTest, StartPPP) {
+ device_->set_ipconfig(dhcp_config_);
+ device_->state_ = Cellular::kStateLinked;
+ EXPECT_CALL(*dhcp_config_, ReleaseIP(_))
+ .Times(AnyNumber())
+ .WillRepeatedly(Return(true));
+ EXPECT_CALL(*dynamic_cast<MockGLib *>(modem_info_.glib()),
+ SpawnAsync(_, _, _, _, _, _, _, _));
+ device_->StartPPP("fake_serial_device");
+ EXPECT_FALSE(device_->ipconfig()); // Any running DHCP client is stopped.
+ EXPECT_EQ(Cellular::kStateLinked, device_->state());
+ // TODO(quiche): test the rest of the functionality of this method.
+ // crbug.com/246826.
+}
+
TEST_F(CellularTest, GetLogin) {
// Doesn't crash when there is no service.
string username_to_pppd;
diff --git a/mock_dhcp_config.h b/mock_dhcp_config.h
index f74ccc6..c7de64c 100644
--- a/mock_dhcp_config.h
+++ b/mock_dhcp_config.h
@@ -21,6 +21,7 @@
virtual ~MockDHCPConfig();
MOCK_METHOD0(RequestIP, bool());
+ MOCK_METHOD1(ReleaseIP, bool(ReleaseReason));
private:
DISALLOW_COPY_AND_ASSIGN(MockDHCPConfig);