shill: Avoid double gets of property store values when iterating.

Cleanup and simplify the property iterator classes by removing the
unused PropertyConstIterator class and caching the property
value. This way we avoid deriving property values twice when iterating
through properties. Also, remove some incorrect and unused logic in
the PropertyStoreInspector related to handling errors coming from the
property store iterators.

BUG=chromium-os:30373
TEST=unit tests

Change-Id: Ie6ba2696f070e1ed43997f17273f48360f85055b
Reviewed-on: https://gerrit.chromium.org/gerrit/24201
Tested-by: Darin Petkov <petkov@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Reviewed-by: mukesh agrawal <quiche@chromium.org>
Commit-Ready: Darin Petkov <petkov@chromium.org>
diff --git a/dbus_adaptor.cc b/dbus_adaptor.cc
index 5a2f855..ae7064d 100644
--- a/dbus_adaptor.cc
+++ b/dbus_adaptor.cc
@@ -95,26 +95,25 @@
 bool DBusAdaptor::GetProperties(const PropertyStore &store,
                                 map<string, ::DBus::Variant> *out,
                                 ::DBus::Error */*error*/) {
-  Error e;
   {
     ReadablePropertyConstIterator<bool> it = store.GetBoolPropertiesIter();
     for ( ; !it.AtEnd(); it.Advance()) {
       SLOG(DBus, 5) << __func__ << " serializing bool " << it.Key();
-      (*out)[it.Key()] = BoolToVariant(it.Value(&e));
+      (*out)[it.Key()] = BoolToVariant(it.value());
     }
   }
   {
     ReadablePropertyConstIterator<int16> it = store.GetInt16PropertiesIter();
     for ( ; !it.AtEnd(); it.Advance()) {
       SLOG(DBus, 5) << __func__ << " serializing int16 " << it.Key();
-      (*out)[it.Key()] = Int16ToVariant(it.Value(&e));
+      (*out)[it.Key()] = Int16ToVariant(it.value());
     }
   }
   {
     ReadablePropertyConstIterator<int32> it = store.GetInt32PropertiesIter();
     for ( ; !it.AtEnd(); it.Advance()) {
       SLOG(DBus, 5) << __func__ << " serializing int32 " << it.Key();
-      (*out)[it.Key()] = Int32ToVariant(it.Value(&e));
+      (*out)[it.Key()] = Int32ToVariant(it.value());
     }
   }
   {
@@ -122,14 +121,14 @@
         store.GetKeyValueStorePropertiesIter();
     for ( ; !it.AtEnd(); it.Advance()) {
       SLOG(DBus, 5) << __func__ << " serializing KeyValueStore " << it.Key();
-      (*out)[it.Key()] = KeyValueStoreToVariant(it.Value(&e));
+      (*out)[it.Key()] = KeyValueStoreToVariant(it.value());
     }
   }
   {
     ReadablePropertyConstIterator<RpcIdentifiers> it =
         store.GetRpcIdentifiersPropertiesIter();
     for ( ; !it.AtEnd(); it.Advance()) {
-      Strings rpc_identifiers_as_strings  = it.Value(&e);
+      const Strings &rpc_identifiers_as_strings  = it.value();
       vector < ::DBus::Path> rpc_identifiers_as_paths;
       for (Strings::const_iterator in = rpc_identifiers_as_strings.begin();
            in != rpc_identifiers_as_strings.end();
@@ -144,7 +143,7 @@
     ReadablePropertyConstIterator<string> it = store.GetStringPropertiesIter();
     for ( ; !it.AtEnd(); it.Advance()) {
       SLOG(DBus, 5) << __func__ << " serializing string " << it.Key();
-      (*out)[it.Key()] = StringToVariant(it.Value(&e));
+      (*out)[it.Key()] = StringToVariant(it.value());
     }
   }
   {
@@ -152,7 +151,7 @@
         store.GetStringmapPropertiesIter();
     for ( ; !it.AtEnd(); it.Advance()) {
       SLOG(DBus, 5) << __func__ << " serializing Stringmap " << it.Key();
-      (*out)[it.Key()]= StringmapToVariant(it.Value(&e));
+      (*out)[it.Key()]= StringmapToVariant(it.value());
     }
   }
   {
@@ -160,7 +159,7 @@
         store.GetStringmapsPropertiesIter();
     for ( ; !it.AtEnd(); it.Advance()) {
       SLOG(DBus, 5) << __func__ << " serializing Stringmaps " << it.Key();
-      (*out)[it.Key()]= StringmapsToVariant(it.Value(&e));
+      (*out)[it.Key()]= StringmapsToVariant(it.value());
     }
   }
   {
@@ -168,28 +167,28 @@
         store.GetStringsPropertiesIter();
     for ( ; !it.AtEnd(); it.Advance()) {
       SLOG(DBus, 5) << __func__ << " serializing Strings " << it.Key();
-      (*out)[it.Key()] = StringsToVariant(it.Value(&e));
+      (*out)[it.Key()] = StringsToVariant(it.value());
     }
   }
   {
     ReadablePropertyConstIterator<uint8> it = store.GetUint8PropertiesIter();
     for ( ; !it.AtEnd(); it.Advance()) {
       SLOG(DBus, 5) << __func__ << " serializing uint8 " << it.Key();
-      (*out)[it.Key()] = ByteToVariant(it.Value(&e));
+      (*out)[it.Key()] = ByteToVariant(it.value());
     }
   }
   {
     ReadablePropertyConstIterator<uint16> it = store.GetUint16PropertiesIter();
     for ( ; !it.AtEnd(); it.Advance()) {
       SLOG(DBus, 5) << __func__ << " serializing uint16 " << it.Key();
-      (*out)[it.Key()] = Uint16ToVariant(it.Value(&e));
+      (*out)[it.Key()] = Uint16ToVariant(it.value());
     }
   }
   {
     ReadablePropertyConstIterator<uint32> it = store.GetUint32PropertiesIter();
     for ( ; !it.AtEnd(); it.Advance()) {
       SLOG(DBus, 5) << __func__ << " serializing uint32 " << it.Key();
-      (*out)[it.Key()] = Uint32ToVariant(it.Value(&e));
+      (*out)[it.Key()] = Uint32ToVariant(it.value());
     }
   }
   {
@@ -197,7 +196,7 @@
         store.GetRpcIdentifierPropertiesIter();
     for ( ; !it.AtEnd(); it.Advance()) {
       SLOG(DBus, 5) << __func__ << " serializing RpcIdentifier " << it.Key();
-      (*out)[it.Key()] = PathToVariant(it.Value(&e));
+      (*out)[it.Key()] = PathToVariant(it.value());
     }
   }
   return true;
diff --git a/l2tp_ipsec_driver_unittest.cc b/l2tp_ipsec_driver_unittest.cc
index 631e7fb..48713cd 100644
--- a/l2tp_ipsec_driver_unittest.cc
+++ b/l2tp_ipsec_driver_unittest.cc
@@ -440,21 +440,21 @@
   driver_->InitPropertyStore(&store);
   PropertyStoreInspector inspector(&store);
   {
-    Error error;
     KeyValueStore props;
-    EXPECT_TRUE(inspector.GetKeyValueStoreProperty(
-        flimflam::kProviderProperty, &props, &error));
+    EXPECT_TRUE(
+        inspector.GetKeyValueStoreProperty(
+            flimflam::kProviderProperty, &props));
     EXPECT_TRUE(props.LookupBool(flimflam::kPassphraseRequiredProperty, false));
     EXPECT_TRUE(
         props.LookupBool(flimflam::kL2tpIpsecPskRequiredProperty, false));
   }
   {
-    Error error;
     KeyValueStore props;
     SetArg(flimflam::kL2tpIpsecPasswordProperty, "random-password");
     SetArg(flimflam::kL2tpIpsecPskProperty, "random-psk");
-    EXPECT_TRUE(inspector.GetKeyValueStoreProperty(
-        flimflam::kProviderProperty, &props, &error));
+    EXPECT_TRUE(
+        inspector.GetKeyValueStoreProperty(
+            flimflam::kProviderProperty, &props));
     EXPECT_FALSE(props.LookupBool(flimflam::kPassphraseRequiredProperty, true));
     EXPECT_FALSE(
         props.LookupBool(flimflam::kL2tpIpsecPskRequiredProperty, true));
diff --git a/openvpn_driver_unittest.cc b/openvpn_driver_unittest.cc
index 1f2a3fa..1e5431d 100644
--- a/openvpn_driver_unittest.cc
+++ b/openvpn_driver_unittest.cc
@@ -697,20 +697,18 @@
   driver_->InitPropertyStore(&store);
   PropertyStoreInspector inspector(&store);
   {
-    Error error;
     KeyValueStore props;
     EXPECT_TRUE(
         inspector.GetKeyValueStoreProperty(
-            flimflam::kProviderProperty, &props, &error));
+            flimflam::kProviderProperty, &props));
     EXPECT_TRUE(props.LookupBool(flimflam::kPassphraseRequiredProperty, false));
   }
   {
-    Error error;
     KeyValueStore props;
     SetArg(flimflam::kOpenVPNPasswordProperty, "random-password");
     EXPECT_TRUE(
         inspector.GetKeyValueStoreProperty(
-            flimflam::kProviderProperty, &props, &error));
+            flimflam::kProviderProperty, &props));
     EXPECT_FALSE(props.LookupBool(flimflam::kPassphraseRequiredProperty, true));
   }
 }
