AU: Ensure update_engine_client get proxy retries are spaced out in time.
Hopefully, this will reduce test flakyness. It seems that last time the test
failed all 4 tries happened within a one-second timeframe.
Also, fix GError object memory leaks throughout the code.
BUG=chromium-os:21351
TEST=unit tests, tested on VM
Change-Id: If0bc5d5767d12f3396d0fcb46f3e04ed6d7dfd5c
Reviewed-on: http://gerrit.chromium.org/gerrit/8862
Commit-Ready: Darin Petkov <petkov@chromium.org>
Reviewed-by: Darin Petkov <petkov@chromium.org>
Tested-by: Darin Petkov <petkov@chromium.org>
diff --git a/chrome_browser_proxy_resolver.cc b/chrome_browser_proxy_resolver.cc
index 8c35afb..7b0bdc6 100644
--- a/chrome_browser_proxy_resolver.cc
+++ b/chrome_browser_proxy_resolver.cc
@@ -60,8 +60,8 @@
kLibCrosServiceInterface,
&error);
if (!proxy) {
- LOG(ERROR) << "Error getting dbus proxy for "
- << kLibCrosServiceName << ": " << utils::GetGErrorMessage(error);
+ LOG(ERROR) << "Error getting dbus proxy for " << kLibCrosServiceName << ": "
+ << utils::GetAndFreeGError(&error);
return NULL;
}
return proxy;
@@ -137,7 +137,7 @@
G_TYPE_STRING, kLibCrosProxyResolveName,
G_TYPE_INVALID, G_TYPE_INVALID)) {
LOG(WARNING) << "dbus_g_proxy_call failed: "
- << utils::GetGErrorMessage(error)
+ << utils::GetAndFreeGError(&error)
<< " Continuing with no proxy.";
timeout = 0;
}
diff --git a/chrome_proxy_resolver.cc b/chrome_proxy_resolver.cc
index 37ccd8c..25af4f6 100644
--- a/chrome_proxy_resolver.cc
+++ b/chrome_proxy_resolver.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -79,7 +79,8 @@
GError* error = NULL;
DBusGConnection* bus = dbus_->BusGet(DBUS_BUS_SYSTEM, &error);
if (!bus) {
- LOG(ERROR) << "Failed to get System Dbus";
+ LOG(ERROR) << "Failed to get System Dbus: "
+ << utils::GetAndFreeGError(&error);
return NULL;
}
DBusGProxy* proxy = dbus_->ProxyNewForNameOwner(bus,
@@ -89,7 +90,7 @@
&error);
if (!proxy) {
LOG(ERROR) << "Error getting FlimFlam proxy: "
- << utils::GetGErrorMessage(error);
+ << utils::GetAndFreeGError(&error);
}
return proxy;
}
@@ -101,7 +102,7 @@
kProxyModePACScript,
kProxyModeSingle,
kProxyModeProxyPerScheme
-};
+};
} // namespace {}
bool ChromeProxyResolver::GetProxiesForUrlWithSettings(
diff --git a/filesystem_copier_action.cc b/filesystem_copier_action.cc
index 364922b..340dd1d 100644
--- a/filesystem_copier_action.cc
+++ b/filesystem_copier_action.cc
@@ -157,7 +157,7 @@
ssize_t bytes_read = g_input_stream_read_finish(src_stream_, res, &error);
if (bytes_read < 0) {
- LOG(ERROR) << "Read failed: " << utils::GetGErrorMessage(error);
+ LOG(ERROR) << "Read failed: " << utils::GetAndFreeGError(&error);
failed_ = true;
buffer_state_[index] = kBufferStateEmpty;
} else if (bytes_read == 0) {
@@ -207,7 +207,7 @@
&error);
if (bytes_written < static_cast<ssize_t>(buffer_valid_size_[index])) {
if (bytes_written < 0) {
- LOG(ERROR) << "Write failed: " << utils::GetGErrorMessage(error);
+ LOG(ERROR) << "Write failed: " << utils::GetAndFreeGError(&error);
} else {
LOG(ERROR) << "Write was short: wrote " << bytes_written
<< " but expected to write " << buffer_valid_size_[index];
diff --git a/flimflam_proxy.cc b/flimflam_proxy.cc
index 27be084..4c1cfc9 100644
--- a/flimflam_proxy.cc
+++ b/flimflam_proxy.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -39,7 +39,7 @@
&error);
if (!proxy) {
LOG(ERROR) << "Error getting FlimFlam proxy: "
- << utils::GetGErrorMessage(error);
+ << utils::GetAndFreeGError(&error);
return false;
}
*out_proxy = proxy;
diff --git a/main.cc b/main.cc
index b96add3..fd84e50 100644
--- a/main.cc
+++ b/main.cc
@@ -58,9 +58,8 @@
GError *error = NULL;
bus = dbus_g_bus_get(DBUS_BUS_SYSTEM, &error);
- if (!bus) {
- LOG(FATAL) << "Failed to get bus";
- }
+ LOG_IF(FATAL, !bus) << "Failed to get bus: "
+ << utils::GetAndFreeGError(&error);
proxy = dbus_g_proxy_new_for_name(bus,
DBUS_SERVICE_DBUS,
DBUS_PATH_DBUS,
@@ -71,11 +70,10 @@
0,
&request_name_ret,
&error)) {
- LOG(FATAL) << "Failed to get name: " << error->message;
+ LOG(FATAL) << "Failed to get name: " << utils::GetAndFreeGError(&error);
}
if (request_name_ret != DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER) {
g_warning("Got result code %u from requesting name", request_name_ret);
- g_error_free(error);
LOG(FATAL) << "Got result code " << request_name_ret
<< " from requesting name, but expected "
<< DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER;
diff --git a/subprocess.cc b/subprocess.cc
index a638145..f0647e2 100644
--- a/subprocess.cc
+++ b/subprocess.cc
@@ -3,14 +3,19 @@
// found in the LICENSE file.
#include "update_engine/subprocess.h"
+
#include <stdlib.h>
#include <string.h>
-#include <string>
#include <unistd.h>
+
+#include <string>
#include <vector>
-#include "base/logging.h"
-#include "base/memory/scoped_ptr.h"
-#include "base/string_util.h"
+
+#include <base/logging.h>
+#include <base/memory/scoped_ptr.h>
+#include <base/string_util.h>
+
+#include "update_engine/utils.h"
using std::string;
using std::tr1::shared_ptr;
@@ -220,10 +225,7 @@
return_code,
&err);
FreeArgv(argv.get());
- if (err) {
- LOG(INFO) << "err is: " << err->code << ", " << err->message;
- g_error_free(err);
- }
+ LOG_IF(INFO, err) << utils::GetAndFreeGError(&err);
if (child_stdout) {
if (stdout) {
*stdout = child_stdout;
diff --git a/update_engine_client.cc b/update_engine_client.cc
index fb2d418..db14fb6 100644
--- a/update_engine_client.cc
+++ b/update_engine_client.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -19,7 +19,7 @@
using chromeos_update_engine::kUpdateEngineServiceName;
using chromeos_update_engine::kUpdateEngineServicePath;
using chromeos_update_engine::kUpdateEngineServiceInterface;
-using chromeos_update_engine::utils::GetGErrorMessage;
+using chromeos_update_engine::utils::GetAndFreeGError;
using std::string;
DEFINE_string(app_version, "", "Force the current app version.");
@@ -41,24 +41,27 @@
DBusGProxy* proxy = NULL;
GError* error = NULL;
const int kTries = 4;
+ const int kRetrySeconds = 10;
bus = dbus_g_bus_get(DBUS_BUS_SYSTEM, &error);
- if (!bus) {
- LOG(FATAL) << "Failed to get bus";
- }
+ LOG_IF(FATAL, !bus) << "Failed to get bus: " << GetAndFreeGError(&error);
for (int i = 0; !proxy && i < kTries; ++i) {
- LOG_IF(INFO, i) << "Trying to get dbus proxy. Try "
- << (i + 1) << "/" << kTries;
+ if (i > 0) {
+ LOG(INFO) << "Retrying to get dbus proxy. Try "
+ << (i + 1) << "/" << kTries;
+ sleep(kRetrySeconds);
+ }
proxy = dbus_g_proxy_new_for_name_owner(bus,
kUpdateEngineServiceName,
kUpdateEngineServicePath,
kUpdateEngineServiceInterface,
&error);
+ LOG_IF(WARNING, !proxy) << "Error getting dbus proxy for "
+ << kUpdateEngineServiceName << ": "
+ << GetAndFreeGError(&error);
}
- if (!proxy) {
- LOG(FATAL) << "Error getting dbus proxy for "
- << kUpdateEngineServiceName << ": " << GetGErrorMessage(error);
- }
+ LOG_IF(FATAL, !proxy) << "Giving up -- unable to get dbus proxy for "
+ << kUpdateEngineServiceName;
*out_proxy = proxy;
return true;
}
@@ -101,7 +104,7 @@
&new_size,
&error);
if (rc == FALSE) {
- LOG(INFO) << "Error getting status: " << GetGErrorMessage(error);
+ LOG(INFO) << "Error getting status: " << GetAndFreeGError(&error);
}
printf("LAST_CHECKED_TIME=%" PRIi64 "\nPROGRESS=%f\nCURRENT_OP=%s\n"
"NEW_VERSION=%s\nNEW_SIZE=%" PRIi64 "\n",
@@ -164,7 +167,7 @@
omaha_url.c_str(),
&error);
CHECK_EQ(rc, TRUE) << "Error checking for update: "
- << GetGErrorMessage(error);
+ << GetAndFreeGError(&error);
return true;
}
@@ -179,7 +182,7 @@
// Reboot error code doesn't necessarily mean that a reboot
// failed. For example, D-Bus may be shutdown before we receive the
// result.
- LOG_IF(INFO, !rc) << "Reboot error message: " << GetGErrorMessage(error);
+ LOG_IF(INFO, !rc) << "Reboot error message: " << GetAndFreeGError(&error);
return true;
}
@@ -194,7 +197,7 @@
track.c_str(),
&error);
CHECK_EQ(rc, true) << "Error setting the track: "
- << GetGErrorMessage(error);
+ << GetAndFreeGError(&error);
LOG(INFO) << "Track permanently set to: " << track;
}
@@ -209,8 +212,7 @@
org_chromium_UpdateEngineInterface_get_track(proxy,
&track,
&error);
- CHECK_EQ(rc, true) << "Error getting the track: "
- << GetGErrorMessage(error);
+ CHECK_EQ(rc, true) << "Error getting the track: " << GetAndFreeGError(&error);
string output = track;
g_free(track);
return output;
diff --git a/utils.cc b/utils.cc
index bcf34d5..5273347 100644
--- a/utils.cc
+++ b/utils.cc
@@ -518,10 +518,17 @@
return true;
}
-const char* GetGErrorMessage(const GError* error) {
- if (!error)
- return "Unknown error.";
- return error->message;
+string GetAndFreeGError(GError** error) {
+ if (!*error) {
+ return "Unknown GLib error.";
+ }
+ string message =
+ base::StringPrintf("GError(%d): %s",
+ (*error)->code,
+ (*error)->message ? (*error)->message : "(unknown)");
+ g_error_free(*error);
+ *error = NULL;
+ return message;
}
bool Reboot() {
diff --git a/utils.h b/utils.h
index 7b39cd4..b0b28a8 100644
--- a/utils.h
+++ b/utils.h
@@ -142,8 +142,9 @@
// param. Returns true on success.
bool GetBootloader(BootLoader* out_bootloader);
-// Returns the error message, if any, from a GError pointer.
-const char* GetGErrorMessage(const GError* error);
+// Returns the error message, if any, from a GError pointer. Frees the GError
+// object and resets error to NULL.
+std::string GetAndFreeGError(GError** error);
// Initiates a system reboot. Returns true on success, false otherwise.
bool Reboot();