shill: serialize Manager.Services as an array of object paths
Previously, we serialized this property as an array of strings.
That was contrary to the API documentation, and caused a crash
in update_engine_client.
BUG=chromium-os:27987
TEST=unit tests, manual
Manual testing: ran on device, and checked that update_engine_client
was able to identify the technology of the default service.
Collateral change: replace PathArray with Paths (and similar
for variations, such as path_array). This makes the naming of
Paths conform to the style used for Strings, Stringmaps, etc.
Change-Id: I4ee4b9df04d3f006ab974c2092f6e515ebf5a9b8
Reviewed-on: https://gerrit.chromium.org/gerrit/18592
Commit-Ready: mukesh agrawal <quiche@chromium.org>
Reviewed-by: mukesh agrawal <quiche@chromium.org>
Tested-by: mukesh agrawal <quiche@chromium.org>
diff --git a/accessor_interface.h b/accessor_interface.h
index 0419dd1..b47278c 100644
--- a/accessor_interface.h
+++ b/accessor_interface.h
@@ -43,6 +43,12 @@
typedef std::vector<uint8_t> ByteArray;
typedef std::vector<ByteArray> ByteArrays;
+// Note that while the RpcIdentifiers type has the same concrete
+// representation as the Strings type, it may be serialized
+// differently. Accordingly, PropertyStore tracks RpcIdentifiers
+// separately from Strings. We create a separate typedef here, to make
+// the PropertyStore-related code read more simply.
+typedef std::vector<std::string> RpcIdentifiers;
typedef std::vector<std::string> Strings;
typedef std::map<std::string, std::string> Stringmap;
typedef std::vector<Stringmap> Stringmaps;
@@ -52,6 +58,11 @@
typedef std::tr1::shared_ptr<AccessorInterface<bool> > BoolAccessor;
typedef std::tr1::shared_ptr<AccessorInterface<int16> > Int16Accessor;
typedef std::tr1::shared_ptr<AccessorInterface<int32> > Int32Accessor;
+// See comment above RpcIdentifiers typedef, for the reason why the
+// RpcIdentifiersAccessor exists (even though it has the same
+// underlying type as StringsAccessor).
+typedef std::tr1::shared_ptr<
+ AccessorInterface<std::vector<std::string> > >RpcIdentifiersAccessor;
typedef std::tr1::shared_ptr<AccessorInterface<std::string> > StringAccessor;
typedef std::tr1::shared_ptr<AccessorInterface<Stringmap> > StringmapAccessor;
typedef std::tr1::shared_ptr<AccessorInterface<Stringmaps> > StringmapsAccessor;
diff --git a/dbus_adaptor.cc b/dbus_adaptor.cc
index a161fbf..084de41 100644
--- a/dbus_adaptor.cc
+++ b/dbus_adaptor.cc
@@ -24,7 +24,7 @@
// static
const char DBusAdaptor::kByteArraysSig[] = "aay";
// static
-const char DBusAdaptor::kPathArraySig[] = "ao";
+const char DBusAdaptor::kPathsSig[] = "ao";
// static
const char DBusAdaptor::kStringmapSig[] = "a{ss}";
// static
@@ -113,6 +113,20 @@
(*out)[it.Key()] = KeyValueStoreToVariant(it.Value(&e));
}
{
+ ReadablePropertyConstIterator<RpcIdentifiers> it =
+ store.GetRpcIdentifiersPropertiesIter();
+ for ( ; !it.AtEnd(); it.Advance()) {
+ Strings rpc_identifiers_as_strings = it.Value(&e);
+ vector < ::DBus::Path> rpc_identifiers_as_paths;
+ for (Strings::const_iterator in = rpc_identifiers_as_strings.begin();
+ in != rpc_identifiers_as_strings.end();
+ ++in) {
+ rpc_identifiers_as_paths.push_back(*in);
+ }
+ (*out)[it.Key()] = PathsToVariant(rpc_identifiers_as_paths);
+ }
+ }
+ {
ReadablePropertyConstIterator<string> it = store.GetStringPropertiesIter();
for ( ; !it.AtEnd(); it.Advance())
(*out)[it.Key()] = StringToVariant(it.Value(&e));
@@ -242,7 +256,7 @@
}
// static
-::DBus::Variant DBusAdaptor::PathArrayToVariant(
+::DBus::Variant DBusAdaptor::PathsToVariant(
const vector< ::DBus::Path> &value) {
::DBus::MessageIter writer;
::DBus::Variant v;
@@ -372,8 +386,8 @@
}
// static
-bool DBusAdaptor::IsPathArray(::DBus::Signature signature) {
- return signature == DBusAdaptor::kPathArraySig;
+bool DBusAdaptor::IsPaths(::DBus::Signature signature) {
+ return signature == DBusAdaptor::kPathsSig;
}
// static
diff --git a/dbus_adaptor.h b/dbus_adaptor.h
index 1e68a8e..7385ddd 100644
--- a/dbus_adaptor.h
+++ b/dbus_adaptor.h
@@ -60,7 +60,7 @@
static ::DBus::Variant Int32ToVariant(int32 value);
static ::DBus::Variant KeyValueStoreToVariant(const KeyValueStore &value);
static ::DBus::Variant PathToVariant(const ::DBus::Path &value);
- static ::DBus::Variant PathArrayToVariant(
+ static ::DBus::Variant PathsToVariant(
const std::vector< ::DBus::Path> &value);
static ::DBus::Variant StringToVariant(const std::string &value);
static ::DBus::Variant StringmapToVariant(const Stringmap &value);
@@ -75,7 +75,7 @@
static bool IsInt16(::DBus::Signature signature);
static bool IsInt32(::DBus::Signature signature);
static bool IsPath(::DBus::Signature signature);
- static bool IsPathArray(::DBus::Signature signature);
+ static bool IsPaths(::DBus::Signature signature);
static bool IsString(::DBus::Signature signature);
static bool IsStringmap(::DBus::Signature signature);
static bool IsStringmaps(::DBus::Signature signature);
@@ -130,7 +130,7 @@
private:
static const char kByteArraysSig[];
- static const char kPathArraySig[];
+ static const char kPathsSig[];
static const char kStringmapSig[];
static const char kStringmapsSig[];
static const char kStringsSig[];
diff --git a/dbus_adaptor_unittest.cc b/dbus_adaptor_unittest.cc
index 80dfa5e..f583dab 100644
--- a/dbus_adaptor_unittest.cc
+++ b/dbus_adaptor_unittest.cc
@@ -73,8 +73,8 @@
ex_stringmap_[ex_string_] = ex_string_;
stringmap_v_ = DBusAdaptor::StringmapToVariant(ex_stringmap_);
- ex_patharray_.push_back(ex_path_);
- patharray_v_ = DBusAdaptor::PathArrayToVariant(ex_patharray_);
+ ex_paths_.push_back(ex_path_);
+ paths_v_ = DBusAdaptor::PathsToVariant(ex_paths_);
}
virtual ~DBusAdaptorTest() {}
@@ -88,7 +88,7 @@
int16 ex_int16_;
int32 ex_int32_;
::DBus::Path ex_path_;
- vector< ::DBus::Path> ex_patharray_;
+ vector< ::DBus::Path> ex_paths_;
string ex_string_;
map<string, string> ex_stringmap_;
vector<map<string, string> > ex_stringmaps_;
@@ -100,7 +100,7 @@
::DBus::Variant int16_v_;
::DBus::Variant int32_v_;
::DBus::Variant path_v_;
- ::DBus::Variant patharray_v_;
+ ::DBus::Variant paths_v_;
::DBus::Variant string_v_;
::DBus::Variant stringmap_v_;
::DBus::Variant stringmaps_v_;
@@ -135,7 +135,7 @@
EXPECT_EQ(ex_path_, path_v_.reader().get_path());
- EXPECT_EQ(ex_path_, patharray_v_.operator vector< ::DBus::Path>()[0]);
+ EXPECT_EQ(ex_path_, paths_v_.operator vector< ::DBus::Path>()[0]);
EXPECT_EQ(string(""), PropertyStoreTest::kStringV.reader().get_string());
EXPECT_EQ(ex_string_, string_v_.reader().get_string());
@@ -152,7 +152,7 @@
EXPECT_TRUE(DBusAdaptor::IsInt16(int16_v_.signature()));
EXPECT_TRUE(DBusAdaptor::IsInt32(int32_v_.signature()));
EXPECT_TRUE(DBusAdaptor::IsPath(path_v_.signature()));
- EXPECT_TRUE(DBusAdaptor::IsPathArray(patharray_v_.signature()));
+ EXPECT_TRUE(DBusAdaptor::IsPaths(paths_v_.signature()));
EXPECT_TRUE(DBusAdaptor::IsString(string_v_.signature()));
EXPECT_TRUE(DBusAdaptor::IsStringmap(stringmap_v_.signature()));
EXPECT_TRUE(DBusAdaptor::IsStrings(strings_v_.signature()));
diff --git a/manager.cc b/manager.cc
index 67ceba1..ab25659 100644
--- a/manager.cc
+++ b/manager.cc
@@ -109,9 +109,8 @@
HelpRegisterDerivedString(flimflam::kStateProperty,
&Manager::CalculateState,
NULL);
- HelpRegisterDerivedStrings(flimflam::kServicesProperty,
- &Manager::EnumerateAvailableServices,
- NULL);
+ HelpRegisterConstDerivedRpcIdentifiers(flimflam::kServicesProperty,
+ &Manager::EnumerateAvailableServices);
HelpRegisterDerivedStrings(flimflam::kServiceWatchListProperty,
&Manager::EnumerateWatchedServices,
NULL);
@@ -561,6 +560,15 @@
return NULL;
}
+void Manager::HelpRegisterConstDerivedRpcIdentifiers(
+ const string &name,
+ RpcIdentifiers(Manager::*get)(Error *)) {
+ store_.RegisterDerivedRpcIdentifiers(
+ name,
+ RpcIdentifiersAccessor(
+ new CustomAccessor<Manager, RpcIdentifiers>(this, get, NULL)));
+}
+
void Manager::HelpRegisterDerivedString(
const string &name,
string(Manager::*get)(Error *),
diff --git a/manager.h b/manager.h
index d4ddbca..85d7c6d 100644
--- a/manager.h
+++ b/manager.h
@@ -187,6 +187,9 @@
std::vector<std::string> EnumerateWatchedServices(Error *error);
std::string GetActiveProfileRpcIdentifier(Error *error);
+ void HelpRegisterConstDerivedRpcIdentifiers(
+ const std::string &name,
+ RpcIdentifiers(Manager::*get)(Error *));
void HelpRegisterDerivedString(
const std::string &name,
std::string(Manager::*get)(Error *),
diff --git a/manager_dbus_adaptor.cc b/manager_dbus_adaptor.cc
index 57a6991..75127d6 100644
--- a/manager_dbus_adaptor.cc
+++ b/manager_dbus_adaptor.cc
@@ -69,7 +69,7 @@
paths.push_back(*it);
}
- PropertyChanged(name, DBusAdaptor::PathArrayToVariant(paths));
+ PropertyChanged(name, DBusAdaptor::PathsToVariant(paths));
}
void ManagerDBusAdaptor::EmitStateChanged(const string &new_state) {
diff --git a/manager_unittest.cc b/manager_unittest.cc
index 2567985..ce74e2e 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -442,6 +442,19 @@
}
}
+TEST_F(ManagerTest, GetServicesProperty) {
+ ProfileRefPtr profile(new MockProfile(control_interface(), manager(), ""));
+ AdoptProfile(manager(), profile);
+ map<string, ::DBus::Variant> props;
+ ::DBus::Error dbus_error;
+ DBusAdaptor::GetProperties(manager()->store(), &props, &dbus_error);
+ map<string, ::DBus::Variant>::const_iterator prop =
+ props.find(flimflam::kServicesProperty);
+ ASSERT_FALSE(prop == props.end());
+ const ::DBus::Variant &variant = prop->second;
+ ASSERT_TRUE(DBusAdaptor::IsPaths(variant.signature()));
+}
+
TEST_F(ManagerTest, MoveService) {
Manager manager(control_interface(),
dispatcher(),
diff --git a/property_store.cc b/property_store.cc
index d3bd527..8e8a35c 100644
--- a/property_store.cc
+++ b/property_store.cc
@@ -147,6 +147,12 @@
ReadablePropertyConstIterator<KeyValueStore>(key_value_store_properties_);
}
+ReadablePropertyConstIterator<RpcIdentifiers>
+PropertyStore::GetRpcIdentifiersPropertiesIter() const {
+ return ReadablePropertyConstIterator<RpcIdentifiers>(
+ rpc_identifiers_properties_);
+}
+
ReadablePropertyConstIterator<string>
PropertyStore::GetStringPropertiesIter() const {
return ReadablePropertyConstIterator<string>(string_properties_);
@@ -400,6 +406,14 @@
key_value_store_properties_[name] = acc;
}
+void PropertyStore::RegisterDerivedRpcIdentifiers(
+ const string &name,
+ const RpcIdentifiersAccessor &accessor) {
+ DCHECK(!Contains(name) || ContainsKey(rpc_identifiers_properties_, name))
+ << "(Already registered " << name << ")";
+ rpc_identifiers_properties_[name] = accessor;
+}
+
void PropertyStore::RegisterDerivedString(const string &name,
const StringAccessor &accessor) {
DCHECK(!Contains(name) || ContainsKey(string_properties_, name))
diff --git a/property_store.h b/property_store.h
index 6c8934e..f3c0b2e 100644
--- a/property_store.h
+++ b/property_store.h
@@ -94,6 +94,8 @@
ReadablePropertyConstIterator<int32> GetInt32PropertiesIter() const;
ReadablePropertyConstIterator<KeyValueStore> GetKeyValueStorePropertiesIter(
) const;
+ ReadablePropertyConstIterator<RpcIdentifiers> GetRpcIdentifiersPropertiesIter(
+ ) const;
ReadablePropertyConstIterator<std::string> GetStringPropertiesIter() const;
ReadablePropertyConstIterator<Stringmap> GetStringmapPropertiesIter() const;
ReadablePropertyConstIterator<Stringmaps> GetStringmapsPropertiesIter() const;
@@ -150,6 +152,8 @@
const Int32Accessor &accessor);
void RegisterDerivedKeyValueStore(const std::string &name,
const KeyValueStoreAccessor &accessor);
+ void RegisterDerivedRpcIdentifiers(const std::string &name,
+ const RpcIdentifiersAccessor &accessor);
void RegisterDerivedString(const std::string &name,
const StringAccessor &accessor);
void RegisterDerivedStringmaps(const std::string &name,
@@ -174,6 +178,7 @@
std::map<std::string, Int16Accessor> int16_properties_;
std::map<std::string, Int32Accessor> int32_properties_;
std::map<std::string, KeyValueStoreAccessor> key_value_store_properties_;
+ std::map<std::string, RpcIdentifiersAccessor> rpc_identifiers_properties_;
std::map<std::string, StringAccessor> string_properties_;
std::map<std::string, StringmapAccessor> stringmap_properties_;
std::map<std::string, StringmapsAccessor> stringmaps_properties_;