Regkey functions return error code instead of bool

chrome/installer/util/google_update_settings.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/installer/util/google_update_settings.h"
 
 #include <algorithm>
 
 #include "base/command_line.h"
 #include "base/string_number_conversions.h"
@@ skipping 22 matching lines @@
   FOUND_SAME_SETTING
 };
 
 bool ReadGoogleUpdateStrKey(const wchar_t* const name, std::wstring* value) {
   // The registry functions below will end up going to disk.  Do this on another
   // thread to avoid slowing the IO thread.  http://crbug.com/62121
   base::ThreadRestrictions::ScopedAllowIO allow_io;
   BrowserDistribution* dist = BrowserDistribution::GetDistribution();
   std::wstring reg_path = dist->GetStateKey();
   RegKey key(HKEY_CURRENT_USER, reg_path.c_str(), KEY_READ);
-  if (!key.ReadValue(name, value)) {
+  if (key.ReadValue(name, value) != ERROR_SUCCESS) {
     RegKey hklm_key(HKEY_LOCAL_MACHINE, reg_path.c_str(), KEY_READ);
-    return hklm_key.ReadValue(name, value);
+    return (hklm_key.ReadValue(name, value) == ERROR_SUCCESS);
   }
   return true;
 }
 
 bool WriteGoogleUpdateStrKey(const wchar_t* const name,
                              const std::wstring& value) {
   BrowserDistribution* dist = BrowserDistribution::GetDistribution();
   std::wstring reg_path = dist->GetStateKey();
   RegKey key(HKEY_CURRENT_USER, reg_path.c_str(), KEY_READ | KEY_WRITE);
-  return key.WriteValue(name, value.c_str());
+  return key.WriteValue(name, value.c_str()) == ERROR_SUCCESS;
 }
 
 bool ClearGoogleUpdateStrKey(const wchar_t* const name) {
   BrowserDistribution* dist = BrowserDistribution::GetDistribution();
   std::wstring reg_path = dist->GetStateKey();
   RegKey key(HKEY_CURRENT_USER, reg_path.c_str(), KEY_READ | KEY_WRITE);
   std::wstring value;
-  if (!key.ReadValue(name, &value))
+  if (key.ReadValue(name, &value) != ERROR_SUCCESS)
     return false;
-  return key.WriteValue(name, L"");
+  return key.WriteValue(name, L"") == ERROR_SUCCESS;
 }
 
 bool RemoveGoogleUpdateStrKey(const wchar_t* const name) {
   BrowserDistribution* dist = BrowserDistribution::GetDistribution();
   std::wstring reg_path = dist->GetStateKey();
   RegKey key(HKEY_CURRENT_USER, reg_path.c_str(), KEY_READ | KEY_WRITE);
   if (!key.ValueExists(name))
     return true;
-  return key.DeleteValue(name);
+  return key.DeleteValue(name) == ERROR_SUCCESS;
 }
 
 EulaSearchResult HasEULASetting(HKEY root, const std::wstring& state_key,
                                 bool setting) {
   RegKey key;
-  DWORD previous_value;
+  DWORD previous_value = setting ? 1 : 0;
 
-  if (!key.Open(root, state_key.c_str(), KEY_QUERY_VALUE))
+  if (key.Open(root, state_key.c_str(), KEY_QUERY_VALUE) != ERROR_SUCCESS)
     return NO_SETTING;
 
-  if (!key.ReadValueDW(google_update::kRegEULAAceptedField, &previous_value))
+  LONG result = key.ReadValueDW(google_update::kRegEULAAceptedField,
+                                &previous_value);
+  if (result != ERROR_SUCCESS)
     return FOUND_CLIENT_STATE;
 
   return ((previous_value != 0) == setting) ?
       FOUND_SAME_SETTING : FOUND_OPPOSITE_SETTING;
 }
 
 }  // namespace.
 
 bool GoogleUpdateSettings::GetCollectStatsConsent() {
   BrowserDistribution* dist = BrowserDistribution::GetDistribution();
   std::wstring reg_path = dist->GetStateKey();
   RegKey key(HKEY_CURRENT_USER, reg_path.c_str(), KEY_READ);
-  DWORD value;
-  if (!key.ReadValueDW(google_update::kRegUsageStatsField, &value)) {
+  DWORD value = 0;
+  LONG result = key.ReadValueDW(google_update::kRegUsageStatsField, &value);
+  if (result != ERROR_SUCCESS) {
     RegKey hklm_key(HKEY_LOCAL_MACHINE, reg_path.c_str(), KEY_READ);

[grt (UTC plus 2), 2011/01/11 03:51:30]

how about use key.Open() rather than making a new instance?

[amit, 2011/01/12 04:11:23]

Done.

-    if (!hklm_key.ReadValueDW(google_update::kRegUsageStatsField, &value))
-      return false;
+    hklm_key.ReadValueDW(google_update::kRegUsageStatsField, &value);

[grt (UTC plus 2), 2011/01/11 03:51:30]

I prefer checking the return value here rather than relying on "value" remaining
0.

   }
   return (1 == value);
 }
 
 bool GoogleUpdateSettings::SetCollectStatsConsent(bool consented) {
   DWORD value = consented? 1 : 0;
   // Writing to HKLM is only a best effort deal.
   BrowserDistribution* dist = BrowserDistribution::GetDistribution();
   std::wstring reg_path = dist->GetStateMediumKey();
   RegKey key_hklm(HKEY_LOCAL_MACHINE, reg_path.c_str(), KEY_READ | KEY_WRITE);
   key_hklm.WriteValue(google_update::kRegUsageStatsField, value);
   // Writing to HKCU is used both by chrome and by the crash reporter.
   reg_path = dist->GetStateKey();
   RegKey key_hkcu(HKEY_CURRENT_USER, reg_path.c_str(), KEY_READ | KEY_WRITE);

[grt (UTC plus 2), 2011/01/11 03:51:30]

Change to use only one RegKey instance?

[amit, 2011/01/12 04:11:23]

Done.

-  return key_hkcu.WriteValue(google_update::kRegUsageStatsField, value);
+  LONG result = key_hkcu.WriteValue(google_update::kRegUsageStatsField, value);
+  return result == ERROR_SUCCESS;
 }
 
 bool GoogleUpdateSettings::GetMetricsId(std::wstring* metrics_id) {
   return ReadGoogleUpdateStrKey(google_update::kRegMetricsId, metrics_id);
 }
 
 bool GoogleUpdateSettings::SetMetricsId(const std::wstring& metrics_id) {
   return WriteGoogleUpdateStrKey(google_update::kRegMetricsId, metrics_id);
 }
 
@@ skipping 21 matching lines @@
         reg_path = product.distribution()->GetStateMediumKey();
       }
     }
     if (status == NO_SETTING) {
       LOG(WARNING)
           << "eulaaccepted value not found; setting consent on package";
       reg_path = package.properties()->GetStateMediumKey();
     }
   }
   RegKey key(HKEY_LOCAL_MACHINE, reg_path.c_str(), KEY_SET_VALUE);