diff --git a/property_iterator.h b/property_iterator.h
index c1c47de..bb252c0 100644
--- a/property_iterator.h
+++ b/property_iterator.h
@@ -1,9 +1,9 @@
-// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef SHILL_PROPERTY_ITERATOR_
-#define SHILL_PROPERTY_ITERATOR_
+#ifndef SHILL_PROPERTY_ITERATOR_H_
+#define SHILL_PROPERTY_ITERATOR_H_
 
 #include <map>
 #include <string>
@@ -11,69 +11,29 @@
 #include "shill/accessor_interface.h"
 #include "shill/error.h"
 
-class Error;
-
 namespace shill {
 
 // An iterator wrapper class to hide the details of what kind of data structure
-// we're using to store key/value pairs for properties.
-// Intended for use with PropertyStore.
-
-// TODO(gauravsh): Consider getting rid of PropertyConstIterator, and just
-// keeping ReadablePropertyConstIterator since it doesn't look like the caller
-// is ever interested in anything but readable properties.
+// we're using to store key/value pairs for properties. It is intended for use
+// with PropertyStore and always advances to the next readable property.
 template <class V>
-class PropertyConstIterator {
+class ReadablePropertyConstIterator {
  public:
-  virtual ~PropertyConstIterator() {}
+  ~ReadablePropertyConstIterator() {}
 
-  virtual void Advance() {
-    if (!AtEnd())
-      ++it_;
+  bool AtEnd() const { return it_ == collection_.end(); }
+
+  void Advance() {
+    if (!AtEnd()) {
+      do {
+        ++it_;
+      } while (MustAdvance());
+    }
   }
 
-  bool AtEnd() { return it_ == collection_.end(); }
-
   const std::string &Key() const { return it_->first; }
 
-  V Value(Error *error) const { return it_->second->Get(error); }
-
-
- protected:
-  typedef std::tr1::shared_ptr<AccessorInterface<V> > VAccessorPtr;
-
-  explicit PropertyConstIterator(
-      const typename std::map<std::string, VAccessorPtr> &collection)
-      : collection_(collection),
-        it_(collection_.begin()) {
-  }
-
- private:
-  friend class PropertyStore;
-
-  const typename std::map<std::string, VAccessorPtr> &collection_;
-  typename std::map<std::string, VAccessorPtr>::const_iterator it_;
-};
-
-// A version of the iterator that always advances to the next readable
-// property.
-template <class V>
-class ReadablePropertyConstIterator : public PropertyConstIterator<V> {
- public:
-  virtual ~ReadablePropertyConstIterator() {}
-
-  virtual void Advance() {
-    Error error;
-    while (!PropertyConstIterator<V>::AtEnd()) {
-      PropertyConstIterator<V>::Advance();
-      if (PropertyConstIterator<V>::AtEnd())
-        return;
-      error.Reset();
-      PropertyConstIterator<V>::Value(&error);
-      if (error.IsSuccess())
-        break;
-    }
-  }
+  const V &value() const { return value_; }
 
  private:
   friend class PropertyStore;
@@ -82,17 +42,27 @@
 
   explicit ReadablePropertyConstIterator(
       const typename std::map<std::string, VAccessorPtr> &collection)
-      : PropertyConstIterator<V>(collection) {
-    if (PropertyConstIterator<V>::AtEnd())
-      return;
-    Error error;
-    PropertyConstIterator<V>::Value(&error);
-    // Start at the next readable property (if any).
-    if (!error.IsSuccess())
+      : collection_(collection),
+        it_(collection_.begin()),
+        value_() {
+    if (MustAdvance()) {
       Advance();
+    }
   }
+
+  bool MustAdvance() { return !AtEnd() && !RetrieveCurrentValue(); }
+
+  bool RetrieveCurrentValue() {
+    Error error;
+    value_ = it_->second->Get(&error);
+    return error.IsSuccess();
+  }
+
+  const typename std::map<std::string, VAccessorPtr> &collection_;
+  typename std::map<std::string, VAccessorPtr>::const_iterator it_;
+  V value_;
 };
 
 }  // namespace shill
 
-#endif  // SHILL_PROPERTY_ITERATOR_
+#endif  // SHILL_PROPERTY_ITERATOR_H_
diff --git a/property_store_inspector.cc b/property_store_inspector.cc
index 106749a..d057d99 100644
--- a/property_store_inspector.cc
+++ b/property_store_inspector.cc
@@ -4,166 +4,90 @@
 
 #include "shill/property_store_inspector.h"
 
-#include "shill/error.h"
-
 using std::map;
 using std::string;
 using std::vector;
 
 namespace shill {
 
-PropertyStoreInspector::PropertyStoreInspector() {}
+PropertyStoreInspector::PropertyStoreInspector() : store_(NULL) {}
 
 PropertyStoreInspector::PropertyStoreInspector(const PropertyStore *store)
     : store_(store) {}
 
 PropertyStoreInspector::~PropertyStoreInspector() {}
 
-bool PropertyStoreInspector::GetBoolProperty(const string &name,
-                                             bool *value,
-                                             Error *error) {
-  return GetProperty(name, value, error, &PropertyStore::GetBoolPropertiesIter);
+bool PropertyStoreInspector::GetBoolProperty(const string &name, bool *value) {
+  return GetProperty(name, value, &PropertyStore::GetBoolPropertiesIter);
 }
 
 bool PropertyStoreInspector::GetInt16Property(const string &name,
-                                              int16 *value,
-                                              Error *error) {
-  return GetProperty(name, value, error,
-                     &PropertyStore::GetInt16PropertiesIter);
+                                              int16 *value) {
+  return GetProperty(name, value, &PropertyStore::GetInt16PropertiesIter);
 }
 
 bool PropertyStoreInspector::GetInt32Property(const string &name,
-                                              int32 *value,
-                                              Error *error) {
-  return GetProperty(name, value, error,
-                     &PropertyStore::GetInt32PropertiesIter);
+                                              int32 *value) {
+  return GetProperty(name, value, &PropertyStore::GetInt32PropertiesIter);
 }
 
 bool PropertyStoreInspector::GetKeyValueStoreProperty(const string &name,
-                                                      KeyValueStore *value,
-                                                      Error *error) {
-  return GetProperty(name, value, error,
+                                                      KeyValueStore *value) {
+  return GetProperty(name, value,
                      &PropertyStore::GetKeyValueStorePropertiesIter);
 }
 
 bool PropertyStoreInspector::GetStringProperty(const string &name,
-                                               string *value,
-                                               Error *error) {
-  return GetProperty(name, value, error,
-                     &PropertyStore::GetStringPropertiesIter);
+                                               string *value) {
+  return GetProperty(name, value, &PropertyStore::GetStringPropertiesIter);
 }
 
 bool PropertyStoreInspector::GetStringmapProperty(const string &name,
-                                                  map<string, string> *values,
-                                                  Error *error) {
-  return GetProperty(name, values, error,
-                     &PropertyStore::GetStringmapPropertiesIter);
+                                                  map<string, string> *values) {
+  return GetProperty(name, values, &PropertyStore::GetStringmapPropertiesIter);
 }
 
 bool PropertyStoreInspector::GetStringsProperty(const string &name,
-                                                vector<string> *values,
-                                                Error *error) {
-  return GetProperty(name, values, error,
-                     &PropertyStore::GetStringsPropertiesIter);
+                                                vector<string> *values) {
+  return GetProperty(name, values, &PropertyStore::GetStringsPropertiesIter);
 }
 
 bool PropertyStoreInspector::GetUint8Property(const string &name,
-                                              uint8 *value,
-                                              Error *error) {
-  return GetProperty(name, value, error,
-                     &PropertyStore::GetUint8PropertiesIter);
+                                              uint8 *value) {
+  return GetProperty(name, value, &PropertyStore::GetUint8PropertiesIter);
 }
 
 bool PropertyStoreInspector::GetUint16Property(const string &name,
-                                               uint16 *value,
-                                               Error *error) {
-  return GetProperty(name, value, error,
-                     &PropertyStore::GetUint16PropertiesIter);
+                                               uint16 *value) {
+  return GetProperty(name, value, &PropertyStore::GetUint16PropertiesIter);
 }
 
 bool PropertyStoreInspector::GetUint32Property(const string &name,
-                                               uint32 *value,
-                                               Error *error) {
-  return GetProperty(name, value, error,
-                     &PropertyStore::GetUint32PropertiesIter);
+                                               uint32 *value) {
+  return GetProperty(name, value, &PropertyStore::GetUint32PropertiesIter);
 }
 
 bool PropertyStoreInspector::GetRpcIdentifierProperty(const string &name,
-                                                      RpcIdentifier *value,
-                                                      Error *error) {
-  return GetProperty(name, value, error,
+                                                      RpcIdentifier *value) {
+  return GetProperty(name, value,
                      &PropertyStore::GetRpcIdentifierPropertiesIter);
 }
 
-bool PropertyStoreInspector::ContainsBoolProperty(const string &name) {
-  return ContainsProperty(name, &PropertyStore::GetBoolPropertiesIter);
-}
-
-bool PropertyStoreInspector::ContainsInt16Property(const string &name) {
-  return ContainsProperty(name, &PropertyStore::GetInt16PropertiesIter);
-}
-
-bool PropertyStoreInspector::ContainsInt32Property(const string &name) {
-  return ContainsProperty(name, &PropertyStore::GetInt32PropertiesIter);
-}
-
-bool PropertyStoreInspector::ContainsKeyValueStoreProperty(const string &name) {
-  return ContainsProperty(name, &PropertyStore::GetKeyValueStorePropertiesIter);
-}
-
-bool PropertyStoreInspector::ContainsStringProperty(const string &name) {
-  return ContainsProperty(name, &PropertyStore::GetStringPropertiesIter);
-}
-
-bool PropertyStoreInspector::ContainsStringmapProperty(const string &name) {
-  return ContainsProperty(name, &PropertyStore::GetStringmapPropertiesIter);
-}
-
-bool PropertyStoreInspector::ContainsStringsProperty(const string &name) {
-  return ContainsProperty(name, &PropertyStore::GetStringsPropertiesIter);
-}
-
-bool PropertyStoreInspector::ContainsUint8Property(const string &name) {
-  return ContainsProperty(name, &PropertyStore::GetUint8PropertiesIter);
-}
-
-bool PropertyStoreInspector::ContainsUint16Property(const string &name) {
-  return ContainsProperty(name, &PropertyStore::GetUint16PropertiesIter);
-}
-
-bool PropertyStoreInspector::ContainsUint32Property(const string &name) {
-  return ContainsProperty(name, &PropertyStore::GetUint32PropertiesIter);
-}
-
-bool PropertyStoreInspector::ContainsRpcIdentifierProperty(const string &name) {
-  return ContainsProperty(name, &PropertyStore::GetRpcIdentifierPropertiesIter);
-}
-
 template <class V>
 bool PropertyStoreInspector::GetProperty(
     const string &name,
     V *value,
-    Error *error,
     ReadablePropertyConstIterator<V>(PropertyStore::*getter)() const) {
-  ReadablePropertyConstIterator<V> it = ((*store_).*getter)();
-  for ( ; !it.AtEnd(); it.Advance()) {
+  for (ReadablePropertyConstIterator<V> it = ((*store_).*getter)();
+       !it.AtEnd(); it.Advance()) {
     if (it.Key() == name) {
-      *value = it.Value(error);
-      return error->IsSuccess();
+      if (value) {
+        *value = it.value();
+      }
+      return true;
     }
   }
-  error->Populate(Error::kNotFound);
   return false;
 };
 
-template <class V>
-bool PropertyStoreInspector::ContainsProperty(
-      const std::string &name,
-      ReadablePropertyConstIterator<V>(PropertyStore::*getter)() const) {
-  V value;
-  Error error;
-  GetProperty(name, &value, &error, getter);
-  return error.type() != Error::kNotFound;
-}
-
 }  // namespace shill
diff --git a/property_store_inspector.h b/property_store_inspector.h
index 12d2bcf..b29a9dc 100644
--- a/property_store_inspector.h
+++ b/property_store_inspector.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef SHILL_PROPERTY_STORE_INSPECTOR_
-#define SHILL_PROPERTY_STORE_INSPECTOR_
+#ifndef SHILL_PROPERTY_STORE_INSPECTOR_H_
+#define SHILL_PROPERTY_STORE_INSPECTOR_H_
 
 #ifndef UNIT_TEST
 #error "Do not use PropertyStoreInspector in non-test code."
@@ -30,63 +30,29 @@
   const PropertyStore *store() const { return store_; }
   void set_store(const PropertyStore *store) { store_ = store; }
 
-  // Methods to allow the getting, of properties stored in the referenced
-  // |store_| by name.  Upon success, these methods return true, return
-  // the property value in |value|, and leave |error| untouched.  Upon
-  // failure, they return false and if non-NULL, |error| is set appropriately.
-  // The value for |error| may be returned by the property getter itself, or
-  // if the property is not found in the store, these functions return
-  // Error::kNotFound.
-  bool GetBoolProperty(const std::string &name, bool *value, Error *error);
-  bool GetInt16Property(const std::string &name, int16 *value, Error *error);
-  bool GetInt32Property(const std::string &name, int32 *value, Error *error);
-  bool GetKeyValueStoreProperty(const std::string &name,
-                                KeyValueStore *value,
-                                Error *error);
-  bool GetStringProperty(const std::string &name,
-                         std::string *value,
-                         Error *error);
+  // Methods to allow the getting of properties stored in the referenced
+  // |store_| by name. Upon success, these methods return true and return the
+  // property value in |value| if non-NULL. Upon failure, they return false and
+  // leave |value| untouched.
+  bool GetBoolProperty(const std::string &name, bool *value);
+  bool GetInt16Property(const std::string &name, int16 *value);
+  bool GetInt32Property(const std::string &name, int32 *value);
+  bool GetKeyValueStoreProperty(const std::string &name, KeyValueStore *value);
+  bool GetStringProperty(const std::string &name, std::string *value);
   bool GetStringmapProperty(const std::string &name,
-                            std::map<std::string, std::string> *values,
-                            Error *error);
+                            std::map<std::string, std::string> *values);
   bool GetStringsProperty(const std::string &name,
-                          std::vector<std::string> *values,
-                          Error *error);
-  bool GetUint8Property(const std::string &name, uint8 *value, Error *error);
-  bool GetUint16Property(const std::string &name, uint16 *value, Error *error);
-  bool GetUint32Property(const std::string &name, uint32 *value, Error *error);
-  bool GetRpcIdentifierProperty(const std::string &name,
-                                RpcIdentifier *value,
-                                Error *error);
-
-  // Methods to test the whether a property is enumerable for reading -- rather
-  // specifically that trying to do so would either succeed or return
-  // an error other than Error::kNotFound.  Therefore, this function will
-  // return true for both successfully readable properties or properties that
-  // return a different error for their getter (e.g., Error:kPermissionDenied).
-  bool ContainsBoolProperty(const std::string &name);
-  bool ContainsInt16Property(const std::string &name);
-  bool ContainsInt32Property(const std::string &name);
-  bool ContainsKeyValueStoreProperty(const std::string &name);
-  bool ContainsStringProperty(const std::string &name);
-  bool ContainsStringmapProperty(const std::string &name);
-  bool ContainsStringsProperty(const std::string &name);
-  bool ContainsUint8Property(const std::string &name);
-  bool ContainsUint16Property(const std::string &name);
-  bool ContainsUint32Property(const std::string &name);
-  bool ContainsRpcIdentifierProperty(const std::string &name);
+                          std::vector<std::string> *values);
+  bool GetUint8Property(const std::string &name, uint8 *value);
+  bool GetUint16Property(const std::string &name, uint16 *value);
+  bool GetUint32Property(const std::string &name, uint32 *value);
+  bool GetRpcIdentifierProperty(const std::string &name, RpcIdentifier *value);
 
  private:
   template <class V>
   bool GetProperty(
       const std::string &name,
       V *value,
-      Error *error,
-      ReadablePropertyConstIterator<V>(PropertyStore::*getter)() const);
-
-  template <class V>
-  bool ContainsProperty(
-      const std::string &name,
       ReadablePropertyConstIterator<V>(PropertyStore::*getter)() const);
 
   const PropertyStore *store_;
@@ -97,4 +63,4 @@
 }  // namespace shill
 
 
-#endif  // SHILL_PROPERTY_STORE_INSPECTOR_
+#endif  // SHILL_PROPERTY_STORE_INSPECTOR_H_
diff --git a/property_store_unittest.cc b/property_store_unittest.cc
index f69107a..15039fc 100644
--- a/property_store_unittest.cc
+++ b/property_store_unittest.cc
@@ -230,7 +230,6 @@
   // Test that properties registered as write-only are not returned
   // when using Get*PropertiesIter().
   PropertyStore store;
-  Error error;
   {
     const string keys[]  = {"boolp1", "boolp2"};
     bool values[] = {true, true};
@@ -238,11 +237,9 @@
     store.RegisterBool(keys[1], &values[1]);
 
     ReadablePropertyConstIterator<bool> it = store.GetBoolPropertiesIter();
-    error.Reset();
     EXPECT_FALSE(it.AtEnd());
     EXPECT_EQ(keys[1], it.Key());
-    EXPECT_TRUE(values[1] == it.Value(&error));
-    EXPECT_TRUE(error.IsSuccess());
+    EXPECT_TRUE(values[1] == it.value());
     it.Advance();
     EXPECT_TRUE(it.AtEnd());
   }
@@ -253,11 +250,9 @@
     store.RegisterInt16(keys[1], &values[1]);
 
     ReadablePropertyConstIterator<int16> it = store.GetInt16PropertiesIter();
-    error.Reset();
     EXPECT_FALSE(it.AtEnd());
     EXPECT_EQ(keys[1], it.Key());
-    EXPECT_EQ(values[1], it.Value(&error));
-    EXPECT_TRUE(error.IsSuccess());
+    EXPECT_EQ(values[1], it.value());
     it.Advance();
     EXPECT_TRUE(it.AtEnd());
   }
@@ -268,11 +263,9 @@
     store.RegisterInt32(keys[1], &values[1]);
 
     ReadablePropertyConstIterator<int32> it = store.GetInt32PropertiesIter();
-    error.Reset();
     EXPECT_FALSE(it.AtEnd());
     EXPECT_EQ(keys[1], it.Key());
-    EXPECT_EQ(values[1], it.Value(&error));
-    EXPECT_TRUE(error.IsSuccess());
+    EXPECT_EQ(values[1], it.value());
     it.Advance();
     EXPECT_TRUE(it.AtEnd());
   }
@@ -283,11 +276,9 @@
     store.RegisterString(keys[1], &values[1]);
 
     ReadablePropertyConstIterator<string> it = store.GetStringPropertiesIter();
-    error.Reset();
     EXPECT_FALSE(it.AtEnd());
     EXPECT_EQ(keys[1], it.Key());
-    EXPECT_EQ(values[1], it.Value(&error));
-    EXPECT_TRUE(error.IsSuccess());
+    EXPECT_EQ(values[1], it.value());
     it.Advance();
     EXPECT_TRUE(it.AtEnd());
   }
@@ -301,11 +292,9 @@
 
     ReadablePropertyConstIterator<Stringmap> it =
         store.GetStringmapPropertiesIter();
-    error.Reset();
     EXPECT_FALSE(it.AtEnd());
     EXPECT_EQ(keys[1], it.Key());
-    EXPECT_TRUE(values[1] == it.Value(&error));
-    EXPECT_TRUE(error.IsSuccess());
+    EXPECT_TRUE(values[1] == it.value());
     it.Advance();
     EXPECT_TRUE(it.AtEnd());
   }
