Regkey functions return error code instead of bool

chrome/installer/util/set_reg_value_work_item.cc

Index: chrome/installer/util/set_reg_value_work_item.cc
===================================================================
--- chrome/installer/util/set_reg_value_work_item.cc	(revision 71561)
+++ chrome/installer/util/set_reg_value_work_item.cc	(working copy)
@@ -5,6 +5,7 @@
 #include "chrome/installer/util/set_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"
 
@@ -19,14 +20,12 @@
     : predefined_root_(predefined_root),
       key_path_(key_path),
       value_name_(value_name),
-      value_data_str_(value_data),
-      value_data_dword_(0),
-      value_data_qword_(0),
       overwrite_(overwrite),
       status_(SET_VALUE),
       type_(REG_SZ),
-      previous_value_dword_(0),
-      previous_value_qword_(0) {
+      previous_type_(0) {
+  const uint8* data = reinterpret_cast<const uint8*>(value_data.c_str());
+  value_.assign(data, data + (value_data.length() + 1) * sizeof(wchar_t));
 }
 
 SetRegValueWorkItem::SetRegValueWorkItem(HKEY predefined_root,
@@ -37,13 +36,12 @@
     : predefined_root_(predefined_root),
       key_path_(key_path),
       value_name_(value_name),
-      value_data_dword_(value_data),
-      value_data_qword_(0),
       overwrite_(overwrite),
       status_(SET_VALUE),
       type_(REG_DWORD),
-      previous_value_dword_(0),
-      previous_value_qword_(0) {
+      previous_type_(0) {
+  const uint8* data = reinterpret_cast<const uint8*>(&value_data);
+  value_.assign(data, data + sizeof(value_data));
 }
 
 SetRegValueWorkItem::SetRegValueWorkItem(HKEY predefined_root,
@@ -54,151 +52,94 @@
     : predefined_root_(predefined_root),
       key_path_(key_path),
       value_name_(value_name),
-      value_data_dword_(0),
-      value_data_qword_(value_data),
       overwrite_(overwrite),
       status_(SET_VALUE),
       type_(REG_QWORD),
-      previous_value_dword_(0),
-      previous_value_qword_(0) {
+      previous_type_(0) {
+  const uint8* data = reinterpret_cast<const uint8*>(&value_data);
+  value_.assign(data, data + sizeof(value_data));
 }
 
 bool SetRegValueWorkItem::Do() {
-  bool success = true;
-
+  LONG result = ERROR_SUCCESS;
+  base::win::RegKey key;
   if (status_ != SET_VALUE) {
     // we already did something.
-    LOG(ERROR) << "multiple calls to Do()";
-    success = false;
+    VLOG(1) << "multiple calls to Do()";
+    result = ERROR_CANTWRITE;
+    return ignore_failure_;
   }
 
-  base::win::RegKey key;
-  if (success && !key.Open(predefined_root_, key_path_.c_str(),
-                           KEY_READ | KEY_SET_VALUE)) {
-    LOG(ERROR) << "can not open " << key_path_;
-    status_ = VALUE_UNCHANGED;
-    success = false;
+  status_ = VALUE_UNCHANGED;
+  result = key.Open(predefined_root_, key_path_.c_str(),
+                    KEY_READ | KEY_SET_VALUE);
+  if (result != ERROR_SUCCESS) {
+    VLOG(1) << "can not open " << key_path_ << " error: " << result;
+    return ignore_failure_;
   }
 
-  if (success) {
-    if (key.ValueExists(value_name_.c_str())) {
-      if (overwrite_) {
-        // Read previous value for rollback and write new value
-        if (type_ == REG_SZ) {
-          std::wstring data;
-          if (key.ReadValue(value_name_.c_str(), &data)) {
-            previous_value_str_.assign(data);
-          }
-          success = key.WriteValue(value_name_.c_str(),
-                                   value_data_str_.c_str());
-        } else if (type_ == REG_DWORD) {
-          DWORD data;
-          if (key.ReadValueDW(value_name_.c_str(), &data)) {
-            previous_value_dword_ = data;
-          }
-          success = key.WriteValue(value_name_.c_str(), value_data_dword_);
-        } else if (type_ == REG_QWORD) {
-          int64 data;
-          DWORD data_size = sizeof(data);
-          DWORD data_type = NULL;
+  DWORD type = 0;
+  DWORD size = 0;
+  result = key.ReadValue(value_name_.c_str(), NULL, &size, &type);
+  // If the value exists but we don't want to overwrite then there's
+  // nothing more to do.
+  if ((result != ERROR_FILE_NOT_FOUND) && !overwrite_) {
+    return true;

[grt (UTC plus 2), 2011/01/16 04:19:48]

Nice catch!

+  }
 
-          if (key.ReadValue(value_name_.c_str(), &data, &data_size,
-                            &data_type)) {
-            previous_value_qword_ = data;
-          }
-          success = key.WriteValue(value_name_.c_str(),
-                                   &value_data_qword_,
-                                   sizeof(value_data_qword_),
-                                   REG_QWORD);
-        }
-        if (success) {
-          VLOG(1) << "overwritten value for " << value_name_;
-          status_ = VALUE_OVERWRITTEN;
-        } else {
-          LOG(ERROR) << "failed to overwrite value for " << value_name_;
-          status_ = VALUE_UNCHANGED;
-        }
-      } else {
-        VLOG(1) << value_name_ << " exists. not changed ";
-        status_ = VALUE_UNCHANGED;
-      }
-    } else {
-      if (type_ == REG_SZ) {
-        success = key.WriteValue(value_name_.c_str(), value_data_str_.c_str());
-      } else if (type_ == REG_DWORD) {
-        success = key.WriteValue(value_name_.c_str(), value_data_dword_);
-      } else if (type_ == REG_QWORD) {
-        success = key.WriteValue(value_name_.c_str(),
-                                 &value_data_qword_,
-                                 sizeof(value_data_qword_),
-                                 REG_QWORD);
-      } else {
-        NOTREACHED() << "Unsupported value type.";
-      }
-      if (success) {
-        VLOG(1) << "created value for " << value_name_;
-        status_ = NEW_VALUE_CREATED;
-      } else {
-        LOG(ERROR) << "failed to create value for " << value_name_;
-        status_ = VALUE_UNCHANGED;
-      }
+  // If there's something to be saved, save it.
+  if (result == ERROR_SUCCESS) {
+    previous_value_.resize(size);
+    result = key.ReadValue(value_name_.c_str(), &previous_value_[0], &size,
+                           &previous_type_);
+    if (result != ERROR_SUCCESS) {
+      previous_value_.clear();
+      VLOG(1) << "Failed to save original value. Error: " << result;
     }
   }
 
-  LOG_IF(ERROR, !success && !log_message_.empty()) << log_message_;
-
-  if (ignore_failure_) {
-    success = true;
+  result = key.WriteValue(value_name_.c_str(), &value_[0],
+                          static_cast<DWORD>(value_.size()), type_);
+  if (result != ERROR_SUCCESS) {
+    VLOG(1) << "Failed to write value " << key_path_ << " error: " << result;
+    return ignore_failure_;
   }
 
-  return success;
+  status_ = previous_type_ ? VALUE_OVERWRITTEN : NEW_VALUE_CREATED;
+  return true;
 }
 
 void SetRegValueWorkItem::Rollback() {
-  if (!ignore_failure_) {
-    if (status_ == SET_VALUE || status_ == VALUE_ROLL_BACK)
-      return;
+  if (ignore_failure_)
+    return;
 
-    if (status_ == VALUE_UNCHANGED) {
-      status_ = VALUE_ROLL_BACK;
-      VLOG(1) << "rollback: setting unchanged, nothing to do";
-      return;
-    }
+  if (status_ == SET_VALUE || status_ == VALUE_ROLL_BACK)
+    return;
 
-    base::win::RegKey key;
-    if (!key.Open(predefined_root_, key_path_.c_str(), KEY_SET_VALUE)) {
-      status_ = VALUE_ROLL_BACK;
-      VLOG(1) << "rollback: can not open " << key_path_;
-      return;
-    }
+  if (status_ == VALUE_UNCHANGED) {
+    status_ = VALUE_ROLL_BACK;
+    VLOG(1) << "rollback: setting unchanged, nothing to do";
+    return;
+  }
 
-    std::wstring result_str(L" failed");
-    if (status_ == NEW_VALUE_CREATED) {
-      if (key.DeleteValue(value_name_.c_str()))
-        result_str.assign(L" succeeded");
-      VLOG(1) << "rollback: deleting " << value_name_ << result_str;
-    } else if (status_ == VALUE_OVERWRITTEN) {
-      // try restore the previous value
-      bool success = true;
-      if (type_ == REG_SZ) {
-        success = key.WriteValue(value_name_.c_str(),
-                                 previous_value_str_.c_str());
-      } else if (type_ == REG_DWORD) {
-        success = key.WriteValue(value_name_.c_str(), previous_value_dword_);
-      } else if (type_ == REG_QWORD) {
-        success = key.WriteValue(value_name_.c_str(),
-                                 &previous_value_qword_,
-                                 sizeof(previous_value_qword_),
-                                 REG_QWORD);
-      } else {
-        NOTREACHED();
-      }
-      if (success)
-        result_str.assign(L" succeeded");
-      VLOG(1) << "rollback: restoring " << value_name_ << result_str;
+  base::win::RegKey key;
+  LONG result = key.Open(predefined_root_, key_path_.c_str(), KEY_SET_VALUE);
+  if (result != ERROR_SUCCESS) {
+    VLOG(1) << "rollback: can not open " << key_path_ << " error: " << result;
+    return;
+  }
 
-      status_ = VALUE_ROLL_BACK;
-      return;
-    }
+  if (status_ == NEW_VALUE_CREATED) {
+    result = key.DeleteValue(value_name_.c_str());
+    VLOG(1) << "rollback: deleting " << value_name_ << " error: " << result;
+  } else if (status_ == VALUE_OVERWRITTEN) {
+    result = key.WriteValue(value_name_.c_str(), &previous_value_[0],
+                            static_cast<DWORD>(previous_value_.size()),
+                            previous_type_);
+    VLOG(1) << "rollback: restoring " << value_name_ << " error: " << result;
+  } else {
+    NOTREACHED();
   }
+
+  status_ = VALUE_ROLL_BACK;
 }