shill: dongles: report PPP authentication failures correctly
Previously, any time we received a disconnect notification from
pppd, we would attempt to set our error state to indicate that
PPP authentication failed. (See below for more on "attempt to".)
This CL tightens up our disconnect handling, to identify which
disconnects are likely to be authentication failures, versus
those that are other failures. Other failures include things like
being unable to acquire an acceptable IP configuration.
We say a disconnect is an authentication failure if PPP was in
the middle of authenticating when the disconnect occurred. Otherwise,
we report kFailureUnknown.
In addition to tightening up the error reporting, this CL fixes a
case where errors would fail to be reported.
Specifically, we used to call SetServiceState, which modifies the
state of selected_service(). However, if we had not yet started
configuring IP, then there would be no selected service, and no
error would be reported.
Fix this, by falling back to |service_| in this case. Note that I
suspect that the failed error reporting is a long-standing issue,
and not specific to PPP dongles.
While there:
- Lower the priority of some log statements (from INFO to VERBOSE2).
- Uncomment a line in CelluarTest.Notify, which I'd meant to uncomment
before it originally landed.
- Improve readability of log statement in TaskProxy::Notify.
BUG=chromium:267682
TEST=(new) unit tests, manual
TEST=network_3GSmokeTest.pseudomodem
TEST=network_3GModemControl.pseudomodem
Manual testing
--------------
1. get a dongle that requires a specific PPP username/password
2. use set_cellular_ppp (in crosh), to configure an incorrect
username/password combination
3. try connecting to the cellular network
4. observe a toast-style notification with an error message
indicating a bad username or password
Change-Id: I60fa7fa9ad8901f5a19eea23b6eb44a6100a02db
Reviewed-on: https://gerrit.chromium.org/gerrit/64863
Reviewed-by: mukesh agrawal <quiche@chromium.org>
Tested-by: mukesh agrawal <quiche@chromium.org>
Commit-Queue: mukesh agrawal <quiche@chromium.org>
diff --git a/cellular.cc b/cellular.cc
index af88967..e5560a0 100644
--- a/cellular.cc
+++ b/cellular.cc
@@ -120,7 +120,8 @@
proxy_factory_(proxy_factory),
ppp_device_factory_(PPPDeviceFactory::GetInstance()),
allow_roaming_(false),
- explicit_disconnect_(false) {
+ explicit_disconnect_(false),
+ is_ppp_authenticating_(false) {
PropertyStore *store = this->mutable_store();
// TODO(jglasgow): kDBusConnectionProperty is deprecated.
store->RegisterConstString(flimflam::kDBusConnectionProperty, &dbus_owner_);
@@ -378,24 +379,36 @@
void Cellular::SetServiceState(Service::ConnectState state) {
if (ppp_device_) {
ppp_device_->SetServiceState(state);
- } else {
+ } else if (selected_service()) {
Device::SetServiceState(state);
+ } else if (service_) {
+ service_->SetState(state);
+ } else {
+ LOG(WARNING) << "State change with no Service.";
}
}
void Cellular::SetServiceFailure(Service::ConnectFailure failure_state) {
if (ppp_device_) {
ppp_device_->SetServiceFailure(failure_state);
- } else {
+ } else if (selected_service()) {
Device::SetServiceFailure(failure_state);
+ } else if (service_) {
+ service_->SetFailure(failure_state);
+ } else {
+ LOG(WARNING) << "State change with no Service.";
}
}
void Cellular::SetServiceFailureSilent(Service::ConnectFailure failure_state) {
if (ppp_device_) {
ppp_device_->SetServiceFailureSilent(failure_state);
- } else {
+ } else if (selected_service()) {
Device::SetServiceFailureSilent(failure_state);
+ } else if (service_) {
+ service_->SetFailureSilent(failure_state);
+ } else {
+ LOG(WARNING) << "State change with no Service.";
}
}
@@ -806,6 +819,7 @@
}
void Cellular::StartPPP(const string &serial_device) {
+ SLOG(PPP, 2) << __func__ << " on " << serial_device;
// Detach any SelectedService from this device. It will be grafted onto
// the PPPDevice after PPP is up (in Cellular::Notify).
//
@@ -838,6 +852,7 @@
args.push_back("plugin"); // Goes with next arg.
args.push_back(PPPDevice::kPluginPath);
args.push_back(serial_device);
+ is_ppp_authenticating_ = false;
scoped_ptr<ExternalTask> new_ppp_task(
new ExternalTask(modem_info_->control_interface(),
modem_info_->glib(),
@@ -852,7 +867,7 @@
// called by |ppp_task_|
void Cellular::GetLogin(string *user, string *password) {
- LOG(INFO) << __func__;
+ SLOG(PPP, 2) << __func__;
if (!service()) {
LOG(ERROR) << __func__ << " with no service ";
return;
@@ -866,18 +881,34 @@
// Called by |ppp_task_|.
void Cellular::Notify(const string &reason,
const map<string, string> &dict) {
- LOG(INFO) << __func__ << " " << reason << " on " << link_name();
+ SLOG(PPP, 2) << __func__ << " " << reason << " on " << link_name();
- if (reason != kPPPReasonConnect) {
- DCHECK_EQ(kPPPReasonDisconnect, reason);
- // DestroyLater, rather than while on stack.
- ppp_task_.release()->DestroyLater(modem_info_->dispatcher());
- // For now, assume PPP failures are due to authentication issues.
- SetServiceFailure(Service::kFailurePPPAuth);
- return;
+ if (reason == kPPPReasonAuthenticating) {
+ OnPPPAuthenticating();
+ } else if (reason == kPPPReasonAuthenticated) {
+ OnPPPAuthenticated();
+ } else if (reason == kPPPReasonConnect) {
+ OnPPPConnected(dict);
+ } else if (reason == kPPPReasonDisconnect) {
+ OnPPPDisconnected();
+ } else {
+ NOTREACHED();
}
+}
- string interface_name = PPPDevice::GetInterfaceName(dict);
+void Cellular::OnPPPAuthenticated() {
+ SLOG(PPP, 2) << __func__;
+ is_ppp_authenticating_ = false;
+}
+
+void Cellular::OnPPPAuthenticating() {
+ SLOG(PPP, 2) << __func__;
+ is_ppp_authenticating_ = true;
+}
+
+void Cellular::OnPPPConnected(const map<string, string> ¶ms) {
+ SLOG(PPP, 2) << __func__;
+ string interface_name = PPPDevice::GetInterfaceName(params);
DeviceInfo *device_info = modem_info_->manager()->device_info();
int interface_index = device_info->GetIndex(interface_name);
if (interface_index < 0) {
@@ -907,7 +938,20 @@
const bool kBlackholeIPv6 = false;
ppp_device_->SetEnabled(true);
ppp_device_->SelectService(service_);
- ppp_device_->UpdateIPConfigFromPPP(dict, kBlackholeIPv6);
+ ppp_device_->UpdateIPConfigFromPPP(params, kBlackholeIPv6);
+}
+
+void Cellular::OnPPPDisconnected() {
+ SLOG(PPP, 2) << __func__;
+ // DestroyLater, rather than while on stack.
+ ppp_task_.release()->DestroyLater(modem_info_->dispatcher());
+ if (is_ppp_authenticating_) {
+ SetServiceFailure(Service::kFailurePPPAuth);
+ } else {
+ // TODO(quiche): Don't set failure if we disconnected intentionally.
+ SetServiceFailure(Service::kFailureUnknown);
+ }
+ return;
}
void Cellular::OnPPPDied(pid_t pid, int exit) {