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 71345)
+++ 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,11 @@
     : 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) {
+      type_(REG_SZ) {
+  const char* data = reinterpret_cast<const char*>(value_data.c_str());
+  value_.assign(data, value_data.length() * sizeof(wchar_t));

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

You must use (value_data.length() + 1) * sizeof(wchar_t) here since you need to
include the terminating NULL in the call to RegSetValueEx.

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

Done.

 }
 
 SetRegValueWorkItem::SetRegValueWorkItem(HKEY predefined_root,
@@ -37,13 +35,11 @@
     : 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) {
+      type_(REG_DWORD) {
+  const char* data = reinterpret_cast<const char*>(&value_data);
+  value_.assign(data, sizeof(value_data));
 }
 
 SetRegValueWorkItem::SetRegValueWorkItem(HKEY predefined_root,
@@ -54,151 +50,89 @@
     : 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) {
+      type_(REG_QWORD) {
+  const char* data = reinterpret_cast<const char*>(&value_data);
+  value_.assign(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 mroe to do.
+  if (!overwrite_ && result != ERROR_FILE_NOT_FOUND) {

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

nit: make the order of the tests and the order of the description in the comment
the same.

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

Done.

+    return ignore_failure_;
+  }
 
-          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_MORE_DATA) {

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

Same comment as before: MSDN leads me to believe that ERROR_SUCCESS is the
expected result here.

[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;
   }
 
-  LOG_IF(ERROR, !success && !log_message_.empty()) << log_message_;
-
-  if (ignore_failure_) {
-    success = true;
+  result = key.WriteValue(value_name_.c_str(), value_.c_str(),

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

If you stick with std::string, use data() rather than c_str().

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

Done.

+                          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_ = overwrite_ ? 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;
-    }
+  status_ = VALUE_ROLL_BACK;

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

This is wrong.

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

Good catch, removed.

+  if (status_ == VALUE_UNCHANGED) {
+    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_.c_str(),

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

If you stick with std::string, use data() rather than c_str().

+                            static_cast<DWORD>(previous_value_.size()),
+                            previous_type_);
+    VLOG(1) << "rollback: restoring " << value_name_ << " error: " << result;
+  } else {
+    NOTREACHED();
   }
 }