Skip to content

Commit

Permalink
Cleanup DBus interface.
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Reviewed-by: David Zeuthen <[email protected]>
Reviewed-by: Alex Deymo <[email protected]>
Commit-Queue: Gilad Arnold <[email protected]>
  • Loading branch information
Gilad Arnold authored and chrome-internal-fetch committed Mar 10, 2014
1 parent 01ca1fb commit b752fb3
Show file tree
Hide file tree
Showing 6 changed files with 185 additions and 333 deletions.
148 changes: 49 additions & 99 deletions chrome_browser_proxy_resolver.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,56 +45,38 @@ const char kLibCrosProxyResolveSignalFilter[] = "type='signal', "
#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;
}
const int kTimeout = 5; // seconds

} // 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,
Expand All @@ -105,24 +87,21 @@ bool ChromeBrowserProxyResolver::Init() {
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) {
Expand All @@ -137,50 +116,22 @@ bool ChromeBrowserProxyResolver::GetProxiesForUrl(const string& url,
GError* error = NULL;
guint timeout = timeout_;
if (proxy_) {
bool dbus_success = true;
bool dbus_reinit = false;
bool dbus_got_error;

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 (!dbus_->ProxyCall_3_0(proxy_,
kLibCrosServiceResolveNetworkProxyMethodName,
&error,
url.c_str(),
kLibCrosProxyResolveSignalInterface,
kLibCrosProxyResolveName)) {

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;
Expand Down Expand Up @@ -212,11 +163,10 @@ DBusHandlerResult ChromeBrowserProxyResolver::FilterMessage(
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;
}
Expand Down
36 changes: 15 additions & 21 deletions chrome_browser_proxy_resolver_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,32 +124,30 @@ void RunTest(bool chrome_replies, bool chrome_alive) {
MockDbusGlib dbus_iface;

EXPECT_CALL(dbus_iface, BusGet(_, _))
.Times(chrome_alive ? 3 : 2)
.Times(2)
.WillRepeatedly(Return(kMockSystemGBus));
EXPECT_CALL(dbus_iface,
ConnectionGetConnection(kMockSystemGBus))
.Times(chrome_alive ? 2 : 1)
.Times(2)
.WillRepeatedly(Return(kMockSystemBus));
EXPECT_CALL(dbus_iface, DbusBusAddMatch(kMockSystemBus, _, _));
EXPECT_CALL(dbus_iface,
DbusConnectionAddFilter(kMockSystemBus, _, _, _))
.WillOnce(Return(1));
EXPECT_CALL(dbus_iface,
ProxyNewForNameOwner(kMockSystemGBus,
StrEq(kLibCrosServiceName),
StrEq(kLibCrosServicePath),
StrEq(kLibCrosServiceInterface),
_))
.WillOnce(Return(chrome_alive ? kMockDbusProxy : NULL));
ProxyNewForName(kMockSystemGBus,
StrEq(kLibCrosServiceName),
StrEq(kLibCrosServicePath),
StrEq(kLibCrosServiceInterface)))
.WillOnce(Return(kMockDbusProxy));
if (chrome_alive)
EXPECT_CALL(dbus_iface, ProxyCall(
EXPECT_CALL(dbus_iface, ProxyCall_3_0(
kMockDbusProxy,
StrEq(kLibCrosServiceResolveNetworkProxyMethodName),
_,
G_TYPE_STRING, StrEq(kUrl),
G_TYPE_STRING, StrEq(kLibCrosProxyResolveSignalInterface),
G_TYPE_STRING, StrEq(kLibCrosProxyResolveName),
G_TYPE_INVALID))
StrEq(kUrl),
StrEq(kLibCrosProxyResolveSignalInterface),
StrEq(kLibCrosProxyResolveName)))
.WillOnce(Return(chrome_alive ? TRUE : FALSE));
EXPECT_CALL(dbus_iface,
DbusConnectionRemoveFilter(kMockSystemBus, _, _));
Expand All @@ -160,21 +158,17 @@ void RunTest(bool chrome_replies, bool chrome_alive) {
kLibCrosProxyResolveName))
.WillOnce(Return(1));
EXPECT_CALL(dbus_iface,
DbusMessageGetArgs(kMockDbusMessage, _,
DBUS_TYPE_STRING, _,
DBUS_TYPE_STRING, _,
DBUS_TYPE_STRING, _,
DBUS_TYPE_INVALID))
.WillOnce(DoAll(SetArgumentPointee<3>(strdup(kUrl)),
SetArgumentPointee<5>(
DbusMessageGetArgs_3(kMockDbusMessage, _, _, _, _))
.WillOnce(DoAll(SetArgumentPointee<2>(strdup(kUrl)),
SetArgumentPointee<3>(
strdup("SOCKS5 192.168.52.83:5555;DIRECT")),
Return(TRUE)));
}

GMainLoop* loop = g_main_loop_new(g_main_context_default(), FALSE);

ChromeBrowserProxyResolver resolver(&dbus_iface);
EXPECT_EQ(chrome_alive, resolver.Init());
EXPECT_EQ(true, resolver.Init());
resolver.set_timeout(1);
SendReplyArgs args = {
kMockSystemBus,
Expand Down
25 changes: 6 additions & 19 deletions connection_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,8 @@ bool GetFlimFlamProxy(DbusGlibInterface* dbus_iface,
LOG(ERROR) << "Failed to get system bus";
return false;
}
proxy = dbus_iface->ProxyNewForNameOwner(bus,
shill::kFlimflamServiceName,
path,
interface,
&error);
if (!proxy) {
LOG(ERROR) << "Error getting FlimFlam proxy: "
<< utils::GetAndFreeGError(&error);
return false;
}
proxy = dbus_iface->ProxyNewForName(bus, shill::kFlimflamServiceName, path,
interface);
*out_proxy = proxy;
return true;
}
Expand All @@ -65,15 +57,10 @@ bool GetProperties(DbusGlibInterface* dbus_iface,
interface,
&proxy));

gboolean rc = dbus_iface->ProxyCall(proxy,
"GetProperties",
&error,
G_TYPE_INVALID,
dbus_g_type_get_map("GHashTable",
G_TYPE_STRING,
G_TYPE_VALUE),
out_hash_table,
G_TYPE_INVALID);
gboolean rc = dbus_iface->ProxyCall_0_1(proxy,
"GetProperties",
&error,
out_hash_table);
dbus_iface->ProxyUnref(proxy);
if (rc == FALSE) {
LOG(ERROR) << "dbus_g_proxy_call failed";
Expand Down
46 changes: 16 additions & 30 deletions connection_manager_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,24 +91,17 @@ void ConnectionManagerTest::SetManagerReply(gconstpointer reply_value,
array_as_value);

// Plumb return value into mock object.
EXPECT_CALL(dbus_iface_, ProxyCall(kMockFlimFlamManagerProxy_,
StrEq(kGetPropertiesMethod),
_,
G_TYPE_INVALID,
dbus_g_type_get_map("GHashTable",
G_TYPE_STRING,
G_TYPE_VALUE),
_,
G_TYPE_INVALID))
.WillOnce(DoAll(SetArgumentPointee<5>(manager_hash_table), Return(TRUE)));
EXPECT_CALL(dbus_iface_, ProxyCall_0_1(kMockFlimFlamManagerProxy_,
StrEq(kGetPropertiesMethod),
_, _))
.WillOnce(DoAll(SetArgumentPointee<3>(manager_hash_table), Return(TRUE)));

// Set other expectations.
EXPECT_CALL(dbus_iface_,
ProxyNewForNameOwner(kMockSystemBus_,
StrEq(shill::kFlimflamServiceName),
StrEq(shill::kFlimflamServicePath),
StrEq(shill::kFlimflamManagerInterface),
_))
ProxyNewForName(kMockSystemBus_,
StrEq(shill::kFlimflamServiceName),
StrEq(shill::kFlimflamServicePath),
StrEq(shill::kFlimflamManagerInterface)))
.WillOnce(Return(kMockFlimFlamManagerProxy_));
EXPECT_CALL(dbus_iface_, ProxyUnref(kMockFlimFlamManagerProxy_));
EXPECT_CALL(dbus_iface_, BusGet(DBUS_BUS_SYSTEM, _))
Expand Down Expand Up @@ -142,24 +135,17 @@ void ConnectionManagerTest::SetServiceReply(const char* service_type,
}

// Plumb return value into mock object.
EXPECT_CALL(dbus_iface_, ProxyCall(kMockFlimFlamServiceProxy_,
StrEq(kGetPropertiesMethod),
_,
G_TYPE_INVALID,
dbus_g_type_get_map("GHashTable",
G_TYPE_STRING,
G_TYPE_VALUE),
_,
G_TYPE_INVALID))
.WillOnce(DoAll(SetArgumentPointee<5>(service_hash_table), Return(TRUE)));
EXPECT_CALL(dbus_iface_, ProxyCall_0_1(kMockFlimFlamServiceProxy_,
StrEq(kGetPropertiesMethod),
_, _))
.WillOnce(DoAll(SetArgumentPointee<3>(service_hash_table), Return(TRUE)));

// Set other expectations.
EXPECT_CALL(dbus_iface_,
ProxyNewForNameOwner(kMockSystemBus_,
StrEq(shill::kFlimflamServiceName),
StrEq(kServicePath_),
StrEq(shill::kFlimflamServiceInterface),
_))
ProxyNewForName(kMockSystemBus_,
StrEq(shill::kFlimflamServiceName),
StrEq(kServicePath_),
StrEq(shill::kFlimflamServiceInterface)))
.WillOnce(Return(kMockFlimFlamServiceProxy_));
EXPECT_CALL(dbus_iface_, ProxyUnref(kMockFlimFlamServiceProxy_));
EXPECT_CALL(dbus_iface_, BusGet(DBUS_BUS_SYSTEM, _))
Expand Down
Loading

0 comments on commit b752fb3

Please sign in to comment.