Extending ONC validator's logging. Completing toplevel validation.

chromeos/network/onc/onc_mapper.cc

Index: chromeos/network/onc/onc_mapper.cc
diff --git a/chromeos/network/onc/onc_mapper.cc b/chromeos/network/onc/onc_mapper.cc
index 8d5d723c4cfa56d81da275c5e57cd6af5ebff969..768f0f6e07f2e209dc2f63851989c32518eb1e9c 100644
--- a/chromeos/network/onc/onc_mapper.cc
+++ b/chromeos/network/onc/onc_mapper.cc
@@ -17,28 +17,25 @@ Mapper::Mapper() {
 Mapper::~Mapper() {
 }
 
-scoped_ptr<base::Value> Mapper::MapValue(
-    const OncValueSignature& signature,
-    const base::Value& onc_value) {
+scoped_ptr<base::Value> Mapper::MapValue(const OncValueSignature& signature,
+                                         const base::Value& onc_value,
+                                         bool* error) {
   scoped_ptr<base::Value> result_value;
   switch (onc_value.GetType()) {
     case base::Value::TYPE_DICTIONARY: {
       const base::DictionaryValue* dict = NULL;
       onc_value.GetAsDictionary(&dict);
-      result_value = MapObject(signature, *dict);
+      result_value = MapObject(signature, *dict, error);
       break;
     }
     case base::Value::TYPE_LIST: {
       const base::ListValue* list = NULL;
       onc_value.GetAsList(&list);
-      bool nested_error_occured = false;
-      result_value = MapArray(signature, *list, &nested_error_occured);
-      if (nested_error_occured)
-        result_value.reset();
+      result_value = MapArray(signature, *list, error);
       break;
     }
     default: {
-      result_value = MapPrimitive(signature, onc_value);
+      result_value = MapPrimitive(signature, onc_value, error);
       break;
     }
   }
@@ -48,44 +45,44 @@ scoped_ptr<base::Value> Mapper::MapValue(
 
 scoped_ptr<base::DictionaryValue> Mapper::MapObject(
     const OncValueSignature& signature,
-    const base::DictionaryValue& onc_object) {
+    const base::DictionaryValue& onc_object,
+    bool* error) {
   scoped_ptr<base::DictionaryValue> result(new base::DictionaryValue);
 
   bool found_unknown_field = false;
-  bool nested_error_occured = false;
-  MapFields(signature, onc_object, &found_unknown_field, &nested_error_occured,
-            result.get());
-  if (!nested_error_occured && !found_unknown_field)
-    return result.Pass();
-  else
-    return scoped_ptr<base::DictionaryValue>();
+  MapFields(signature, onc_object, &found_unknown_field, error, result.get());
+  if (found_unknown_field)
+    *error = true;
+  return result.Pass();
 }
 
-scoped_ptr<base::Value> Mapper::MapPrimitive(
-    const OncValueSignature& signature,
-    const base::Value& onc_primitive) {
+scoped_ptr<base::Value> Mapper::MapPrimitive(const OncValueSignature& signature,
+                                             const base::Value& onc_primitive,
+                                             bool* error) {
   return make_scoped_ptr(onc_primitive.DeepCopy());
 }
 
-void Mapper::MapFields(
-    const OncValueSignature& object_signature,
-    const base::DictionaryValue& onc_object,
-    bool* found_unknown_field,
-    bool* nested_error_occured,
-    base::DictionaryValue* result) {
+void Mapper::MapFields(const OncValueSignature& object_signature,
+                       const base::DictionaryValue& onc_object,
+                       bool* found_unknown_field,
+                       bool* nested_error,
+                       base::DictionaryValue* result) {
 
   for (base::DictionaryValue::Iterator it(onc_object); it.HasNext();
        it.Advance()) {
     bool current_field_unknown = false;
-    scoped_ptr<base::Value> result_value = MapField(
-        it.key(), object_signature, it.value(), &current_field_unknown);
+    scoped_ptr<base::Value> result_value = MapField(it.key(),
+                                                    object_signature,
+                                                    it.value(),
+                                                    &current_field_unknown,
+                                                    nested_error);
 
     if (current_field_unknown)
       *found_unknown_field = true;
     else if (result_value.get() != NULL)
       result->SetWithoutPathExpansion(it.key(), result_value.release());
     else
-      *nested_error_occured = true;
+      DCHECK(*nested_error);
   }
 }
 
@@ -93,18 +90,16 @@ scoped_ptr<base::Value> Mapper::MapField(
     const std::string& field_name,
     const OncValueSignature& object_signature,
     const base::Value& onc_value,
-    bool* found_unknown_field) {
+    bool* found_unknown_field,
+    bool* error) {
   const OncFieldSignature* field_signature =
       GetFieldSignature(object_signature, field_name);
 
   if (field_signature != NULL) {
-    if (field_signature->value_signature == NULL) {
-      NOTREACHED() << "Found missing value signature at field '"
-                   << field_name << "'.";
-      return scoped_ptr<base::Value>();
-    }
+    DCHECK(field_signature->value_signature != NULL)

[pastarmovj, 2012/12/13 10:15:44]

Leave just DCHECK(pointer) it's equivalent.

[pneubeck (no reviews), 2012/12/13 14:10:03]

For consistency with the other null-pointer checks, I would better like to keep
it that way.

+        << "Found missing value signature at field '" << field_name << "'.";
 
-    return MapValue(*field_signature->value_signature, onc_value);
+    return MapValue(*field_signature->value_signature, onc_value, error);
   } else {
     DVLOG(1) << "Found unknown field name: '" << field_name << "'";
     *found_unknown_field = true;
@@ -115,26 +110,35 @@ scoped_ptr<base::Value> Mapper::MapField(
 scoped_ptr<base::ListValue> Mapper::MapArray(
     const OncValueSignature& array_signature,
     const base::ListValue& onc_array,
-    bool* nested_error_occured) {
-  if (array_signature.onc_array_entry_signature == NULL) {
-    NOTREACHED() << "Found missing onc_array_entry_signature.";
-    return scoped_ptr<base::ListValue>();
-  }
+    bool* nested_error) {
+  DCHECK(array_signature.onc_array_entry_signature != NULL)

[pastarmovj, 2012/12/13 10:15:44]

Ditto.

[pneubeck (no reviews), 2012/12/13 14:10:03]

see above.

+      << "Found missing onc_array_entry_signature.";
 
   scoped_ptr<base::ListValue> result_array(new base::ListValue);
+  int original_index = 0;
   for (base::ListValue::const_iterator it = onc_array.begin();
-       it != onc_array.end(); ++it) {
+       it != onc_array.end(); ++it, ++original_index) {
     const base::Value* entry = *it;
 
     scoped_ptr<base::Value> result_entry;
-    result_entry = MapValue(*array_signature.onc_array_entry_signature, *entry);
+    result_entry = MapEntry(original_index,
+                            *array_signature.onc_array_entry_signature,
+                            *entry,
+                            nested_error);
     if (result_entry.get() != NULL)
       result_array->Append(result_entry.release());
     else
-      *nested_error_occured = true;
+      DCHECK(*nested_error);
   }
   return result_array.Pass();
 }
 
+scoped_ptr<base::Value> Mapper::MapEntry(int index,
+                                         const OncValueSignature& signature,
+                                         const base::Value& onc_value,
+                                         bool* error) {
+  return MapValue(signature, onc_value, error);
+}
+
 }  // namespace onc
 }  // namespace chromeos