Index: chromeos/dbus/session_manager_client.cc |
diff --git a/chromeos/dbus/session_manager_client.cc b/chromeos/dbus/session_manager_client.cc |
index 90322ff80789326d2542afd306515200c97449c8..62af5df8e49327ccdbb70eb40c1d4da761457230 100644 |
--- a/chromeos/dbus/session_manager_client.cc |
+++ b/chromeos/dbus/session_manager_client.cc |
@@ -15,6 +15,7 @@ |
#include "base/files/file_util.h" |
#include "base/location.h" |
#include "base/macros.h" |
+#include "base/metrics/histogram_macros.h" |
#include "base/path_service.h" |
#include "base/strings/string_number_conversions.h" |
#include "base/strings/string_util.h" |
@@ -30,15 +31,15 @@ |
#include "dbus/message.h" |
#include "dbus/object_path.h" |
#include "dbus/object_proxy.h" |
+#include "dbus/scoped_dbus_error.h" |
#include "third_party/cros_system_api/dbus/service_constants.h" |
namespace chromeos { |
namespace { |
-// TODO(hidehiko): Share the constant between Chrome and ChromeOS. |
-constexpr char kArcLowDiskError[] = |
- "org.chromium.SessionManagerInterface.LowFreeDisk"; |
+using RetrievePolicyResponseType = |
+ SessionManagerClient::RetrievePolicyResponseType; |
constexpr char kStubPolicyFile[] = "stub_policy"; |
constexpr char kStubDevicePolicyFile[] = "stub_device_policy"; |
@@ -103,6 +104,50 @@ void RunStateKeysCallbackStub(SessionManagerClient::StateKeysCallback callback, |
callback.Run(state_keys); |
} |
+// Helper to notify the callback with SUCCESS, to be used by the stub. |
+void NotifyOnRetrievePolicySuccess( |
+ const SessionManagerClient::RetrievePolicyCallback& callback, |
+ const std::string& protobuf) { |
+ callback.Run(protobuf, RetrievePolicyResponseType::SUCCESS); |
+} |
+ |
+// Helper to get the enum type of RetrievePolicyResponseType based on error |
+// name. |
+RetrievePolicyResponseType GetResponseTypeBasedOnError( |
+ const std::string& error_name) { |
+ if (error_name == login_manager::dbus_error::kNone) { |
+ return RetrievePolicyResponseType::SUCCESS; |
+ } |
+ if (error_name == login_manager::dbus_error::kSessionDoesNotExist) { |
+ return RetrievePolicyResponseType::SESSION_DOES_NOT_EXIST; |
+ } |
+ if (error_name == login_manager::dbus_error::kSigEncodeFail) { |
+ return RetrievePolicyResponseType::POLICY_ENCODE_ERROR; |
+ } |
+ return RetrievePolicyResponseType::OTHER_ERROR; |
+} |
+ |
+// Logs UMA stat for retrieve policy request, corresponding to D-Bus method name |
+// used. |
+void LogPolicyResponseUma(const std::string& method_name, |
+ RetrievePolicyResponseType response) { |
+ if (method_name == login_manager::kSessionManagerRetrievePolicy) { |
+ UMA_HISTOGRAM_ENUMERATION("Enterprise.RetrievePolicyResponse.Device", |
+ response, RetrievePolicyResponseType::COUNT); |
+ } else if (method_name == |
+ login_manager::kSessionManagerRetrieveDeviceLocalAccountPolicy) { |
+ UMA_HISTOGRAM_ENUMERATION( |
+ "Enterprise.RetrievePolicyResponse.DeviceLocalAccount", response, |
+ RetrievePolicyResponseType::COUNT); |
+ } else if (method_name == |
+ login_manager::kSessionManagerRetrievePolicyForUser) { |
+ UMA_HISTOGRAM_ENUMERATION("Enterprise.RetrievePolicyResponse.User", |
+ response, RetrievePolicyResponseType::COUNT); |
+ } else { |
+ LOG(ERROR) << "Invalid method_name: " << method_name; |
+ } |
+} |
+ |
} // namespace |
// The SessionManagerClient implementation used in production. |
@@ -231,24 +276,36 @@ class SessionManagerClientImpl : public SessionManagerClient { |
void RetrieveDevicePolicy(const RetrievePolicyCallback& callback) override { |
dbus::MethodCall method_call(login_manager::kSessionManagerInterface, |
login_manager::kSessionManagerRetrievePolicy); |
- session_manager_proxy_->CallMethod( |
- &method_call, |
- dbus::ObjectProxy::TIMEOUT_USE_DEFAULT, |
- base::Bind(&SessionManagerClientImpl::OnRetrievePolicy, |
+ session_manager_proxy_->CallMethodWithErrorCallback( |
+ &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT, |
+ base::Bind(&SessionManagerClientImpl::OnRetrievePolicySuccess, |
weak_ptr_factory_.GetWeakPtr(), |
- login_manager::kSessionManagerRetrievePolicy, |
- callback)); |
+ login_manager::kSessionManagerRetrievePolicy, callback), |
+ base::Bind(&SessionManagerClientImpl::OnRetrievePolicyError, |
+ weak_ptr_factory_.GetWeakPtr(), |
+ login_manager::kSessionManagerRetrievePolicy, callback)); |
} |
- std::string BlockingRetrieveDevicePolicy() override { |
+ RetrievePolicyResponseType BlockingRetrieveDevicePolicy( |
+ std::string* policy) override { |
dbus::MethodCall method_call(login_manager::kSessionManagerInterface, |
login_manager::kSessionManagerRetrievePolicy); |
+ dbus::ScopedDBusError error; |
std::unique_ptr<dbus::Response> response = |
- blocking_method_caller_->CallMethodAndBlock(&method_call); |
- std::string policy; |
- ExtractString(login_manager::kSessionManagerRetrievePolicy, response.get(), |
- &policy); |
- return policy; |
+ blocking_method_caller_->CallMethodAndBlockWithError(&method_call, |
+ &error); |
+ RetrievePolicyResponseType result = RetrievePolicyResponseType::SUCCESS; |
+ if (error.is_set() && error.name()) { |
+ result = GetResponseTypeBasedOnError(error.name()); |
+ } |
+ if (result == RetrievePolicyResponseType::SUCCESS) { |
+ ExtractString(login_manager::kSessionManagerRetrievePolicy, |
+ response.get(), policy); |
+ } else { |
+ *policy = ""; |
+ } |
+ LogPolicyResponseUma(login_manager::kSessionManagerRetrievePolicy, result); |
+ return result; |
} |
void RetrievePolicyForUser(const cryptohome::Identification& cryptohome_id, |
@@ -258,20 +315,12 @@ class SessionManagerClientImpl : public SessionManagerClient { |
callback); |
} |
- std::string BlockingRetrievePolicyForUser( |
- const cryptohome::Identification& cryptohome_id) override { |
- dbus::MethodCall method_call( |
- login_manager::kSessionManagerInterface, |
- login_manager::kSessionManagerRetrievePolicyForUser); |
- dbus::MessageWriter writer(&method_call); |
- writer.AppendString(cryptohome_id.id()); |
- std::unique_ptr<dbus::Response> response = |
- blocking_method_caller_->CallMethodAndBlock(&method_call); |
- std::string policy; |
- ExtractString(login_manager::kSessionManagerRetrievePolicyForUser, |
- response.get(), |
- &policy); |
- return policy; |
+ RetrievePolicyResponseType BlockingRetrievePolicyForUser( |
+ const cryptohome::Identification& cryptohome_id, |
+ std::string* policy) override { |
+ return BlockingRetrievePolicyByUsername( |
+ login_manager::kSessionManagerRetrievePolicyForUser, cryptohome_id.id(), |
+ policy); |
} |
void RetrieveDeviceLocalAccountPolicy( |
@@ -279,24 +328,15 @@ class SessionManagerClientImpl : public SessionManagerClient { |
const RetrievePolicyCallback& callback) override { |
CallRetrievePolicyByUsername( |
login_manager::kSessionManagerRetrieveDeviceLocalAccountPolicy, |
- account_name, |
- callback); |
+ account_name, callback); |
} |
- std::string BlockingRetrieveDeviceLocalAccountPolicy( |
- const std::string& account_name) override { |
- dbus::MethodCall method_call( |
- login_manager::kSessionManagerInterface, |
- login_manager::kSessionManagerRetrieveDeviceLocalAccountPolicy); |
- dbus::MessageWriter writer(&method_call); |
- writer.AppendString(account_name); |
- std::unique_ptr<dbus::Response> response = |
- blocking_method_caller_->CallMethodAndBlock(&method_call); |
- std::string policy; |
- ExtractString( |
+ RetrievePolicyResponseType BlockingRetrieveDeviceLocalAccountPolicy( |
+ const std::string& account_name, |
+ std::string* policy) override { |
+ return BlockingRetrievePolicyByUsername( |
login_manager::kSessionManagerRetrieveDeviceLocalAccountPolicy, |
- response.get(), &policy); |
- return policy; |
+ account_name, policy); |
} |
void StoreDevicePolicy(const std::string& policy_blob, |
@@ -524,14 +564,39 @@ class SessionManagerClientImpl : public SessionManagerClient { |
method_name); |
dbus::MessageWriter writer(&method_call); |
writer.AppendString(account_id); |
- session_manager_proxy_->CallMethod( |
- &method_call, |
- dbus::ObjectProxy::TIMEOUT_USE_DEFAULT, |
- base::Bind( |
- &SessionManagerClientImpl::OnRetrievePolicy, |
- weak_ptr_factory_.GetWeakPtr(), |
- method_name, |
- callback)); |
+ session_manager_proxy_->CallMethodWithErrorCallback( |
+ &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT, |
+ base::Bind(&SessionManagerClientImpl::OnRetrievePolicySuccess, |
+ weak_ptr_factory_.GetWeakPtr(), method_name, callback), |
+ base::Bind(&SessionManagerClientImpl::OnRetrievePolicyError, |
+ weak_ptr_factory_.GetWeakPtr(), method_name, callback)); |
+ } |
+ |
+ // Helper for blocking RetrievePolicyForUser and |
+ // RetrieveDeviceLocalAccountPolicy. |
+ RetrievePolicyResponseType BlockingRetrievePolicyByUsername( |
+ const std::string& method_name, |
+ const std::string& account_name, |
+ std::string* policy) { |
+ dbus::MethodCall method_call(login_manager::kSessionManagerInterface, |
+ method_name); |
+ dbus::MessageWriter writer(&method_call); |
+ writer.AppendString(account_name); |
+ dbus::ScopedDBusError error; |
+ std::unique_ptr<dbus::Response> response = |
+ blocking_method_caller_->CallMethodAndBlockWithError(&method_call, |
+ &error); |
+ RetrievePolicyResponseType result = RetrievePolicyResponseType::SUCCESS; |
+ if (error.is_set() && error.name()) { |
+ result = GetResponseTypeBasedOnError(error.name()); |
+ } |
+ if (result == RetrievePolicyResponseType::SUCCESS) { |
+ ExtractString(method_name, response.get(), policy); |
+ } else { |
+ *policy = ""; |
+ } |
+ LogPolicyResponseUma(method_name, result); |
+ return result; |
} |
void CallStorePolicyByUsername(const std::string& method_name, |
@@ -643,13 +708,27 @@ class SessionManagerClientImpl : public SessionManagerClient { |
} |
// Called when kSessionManagerRetrievePolicy or |
- // kSessionManagerRetrievePolicyForUser method is complete. |
- void OnRetrievePolicy(const std::string& method_name, |
- const RetrievePolicyCallback& callback, |
- dbus::Response* response) { |
+ // kSessionManagerRetrievePolicyForUser method is successfully complete. |
+ void OnRetrievePolicySuccess(const std::string& method_name, |
+ const RetrievePolicyCallback& callback, |
+ dbus::Response* response) { |
std::string serialized_proto; |
ExtractString(method_name, response, &serialized_proto); |
- callback.Run(serialized_proto); |
+ callback.Run(serialized_proto, RetrievePolicyResponseType::SUCCESS); |
+ |
+ LogPolicyResponseUma(method_name, RetrievePolicyResponseType::SUCCESS); |
+ } |
+ |
+ // Called when kSessionManagerRetrievePolicy or |
+ // kSessionManagerRetrievePolicyForUser method fails. |
+ void OnRetrievePolicyError(const std::string& method_name, |
+ const RetrievePolicyCallback& callback, |
+ dbus::ErrorResponse* response) { |
+ RetrievePolicyResponseType response_type = |
+ GetResponseTypeBasedOnError(response->GetErrorName()); |
+ callback.Run(std::string(), response_type); |
+ |
+ LogPolicyResponseUma(method_name, response_type); |
} |
// Called when kSessionManagerStorePolicy or kSessionManagerStorePolicyForUser |
@@ -820,10 +899,12 @@ class SessionManagerClientImpl : public SessionManagerClient { |
dbus::ErrorResponse* response) { |
LOG(ERROR) << "Failed to call StartArcInstance: " |
<< (response ? response->ToString() : "(null)"); |
- if (!callback.is_null()) |
- callback.Run(response && response->GetErrorName() == kArcLowDiskError |
+ if (!callback.is_null()) { |
+ callback.Run(response && response->GetErrorName() == |
+ login_manager::dbus_error::kLowFreeDisk |
? StartArcInstanceResult::LOW_FREE_DISK_SPACE |
: StartArcInstanceResult::UNKNOWN_ERROR); |
+ } |
} |
dbus::ObjectProxy* session_manager_proxy_; |
@@ -890,26 +971,31 @@ class SessionManagerClientStubImpl : public SessionManagerClient { |
void RetrieveDevicePolicy(const RetrievePolicyCallback& callback) override { |
base::FilePath owner_key_path; |
if (!PathService::Get(chromeos::FILE_OWNER_KEY, &owner_key_path)) { |
- callback.Run(""); |
+ callback.Run("", RetrievePolicyResponseType::SUCCESS); |
return; |
} |
base::FilePath device_policy_path = |
owner_key_path.DirName().AppendASCII(kStubDevicePolicyFile); |
base::PostTaskWithTraitsAndReplyWithResult( |
- FROM_HERE, base::TaskTraits() |
- .WithShutdownBehavior( |
- base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN) |
- .MayBlock(), |
- base::Bind(&GetFileContent, device_policy_path), callback); |
+ FROM_HERE, |
+ base::TaskTraits() |
+ .WithShutdownBehavior( |
+ base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN) |
+ .MayBlock(), |
+ base::Bind(&GetFileContent, device_policy_path), |
+ base::Bind(&NotifyOnRetrievePolicySuccess, callback)); |
} |
- std::string BlockingRetrieveDevicePolicy() override { |
+ RetrievePolicyResponseType BlockingRetrieveDevicePolicy( |
+ std::string* policy) override { |
base::FilePath owner_key_path; |
if (!PathService::Get(chromeos::FILE_OWNER_KEY, &owner_key_path)) { |
- return ""; |
+ *policy = ""; |
+ return RetrievePolicyResponseType::SUCCESS; |
} |
base::FilePath device_policy_path = |
owner_key_path.DirName().AppendASCII(kStubDevicePolicyFile); |
- return GetFileContent(device_policy_path); |
+ *policy = GetFileContent(device_policy_path); |
+ return RetrievePolicyResponseType::SUCCESS; |
} |
void RetrievePolicyForUser(const cryptohome::Identification& cryptohome_id, |
const RetrievePolicyCallback& callback) override { |
@@ -921,11 +1007,13 @@ class SessionManagerClientStubImpl : public SessionManagerClient { |
.MayBlock(), |
base::Bind(&GetFileContent, |
GetUserFilePath(cryptohome_id, kStubPolicyFile)), |
- callback); |
+ base::Bind(&NotifyOnRetrievePolicySuccess, callback)); |
} |
- std::string BlockingRetrievePolicyForUser( |
- const cryptohome::Identification& cryptohome_id) override { |
- return GetFileContent(GetUserFilePath(cryptohome_id, kStubPolicyFile)); |
+ RetrievePolicyResponseType BlockingRetrievePolicyForUser( |
+ const cryptohome::Identification& cryptohome_id, |
+ std::string* policy) override { |
+ *policy = GetFileContent(GetUserFilePath(cryptohome_id, kStubPolicyFile)); |
+ return RetrievePolicyResponseType::SUCCESS; |
} |
void RetrieveDeviceLocalAccountPolicy( |
const std::string& account_id, |
@@ -933,10 +1021,11 @@ class SessionManagerClientStubImpl : public SessionManagerClient { |
RetrievePolicyForUser(cryptohome::Identification::FromString(account_id), |
callback); |
} |
- std::string BlockingRetrieveDeviceLocalAccountPolicy( |
- const std::string& account_id) override { |
+ RetrievePolicyResponseType BlockingRetrieveDeviceLocalAccountPolicy( |
+ const std::string& account_id, |
+ std::string* policy) override { |
return BlockingRetrievePolicyForUser( |
- cryptohome::Identification::FromString(account_id)); |
+ cryptohome::Identification::FromString(account_id), policy); |
} |
void StoreDevicePolicy(const std::string& policy_blob, |
const StorePolicyCallback& callback) override { |