-  return key.WriteValue(google_update::kRegEULAAceptedField, consented ? 1 : 0);
+  LONG result = key.WriteValue(google_update::kRegEULAAceptedField,
+                               consented ? 1 : 0);
+  return result == ERROR_SUCCESS;
 }
 
 int GoogleUpdateSettings::GetLastRunTime() {
   std::wstring time_s;
   if (!ReadGoogleUpdateStrKey(google_update::kRegLastRunTimeField, &time_s))
     return -1;
   int64 time_i;
   if (!base::StringToInt64(time_s, &time_i))
     return -1;
   base::TimeDelta td =
@@ skipping 70 matching lines @@
 void GoogleUpdateSettings::UpdateDiffInstallStatus(bool system_install,
     bool incremental_install, int install_return_code,
     const std::wstring& product_guid) {
   HKEY reg_root = (system_install) ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER;
 
   RegKey key;
   installer::ChannelInfo channel_info;
   std::wstring reg_key(google_update::kRegPathClientState);
   reg_key.append(L"\\");
   reg_key.append(product_guid);
-  if (!key.Open(reg_root, reg_key.c_str(), KEY_ALL_ACCESS) ||
-      !channel_info.Initialize(key)) {
+  LONG result = key.Open(reg_root, reg_key.c_str(), KEY_ALL_ACCESS);
+  if (result != ERROR_SUCCESS || !channel_info.Initialize(key)) {
     VLOG(1) << "Application key not found.";
     if (!incremental_install || !install_return_code) {
       VLOG(1) << "Returning without changing application key.";
       key.Close();

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

Remove

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

Done.

       return;
     } else if (!key.Valid()) {
       reg_key.assign(google_update::kRegPathClientState);
-      if (!key.Open(reg_root, reg_key.c_str(), KEY_ALL_ACCESS) ||
-          !key.CreateKey(product_guid.c_str(), KEY_ALL_ACCESS)) {
+      result = key.Open(reg_root, reg_key.c_str(), KEY_ALL_ACCESS);
+      if (result != ERROR_SUCCESS)

[grt (UTC plus 2), 2011/01/11 03:51:30]

result == ERROR_SUCCESS

[amit, 2011/01/12 04:11:23]

Are you sure? As per the old code, key.CreateKey will execute only if key.Open
fails.

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

I'm pretty sure CreateKey executes only if Open succeeds (unless the demons are
talking to me again).  My read of the old code is "if we can't Open the
ClientState key or if we can't create the product GUID key beneath it, log and
bail."

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

Done.

+        key.CreateKey(product_guid.c_str(), KEY_ALL_ACCESS);

[grt (UTC plus 2), 2011/01/11 03:51:30]

result = key.CreateKey(...)

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

Ping.

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

Done.

+
+      if (result != ERROR_SUCCESS) {
         LOG(ERROR) << "Failed to create application key.";
         key.Close();

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

Remove

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

Done.

         return;
       }
     }
   }
 
   if (UpdateGoogleUpdateApKey(incremental_install, install_return_code,
                               &channel_info) &&
       !channel_info.Write(&key)) {
     LOG(ERROR) << "Failed to write value " << channel_info.value()
                << " to the registry field " << google_update::kRegApField;
@@ skipping 65 matching lines @@
   };
   const wchar_t** end = &kBrands[arraysize(kBrands)];
   const wchar_t** found = std::find(&kBrands[0], end, brand);
   if (found != end)
     return true;
   if (StartsWith(brand, L"EUB", true) || StartsWith(brand, L"EUC", true) ||
       StartsWith(brand, L"GGR", true))
     return true;
   return false;
 }