Regkey functions return error code instead of bool

chrome/browser/policy/configuration_policy_provider_delegate_win.cc

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/policy/configuration_policy_provider_delegate_win.h"
 
 #include "base/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "base/win/registry.h"
 #include "chrome/common/policy_constants.h"
 
 using base::win::RegKey;
 
 namespace {
 
 bool ReadRegistryStringValue(RegKey* key, const string16& name,
                              string16* result) {
   DWORD value_size = 0;
   DWORD key_type = 0;
   scoped_array<uint8> buffer;
 
-  if (!key->ReadValue(name.c_str(), 0, &value_size, &key_type))
+  if (key->ReadValue(name.c_str(), 0, &value_size, &key_type) != ERROR_SUCCESS)
     return false;
   if (key_type != REG_SZ)
     return false;
 
   // According to the Microsoft documentation, the string
   // buffer may not be explicitly 0-terminated. Allocate a
   // slightly larger buffer and pre-fill to zeros to guarantee
   // the 0-termination.
   buffer.reset(new uint8[value_size + 2]);
   memset(buffer.get(), 0, value_size + 2);
@@ skipping 49 matching lines @@
       }
       default:
         NOTREACHED();
     }
   }
   return result;
 }
 
 bool ConfigurationPolicyProviderDelegateWin::GetRegistryPolicyString(
     const string16& name, string16* result) const {
-  string16 path = string16(kRegistrySubKey);
-  RegKey policy_key;
+  RegKey policy_key(HKEY_LOCAL_MACHINE, kRegistrySubKey, KEY_READ);
   // First try the global policy.
-  if (policy_key.Open(HKEY_LOCAL_MACHINE, path.c_str(), KEY_READ)) {
-    if (ReadRegistryStringValue(&policy_key, name, result))
-      return true;
-    policy_key.Close();
-  }
+  if (ReadRegistryStringValue(&policy_key, name, result))
+    return true;
+
   // Fall back on user-specific policy.
-  if (!policy_key.Open(HKEY_CURRENT_USER, path.c_str(), KEY_READ))
+  if (policy_key.Open(HKEY_CURRENT_USER, kRegistrySubKey,
+                      KEY_READ) != ERROR_SUCCESS)
     return false;
   return ReadRegistryStringValue(&policy_key, name, result);
 }
 
 bool ConfigurationPolicyProviderDelegateWin::GetRegistryPolicyStringList(
     const string16& key, ListValue* result) const {
   string16 path = string16(kRegistrySubKey);
   path += ASCIIToUTF16("\\") + key;
   RegKey policy_key;
-  if (!policy_key.Open(HKEY_LOCAL_MACHINE, path.c_str(), KEY_READ)) {
-    policy_key.Close();
+  LONG ret = policy_key.Open(HKEY_LOCAL_MACHINE, path.c_str(), KEY_READ);
+  if (ret != ERROR_SUCCESS) {
     // Fall back on user-specific policy.
-    if (!policy_key.Open(HKEY_CURRENT_USER, path.c_str(), KEY_READ))
+    ret = policy_key.Open(HKEY_CURRENT_USER, path.c_str(), KEY_READ);
+    if (ret != ERROR_SUCCESS)
       return false;
   }
   string16 policy_string;
   int index = 0;
   while (ReadRegistryStringValue(&policy_key, base::IntToString16(++index),
                                  &policy_string)) {
     result->Append(Value::CreateStringValue(policy_string));
   }
   return true;
 }
 
 bool ConfigurationPolicyProviderDelegateWin::GetRegistryPolicyBoolean(
     const string16& value_name, bool* result) const {
-  DWORD value;
-  RegKey hkcu_policy_key(HKEY_LOCAL_MACHINE, kRegistrySubKey, KEY_READ);
-  if (hkcu_policy_key.ReadValueDW(value_name.c_str(), &value)) {
-    *result = value != 0;
-    return true;
-  }
-
-  RegKey hklm_policy_key(HKEY_CURRENT_USER, kRegistrySubKey, KEY_READ);
-  if (hklm_policy_key.ReadValueDW(value_name.c_str(), &value)) {
-    *result = value != 0;
-    return true;
-  }
-  return false;
+  uint32 local_result = 0;
+  bool ret = GetRegistryPolicyInteger(value_name, &local_result);
+  *result = local_result != 0;

[grt (UTC plus 2), 2011/01/12 15:06:38]

Same comment as before: only modify *result when ret != false.  (Consistent with
your "I think it's not proper for an API to return failure but cause side
effects in the return arg." statement, which I'm down with.)

[amit, 2011/01/14 05:23:33]

Good catch, done.

+  return ret;
 }
 
 bool ConfigurationPolicyProviderDelegateWin::GetRegistryPolicyInteger(
     const string16& value_name, uint32* result) const {
-  DWORD value;
-  RegKey hkcu_policy_key(HKEY_LOCAL_MACHINE, kRegistrySubKey, KEY_READ);
-  if (hkcu_policy_key.ReadValueDW(value_name.c_str(), &value)) {
+  DWORD value = 0;
+  RegKey policy_key(HKEY_LOCAL_MACHINE, kRegistrySubKey, KEY_READ);
+  if (policy_key.ReadValueDW(value_name.c_str(), &value) == ERROR_SUCCESS) {
     *result = value;
     return true;
   }
 
-  RegKey hklm_policy_key(HKEY_CURRENT_USER, kRegistrySubKey, KEY_READ);
-  if (hklm_policy_key.ReadValueDW(value_name.c_str(), &value)) {
-    *result = value;
-    return true;
+  LONG ret = policy_key.Open(HKEY_CURRENT_USER, kRegistrySubKey, KEY_READ);
+  if (ret == ERROR_SUCCESS) {
+    if (policy_key.ReadValueDW(value_name.c_str(), &value) == ERROR_SUCCESS) {
+      *result = value;
+      return true;
+    }
   }
   return false;
 }
 
 }  // namespace policy