@@ -323,11 +312,9 @@
 
     ReadablePropertyConstIterator<Stringmaps> it =
         store.GetStringmapsPropertiesIter();
-    error.Reset();
     EXPECT_FALSE(it.AtEnd());
     EXPECT_EQ(keys[1], it.Key());
-    EXPECT_TRUE(values[1] == it.Value(&error));
-    EXPECT_TRUE(error.IsSuccess());
+    EXPECT_TRUE(values[1] == it.value());
     it.Advance();
     EXPECT_TRUE(it.AtEnd());
   }
@@ -344,11 +331,9 @@
 
     ReadablePropertyConstIterator<Strings> it =
         store.GetStringsPropertiesIter();
-    error.Reset();
     EXPECT_FALSE(it.AtEnd());
     EXPECT_EQ(keys[1], it.Key());
-    EXPECT_TRUE(values[1] == it.Value(&error));
-    EXPECT_TRUE(error.IsSuccess());
+    EXPECT_TRUE(values[1] == it.value());
     it.Advance();
     EXPECT_TRUE(it.AtEnd());
   }
@@ -359,11 +344,9 @@
     store.RegisterUint8(keys[1], &values[1]);
 
     ReadablePropertyConstIterator<uint8> it = store.GetUint8PropertiesIter();
-    error.Reset();
     EXPECT_FALSE(it.AtEnd());
     EXPECT_EQ(keys[1], it.Key());
