Cleanup DBus interface.
Prior to importing the DBus interface into the Policy Manager, this
removes some clutter and fixes some behaviors.
* Switch from using dbus_g_proxy_new_for_name_owner() to using
dbus_g_proxy_new_for_name() instead. This has been advised in the past
(see chromium:209102) as the _owner version is mostly used for binding
to interfaces that do not retain the well-known name. It is not
appropriate for our use cases, where the provider inhibits the
well-known location constantly, nor is it a good thing given that
providers (e.g. Chrome) could get recycled.
* More consistent abstraction for variadic DBus functions: (a) We now
distinguish between different numbers of input/output arguments by
appending these numbers to the names of interface functions (e.g.
ProxyCall_3_0 delegates 3 input arguments and zero outputs); (b) We
drop G_TYPE arguments and embed them as constants in the delegating
code. This makes more sense because these types are constants and
depend on the actual values, which are bound to predetermined C/C++
types anyway; (c) It is still possible to override such functions by
variating the types of actual arguments (currently not exercised).
* The above also shortens the argument list for several DBus interface
functions, saving us from needing to prune them to fit in mock methods
with a maximum of 10 arguments (as was previously necessary).
* Removed an unnecessary #include; better comments; more descriptive
argument names.
Other notable changes in client code:
* Some cleaup in chrome_browser_proxy_resolver.cc, removing unnecessary
functions and reverting the proxy reacquisition logic introduced in
CL:15693, which is now redundant.
BUG=None
TEST=Unit tests.
Change-Id: I8063bb3e35c34212a8be1ae507834c931ee5a0b0
Reviewed-on: https://chromium-review.googlesource.com/188560
Tested-by: Gilad Arnold <garnold@chromium.org>
Reviewed-by: David Zeuthen <zeuthen@chromium.org>
Reviewed-by: Alex Deymo <deymo@chromium.org>
Commit-Queue: Gilad Arnold <garnold@chromium.org>
diff --git a/chrome_browser_proxy_resolver.cc b/chrome_browser_proxy_resolver.cc
index 3c1fe23..3841fe9 100644
--- a/chrome_browser_proxy_resolver.cc
+++ b/chrome_browser_proxy_resolver.cc
@@ -45,56 +45,38 @@
#undef LIB_CROS_PROXY_RESOLVE_NAME
namespace {
+
const int kTimeout = 5; // seconds
-DBusGProxy* GetProxy(DbusGlibInterface* dbus) {
- GError* error = NULL;
-
- DBusGConnection* bus = dbus->BusGet(DBUS_BUS_SYSTEM, &error);
- if (!bus) {
- LOG(ERROR) << "Failed to get bus";
- return NULL;
- }
- DBusGProxy* proxy = dbus->ProxyNewForNameOwner(bus,
- kLibCrosServiceName,
- kLibCrosServicePath,
- kLibCrosServiceInterface,
- &error);
- if (!proxy) {
- LOG(ERROR) << "Error getting dbus proxy for " << kLibCrosServiceName << ": "
- << utils::GetAndFreeGError(&error);
- return NULL;
- }
- return proxy;
-}
-
} // namespace {}
ChromeBrowserProxyResolver::ChromeBrowserProxyResolver(DbusGlibInterface* dbus)
: dbus_(dbus), proxy_(NULL), timeout_(kTimeout) {}
bool ChromeBrowserProxyResolver::Init() {
- // Set up signal handler. Code lifted from libcros
- if (proxy_) {
- // Already initialized
- return true;
- }
- GError* gerror = NULL;
- DBusGConnection* gbus = dbus_->BusGet(DBUS_BUS_SYSTEM, &gerror);
- TEST_AND_RETURN_FALSE(gbus);
- DBusConnection* connection = dbus_->ConnectionGetConnection(gbus);
+ if (proxy_)
+ return true; // Already initialized.
+
+ // Set up signal handler. Code lifted from libcros.
+ GError* g_error = NULL;
+ DBusGConnection* bus = dbus_->BusGet(DBUS_BUS_SYSTEM, &g_error);
+ TEST_AND_RETURN_FALSE(bus);
+ DBusConnection* connection = dbus_->ConnectionGetConnection(bus);
TEST_AND_RETURN_FALSE(connection);
- DBusError error;
- dbus_error_init(&error);
- dbus_->DbusBusAddMatch(connection, kLibCrosProxyResolveSignalFilter, &error);
- TEST_AND_RETURN_FALSE(!dbus_error_is_set(&error));
+
+ DBusError dbus_error;
+ dbus_error_init(&dbus_error);
+ dbus_->DbusBusAddMatch(connection, kLibCrosProxyResolveSignalFilter,
+ &dbus_error);
+ TEST_AND_RETURN_FALSE(!dbus_error_is_set(&dbus_error));
TEST_AND_RETURN_FALSE(dbus_->DbusConnectionAddFilter(
connection,
&ChromeBrowserProxyResolver::StaticFilterMessage,
this,
NULL));
- proxy_ = GetProxy(dbus_);
+ proxy_ = dbus_->ProxyNewForName(bus, kLibCrosServiceName, kLibCrosServicePath,
+ kLibCrosServiceInterface);
if (!proxy_) {
dbus_->DbusConnectionRemoveFilter(
connection,
@@ -105,24 +87,21 @@
return true;
}
-void ChromeBrowserProxyResolver::Shutdown() {
- if (!proxy_)
- return;
-
- GError* gerror = NULL;
- DBusGConnection* gbus = dbus_->BusGet(DBUS_BUS_SYSTEM, &gerror);
- TEST_AND_RETURN(gbus);
- DBusConnection* connection = dbus_->ConnectionGetConnection(gbus);
- dbus_->DbusConnectionRemoveFilter(
- connection,
- &ChromeBrowserProxyResolver::StaticFilterMessage,
- this);
- proxy_ = NULL;
-}
-
ChromeBrowserProxyResolver::~ChromeBrowserProxyResolver() {
- // Kill proxy object.
- Shutdown();
+ // Remove DBus connection filters and Kill proxy object.
+ if (proxy_) {
+ GError* gerror = NULL;
+ DBusGConnection* gbus = dbus_->BusGet(DBUS_BUS_SYSTEM, &gerror);
+ if (gbus) {
+ DBusConnection* connection = dbus_->ConnectionGetConnection(gbus);
+ dbus_->DbusConnectionRemoveFilter(
+ connection,
+ &ChromeBrowserProxyResolver::StaticFilterMessage,
+ this);
+ }
+ dbus_->ProxyUnref(proxy_);
+ }
+
// Kill outstanding timers
for (TimeoutsMap::iterator it = timers_.begin(), e = timers_.end(); it != e;
++it) {
@@ -137,50 +116,22 @@
GError* error = NULL;
guint timeout = timeout_;
if (proxy_) {
- bool dbus_success = true;
- bool dbus_reinit = false;
- bool dbus_got_error;
+ if (!dbus_->ProxyCall_3_0(proxy_,
+ kLibCrosServiceResolveNetworkProxyMethodName,
+ &error,
+ url.c_str(),
+ kLibCrosProxyResolveSignalInterface,
+ kLibCrosProxyResolveName)) {
- do {
- if (!dbus_success) {
- // We failed with a null error, time to re-init the dbus proxy.
- LOG(WARNING) << "attempting to reinitialize dbus proxy and retrying";
- Shutdown();
- if (!Init()) {
- LOG(WARNING) << "failed to reinitialize the dbus proxy";
- break;
- }
- dbus_reinit = true;
- }
-
- dbus_success = dbus_->ProxyCall(
- proxy_,
- kLibCrosServiceResolveNetworkProxyMethodName,
- &error,
- G_TYPE_STRING, url.c_str(),
- G_TYPE_STRING, kLibCrosProxyResolveSignalInterface,
- G_TYPE_STRING, kLibCrosProxyResolveName,
- G_TYPE_INVALID, G_TYPE_INVALID);
-
- dbus_got_error = false;
-
- if (dbus_success) {
- LOG(INFO) << "dbus_g_proxy_call succeeded!";
+ if (error) {
+ LOG(WARNING) << "dbus_g_proxy_call failed, continuing with no proxy: "
+ << utils::GetAndFreeGError(&error);
} else {
- if (error) {
- // Register the fact that we did receive an error, as it is nullified
- // on the next line.
- dbus_got_error = true;
- LOG(WARNING) << "dbus_g_proxy_call failed: "
- << utils::GetAndFreeGError(&error)
- << " Continuing with no proxy.";
- } else {
- LOG(WARNING) << "dbus_g_proxy_call failed with no error string, "
- "continuing with no proxy.";
- }
- timeout = 0;
+ LOG(WARNING) << "dbus_g_proxy_call failed with no error string, "
+ "continuing with no proxy.";
}
- } while (!(dbus_success || dbus_got_error || dbus_reinit));
+ timeout = 0;
+ }
} else {
LOG(WARNING) << "dbus proxy object missing, continuing with no proxy.";
timeout = 0;
@@ -212,11 +163,10 @@
char* error = NULL;
DBusError arg_error;
dbus_error_init(&arg_error);
- if (!dbus_->DbusMessageGetArgs(message, &arg_error,
- DBUS_TYPE_STRING, &source_url,
- DBUS_TYPE_STRING, &proxy_list,
- DBUS_TYPE_STRING, &error,
- DBUS_TYPE_INVALID)) {
+ if (!dbus_->DbusMessageGetArgs_3(message, &arg_error,
+ &source_url,
+ &proxy_list,
+ &error)) {
LOG(ERROR) << "Error reading dbus signal.";
return DBUS_HANDLER_RESULT_HANDLED;
}