Regkey functions return error code instead of bool

chrome/installer/util/delete_reg_value_work_item.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/delete_reg_value_work_item.h"
 
 #include "base/logging.h"
 #include "base/win/registry.h"
 #include "chrome/installer/util/logging_installer.h"
 
@@ skipping 16 matching lines @@
 
 bool DeleteRegValueWorkItem::Do() {
   if (status_ != DELETE_VALUE) {
     // we already did something.
     LOG(ERROR) << "multiple calls to Do()";
     return false;
   }
 
   status_ = VALUE_UNCHANGED;
 
-  // A big flaw in the RegKey implementation is that all error information
-  // (besides success/failure) is lost in the translation from LSTATUS to bool.
-  // So, we resort to direct API calls here. :-/
-  HKEY raw_key = NULL;
-  LSTATUS err = RegOpenKeyEx(predefined_root_, key_path_.c_str(), 0,
-                             KEY_READ, &raw_key);
-  if (err != ERROR_SUCCESS) {
-    if (err == ERROR_FILE_NOT_FOUND) {
-      LOG(INFO) << "(delete value) can not open " << key_path_;
-      status_ = VALUE_NOT_FOUND;
-      return true;
-    }
-  } else {
-    ::RegCloseKey(raw_key);
+  RegKey key;
+  LONG err = key.Open(predefined_root_, key_path_.c_str(), KEY_READ);
+  if (err == ERROR_FILE_NOT_FOUND) {
+    LOG(INFO) << "(delete value) can not open " << key_path_;
+    status_ = VALUE_NOT_FOUND;
+    return true;
   }
 
-  RegKey key;
   bool result = false;
-  if (!key.Open(predefined_root_, key_path_.c_str(), KEY_READ | KEY_WRITE)) {
-    LOG(ERROR) << "can not open " << key_path_;
-  } else if (!key.ValueExists(value_name_.c_str())) {
+  if (!key.ValueExists(value_name_.c_str())) {

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

It's less work and less code to remove this call to ValueExists and hande
ERROR_FILE_NOT_FOUND from the later calls to ReadValue*, no?

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

Ping.

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

Sorry, missed it in the first iteration. Now made it even better :)

     status_ = VALUE_NOT_FOUND;
     result = true;
   // Read previous value for rollback and delete
-  } else if (((is_str_type_ && key.ReadValue(value_name_.c_str(),
-                                             &old_str_)) ||
-              (!is_str_type_ && key.ReadValueDW(value_name_.c_str(),
-                                                &old_dw_))) &&
-             (key.DeleteValue(value_name_.c_str()))) {
-    status_ = VALUE_DELETED;
-    result = true;
   } else {
-    LOG(ERROR) << "failed to read/delete value " << value_name_;
+    if (is_str_type_)
+      err = key.ReadValue(value_name_.c_str(), &old_str_);
+    else
+      err = key.ReadValueDW(value_name_.c_str(), &old_dw_);
+
+    if (err == ERROR_SUCCESS)
+      err = key.DeleteValue(value_name_.c_str());
+
+    if (err == ERROR_SUCCESS) {
+      status_ = VALUE_DELETED;
+      result = true;
+    } else {
+      LOG(ERROR) << "failed to read/delete value " << value_name_
+                 << " error: " << err;
+    }
   }
 
   return result;
 }
 
 void DeleteRegValueWorkItem::Rollback() {
   if (status_ == DELETE_VALUE || status_ == VALUE_ROLLED_BACK)
     return;
   if (status_ == VALUE_UNCHANGED || status_ == VALUE_NOT_FOUND) {
     status_ = VALUE_ROLLED_BACK;
     VLOG(1) << "rollback: setting unchanged, nothing to do";
     return;
   }
 
   // At this point only possible state is VALUE_DELETED.
   RegKey key;
-  if (!key.Open(predefined_root_, key_path_.c_str(), KEY_READ | KEY_WRITE)) {
-    LOG(ERROR) << "rollback: can not open " << key_path_;
+  LONG result = key.Open(predefined_root_, key_path_.c_str(),
+                         KEY_READ | KEY_WRITE);
+  if (result == ERROR_SUCCESS) {
   // try to restore the previous value
-  } else if ((is_str_type_ && key.WriteValue(value_name_.c_str(),
-                                             old_str_.c_str())) ||
-             (!is_str_type_ && key.WriteValue(value_name_.c_str(),
-                                              old_dw_))) {
+    if (is_str_type_) {
+      result = key.WriteValue(value_name_.c_str(), old_str_.c_str());
+    } else {
+      result = key.WriteValue(value_name_.c_str(), old_dw_);
+    }
+  }
+
+  if (result == ERROR_SUCCESS) {
     status_ = VALUE_ROLLED_BACK;
-    VLOG(1) << "rollback: restored " << value_name_;
+    VLOG(1) << "rollback: restored: " << value_name_ << " error: " << result;
   } else {
-    LOG(ERROR) << "failed to restore value " << value_name_;
+    LOG(ERROR) << "rollback failed: " << value_name_ << " error: " << result;
   }
 
   key.Close();
   return;
 }