-    EXPECT_EQ(values[1], it.Value(&error));
-    EXPECT_TRUE(error.IsSuccess());
+    EXPECT_EQ(values[1], it.value());
     it.Advance();
     EXPECT_TRUE(it.AtEnd());
   }
@@ -374,11 +357,9 @@
     store.RegisterUint16(keys[1], &values[1]);
 
     ReadablePropertyConstIterator<uint16> it = store.GetUint16PropertiesIter();
-    error.Reset();
     EXPECT_FALSE(it.AtEnd());
     EXPECT_EQ(keys[1], it.Key());
-    EXPECT_EQ(values[1], it.Value(&error));
-    EXPECT_TRUE(error.IsSuccess());
+    EXPECT_EQ(values[1], it.value());
     it.Advance();
     EXPECT_TRUE(it.AtEnd());
   }
diff --git a/service_unittest.cc b/service_unittest.cc
index e7fee5b..2f17f12 100644
--- a/service_unittest.cc
+++ b/service_unittest.cc
@@ -559,7 +559,7 @@
 TEST_P(ReadOnlyServicePropertyTest, PropertyWriteOnly) {
   string property(GetParam().reader().get_string());
   PropertyStoreInspector inspector(&service_->store());
-  EXPECT_FALSE(inspector.ContainsStringProperty(property));
+  EXPECT_FALSE(inspector.GetStringProperty(property, NULL));
 }
 
 INSTANTIATE_TEST_CASE_P(
diff --git a/static_ip_parameters_unittest.cc b/static_ip_parameters_unittest.cc
index afc7f74..73397c7 100644
--- a/static_ip_parameters_unittest.cc
+++ b/static_ip_parameters_unittest.cc
@@ -119,21 +119,20 @@
   EXPECT_EQ(kTestMtu, props.mtu);
 
   PropertyStoreInspector inspector(&store);
-  EXPECT_FALSE(inspector.ContainsStringProperty("StaticIP.Address"));
+  EXPECT_FALSE(inspector.GetStringProperty("StaticIP.Address", NULL));
   string string_value;
-  EXPECT_TRUE(inspector.GetStringProperty("StaticIP.Gateway",
-                                          &string_value, &error));
+  EXPECT_TRUE(inspector.GetStringProperty("StaticIP.Gateway", &string_value));
   EXPECT_EQ(kGateway, string_value);
-  EXPECT_FALSE(inspector.ContainsInt32Property("StaticIP.Mtu"));
+  EXPECT_FALSE(inspector.GetInt32Property("StaticIP.Mtu", NULL));
   EXPECT_TRUE(inspector.GetStringProperty("StaticIP.NameServers",
-                                          &string_value, &error));
+                                          &string_value));
   EXPECT_EQ(kNameServers, string_value);
   EXPECT_TRUE(inspector.GetStringProperty("StaticIP.PeerAddress",
-                                          &string_value, &error));
+                                          &string_value));
   EXPECT_EQ(kPeerAddress, string_value);
   int32 int_value;
   EXPECT_TRUE(inspector.GetInt32Property("StaticIP.Prefixlen",
-                                         &int_value, &error));
+                                         &int_value));
   EXPECT_EQ(kPrefixLen, int_value);
 }
 
