shill: PPP dongles: cleanly move Service from Cellular to PPPDevice
In CL:61602, I added a CHECK that the Cellular object doesn't have
a selected_service() when PPP comes up. The idea was to ensure that
we only have one Device trying to drive the state of the CellularService.
This CHECK fails on devices with an embryonic cdc_ether or cdc_ncm
interface, because we SelectService() in LinkEvent().
Resolve this by dropping the selected_service() in StartPPP().
We do this using DropConnection(), which may be what I should have
done all along. (DropConnection() also takes care of killing dhcpcd,
if it is running.)
While there: fix up a couple of buggy log statements in device_info.
BUG=chromium:261364
TEST=unit tests, manual
Manual testing
--------------
- plug in a dongle with an embryonic cdc_ether or cdc_ncm interface.
e.g. the huawei UMG1691 dongle
- check that the cellular connection comes up (IP address is configured,
can load web pages)
- check that shill did not crash in the process (grep crash /var/log/messages)
- repeat steps 1-3 with a ppp-only dongle (e.g. huawei e369)
Change-Id: Ia270f00746dc321b41a223e9f848ffc796dd8528
Reviewed-on: https://gerrit.chromium.org/gerrit/62712
Reviewed-by: mukesh agrawal <quiche@chromium.org>
Tested-by: mukesh agrawal <quiche@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>
Commit-Queue: mukesh agrawal <quiche@chromium.org>
diff --git a/cellular.cc b/cellular.cc
index 37661aa..2896a32 100644
--- a/cellular.cc
+++ b/cellular.cc
@@ -803,12 +803,23 @@
}
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();
+ // Detach any SelectedService from this device. It will be grafted onto
+ // the PPPDevice after PPP is up (in Cellular::Notify).
+ //
+ // This has two important effects: 1) kills dhcpcd if it is running.
+ // 2) stops Cellular::LinkEvent from driving changes to the
+ // SelectedService.
+ if (selected_service()) {
+ CHECK_EQ(service_.get(), selected_service().get());
+ // Save and restore |service_| state, as DropConnection calls
+ // SelectService, and SelectService will move selected_service()
+ // to kStateIdle.
+ Service::ConnectState original_state(service_->state());
+ Device::DropConnection(); // Don't redirect to PPPDevice.
+ service_->SetState(original_state);
+ } else {
+ CHECK(!ipconfig()); // Shouldn't have ipconfig without selected_service().
+ }
base::Callback<void(pid_t, int)> death_callback(
Bind(&Cellular::OnPPPDied, weak_ptr_factory_.GetWeakPtr()));
diff --git a/cellular.h b/cellular.h
index 9b1f937..0387388 100644
--- a/cellular.h
+++ b/cellular.h
@@ -313,7 +313,8 @@
FRIEND_TEST(CellularTest, StartCDMARegister);
FRIEND_TEST(CellularTest, StartGSMRegister);
FRIEND_TEST(CellularTest, StartLinked);
- FRIEND_TEST(CellularTest, StartPPP); // for |state_|
+ FRIEND_TEST(CellularTest, StartPPPAfterEthernetUp);
+ FRIEND_TEST(CellularTest, StartPPPWithoutEthernet);
FRIEND_TEST(Modem1Test, CreateDeviceMM1);
// Names of properties in storage
diff --git a/cellular_unittest.cc b/cellular_unittest.cc
index c945271..892da29 100644
--- a/cellular_unittest.cc
+++ b/cellular_unittest.cc
@@ -1031,21 +1031,32 @@
device_->LinkEvent(IFF_UP, 0);
}
-TEST_F(CellularTest, StartPPP) {
- device_->set_ipconfig(dhcp_config_);
+TEST_F(CellularTest, StartPPPAfterEthernetUp) {
+ CellularService *service(SetService());
device_->state_ = Cellular::kStateLinked;
+ device_->set_ipconfig(dhcp_config_);
+ device_->SelectService(service);
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_FALSE(device_->ipconfig()); // The DHCP client was stopped.
+ EXPECT_FALSE(device_->selected_service());
EXPECT_EQ(Cellular::kStateLinked, device_->state());
- // TODO(quiche): test the rest of the functionality of this method.
- // crbug.com/246826.
}
+TEST_F(CellularTest, StartPPPWithoutEthernet) {
+ EXPECT_CALL(*dynamic_cast<MockGLib *>(modem_info_.glib()),
+ SpawnAsync(_, _, _, _, _, _, _, _));
+ device_->StartPPP("fake_serial_device");
+ EXPECT_FALSE(device_->ipconfig()); // No DHCP client.
+ EXPECT_FALSE(device_->selected_service());
+}
+
+// TODO(quiche): test the common bits of StartPPP. crbug.com/246826.
+
TEST_F(CellularTest, GetLogin) {
// Doesn't crash when there is no service.
string username_to_pppd;
diff --git a/device_info.cc b/device_info.cc
index d21a0e0..e7240c5 100644
--- a/device_info.cc
+++ b/device_info.cc
@@ -302,11 +302,11 @@
if (driver_name == kDriverCdcEther || driver_name == kDriverCdcNcm) {
if (IsCdcEthernetModemDevice(iface_name)) {
LOG(INFO) << StringPrintf("%s: device %s is a %s modem device", __func__,
- driver_name.c_str(), iface_name.c_str());
+ iface_name.c_str(), driver_name.c_str());
return Technology::kCellular;
}
SLOG(Device, 2) << StringPrintf("%s: device %s is a %s device", __func__,
- driver_name.c_str(), iface_name.c_str());
+ iface_name.c_str(), driver_name.c_str());
return Technology::kCDCEthernet;
}