Regkey functions return error code instead of bool

chrome/installer/util/delete_reg_value_work_item.cc

Index: chrome/installer/util/delete_reg_value_work_item.cc
===================================================================
--- chrome/installer/util/delete_reg_value_work_item.cc	(revision 71345)
+++ chrome/installer/util/delete_reg_value_work_item.cc	(working copy)
@@ -5,6 +5,7 @@
 #include "chrome/installer/util/delete_reg_value_work_item.h"
 
 #include "base/logging.h"
+#include "base/string_util.h"
 #include "base/win/registry.h"
 #include "chrome/installer/util/logging_installer.h"
 
@@ -12,16 +13,12 @@
 
 DeleteRegValueWorkItem::DeleteRegValueWorkItem(HKEY predefined_root,
                                                const std::wstring& key_path,
-                                               const std::wstring& value_name,
-                                               DWORD type)
+                                               const std::wstring& value_name)
     : predefined_root_(predefined_root),
       key_path_(key_path),
       value_name_(value_name),
-      type_(type),
-      status_(DELETE_VALUE),
-      old_dw_(0),
-      old_qword_(0) {
-  DCHECK(type_ == REG_QWORD || type_ == REG_DWORD || type == REG_SZ);
+      previous_type_(0),
+      status_(DELETE_VALUE) {
 }
 
 DeleteRegValueWorkItem::~DeleteRegValueWorkItem() {
@@ -36,51 +33,36 @@
 
   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;
-  bool result = false;
-
-  // Used in REG_QWORD case only
-  DWORD value_size = sizeof(old_qword_);
   DWORD type = 0;
+  DWORD size = 0;
+  LONG result = key.Open(predefined_root_, key_path_.c_str(), KEY_READ);
+  if (result == ERROR_SUCCESS)
+    result = key.ReadValue(value_name_.c_str(), NULL, &size, &type);
 
-  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 (result == ERROR_FILE_NOT_FOUND) {
+    LOG(INFO) << "(delete value) Key: " << key_path_ << " or Value: "
+              << value_name_ << " does not exist.";
     status_ = VALUE_NOT_FOUND;
-    result = true;
-  // Read previous value for rollback and delete
-  } else if (((type_ == REG_SZ && key.ReadValue(value_name_.c_str(),
-                                                &old_str_)) ||
-              (type_ == REG_DWORD && key.ReadValueDW(value_name_.c_str(),
-                                                     &old_dw_)) ||
-              (type_ == REG_QWORD && key.ReadValue(value_name_.c_str(),
-                                                   &old_qword_,
-                                                   &value_size, &type) &&
-                  type == REG_QWORD && value_size == sizeof(old_qword_))) &&
-             (key.DeleteValue(value_name_.c_str()))) {
-    status_ = VALUE_DELETED;
-    result = true;
-  } else {
-    LOG(ERROR) << "failed to read/delete value " << value_name_;
+    return true;
   }
 
-  return result;
+  if (result == ERROR_MORE_DATA) {

[grt (UTC plus 2), 2011/01/14 15:53:43]

Have you verified this?  According to MSDN, you should be testing for
ERROR_SUCCESS here rather than MORE_DATA: "If lpData is NULL, and lpcbData is
non-NULL, the function returns ERROR_SUCCESS and stores the size of the data, in
bytes, in the variable pointed to by lpcbData."

[amit, 2011/01/15 01:28:11]

Done.

+    result = key.ReadValue(value_name_.c_str(),
+                           WriteInto(&previous_value_, size), &size,
+                           &previous_type_);
+    VLOG_IF(1, result != ERROR_SUCCESS) << "Failed to save original value. "
+                                           "Error: " << result;
+  }
+
+  result = key.DeleteValue(value_name_.c_str());
+  if (result != ERROR_SUCCESS) {
+    VLOG(1) << "Failed to delete value " << value_name_ << " error: " << result;
+    return false;
+  }
+
+  status_ = VALUE_DELETED;
+  return true;
 }
 
 void DeleteRegValueWorkItem::Rollback() {
@@ -94,23 +76,16 @@
 
   // 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_;
-  // try to restore the previous value
-  } else if ((type_ == REG_SZ && key.WriteValue(value_name_.c_str(),
-                                                old_str_.c_str())) ||
-             (type_ == REG_DWORD && key.WriteValue(value_name_.c_str(),
-                                                   old_dw_)) ||
-             (type_ == REG_QWORD && key.WriteValue(value_name_.c_str(),
-                                                   &old_qword_,
-                                                   sizeof(old_qword_),
-                                                   REG_QWORD))) {
-    status_ = VALUE_ROLLED_BACK;
-    VLOG(1) << "rollback: restored " << value_name_;
+  LONG result = key.Open(predefined_root_, key_path_.c_str(),
+                         KEY_READ | KEY_WRITE);
+  if (result == ERROR_SUCCESS) {
+    // try to restore the previous value
+    result = key.WriteValue(value_name_.c_str(), previous_value_.c_str(),

[grt (UTC plus 2), 2011/01/14 15:53:43]

If you use std::string where std::vector<uint8> belongs, please use data() to
get at the bytes rather than c_str().

+                            static_cast<DWORD>(previous_value_.size()),
+                            previous_type_);
+    VLOG_IF(1, result != ERROR_SUCCESS) << "rollback: restoring "
+                                        << value_name_ << " error: " << result;
   } else {
-    LOG(ERROR) << "failed to restore value " << value_name_;
+    VLOG(1) << "can not open " << key_path_ << " error: " << result;
   }
-
-  key.Close();
-  return;
 }