diff --git a/vpn_driver_unittest.cc b/vpn_driver_unittest.cc
index e0ad631..e37ddbb 100644
--- a/vpn_driver_unittest.cc
+++ b/vpn_driver_unittest.cc
@@ -112,11 +112,10 @@
 bool VPNDriverTest::GetProviderProperty(const PropertyStore &store,
                                         const string &key,
                                         string *value) {
-  Error error;
   PropertyStoreInspector inspector(&store);
   KeyValueStore provider_properties;
   EXPECT_TRUE(inspector.GetKeyValueStoreProperty(
-      flimflam::kProviderProperty, &provider_properties, &error));
+      flimflam::kProviderProperty, &provider_properties));
   if (!provider_properties.ContainsString(key)) {
     return false;
   }
@@ -192,7 +191,7 @@
   PropertyStoreInspector inspector(&store);
 
   // An un-set property should not be readable.
-  EXPECT_FALSE(inspector.ContainsStringProperty(kPortProperty));
+  EXPECT_FALSE(inspector.GetStringProperty(kPortProperty, NULL));
   EXPECT_FALSE(GetProviderProperty(store, kPortProperty, NULL));
 
   const string kProviderName = "boo";
@@ -202,7 +201,7 @@
 
   // We should not be able to read a property out of the driver args using the
   // key to the args directly.
-  EXPECT_FALSE(inspector.ContainsStringProperty(kPortProperty));
+  EXPECT_FALSE(inspector.GetStringProperty(kPortProperty, NULL));
 
   // We should instead be able to find it within the "Provider" stringmap.
   string value;