metrics library: convert to proper C++/libbase
This mostly converts fprintfs to proper logs, char* to
std::string wherever appropriate.
BUG=chromium:355796
TEST=unit tests
Change-Id: Ieb1cb110be5e281b7e0c764a0dfce895f33d4a3c
Reviewed-on: https://chromium-review.googlesource.com/199610
Reviewed-by: Luigi Semenzato <semenzato@chromium.org>
Commit-Queue: Luigi Semenzato <semenzato@chromium.org>
Tested-by: Luigi Semenzato <semenzato@chromium.org>
diff --git a/metrics/metrics_library.cc b/metrics/metrics_library.cc
index 32bacf6..97cb278 100644
--- a/metrics/metrics_library.cc
+++ b/metrics/metrics_library.cc
@@ -4,6 +4,8 @@
#include "metrics_library.h"
+#include <base/logging.h>
+#include <base/strings/stringprintf.h>
#include <errno.h>
#include <sys/file.h>
#include <sys/stat.h>
@@ -51,23 +53,6 @@
time_t MetricsLibrary::cached_enabled_time_ = 0;
bool MetricsLibrary::cached_enabled_ = false;
-using std::string;
-
-// TODO(sosa@chromium.org) - use Chromium logger instead of stderr
-static void PrintError(const char* message, const char* file,
- int code) {
- static const char kProgramName[] = "libmetrics";
- if (code == 0) {
- fprintf(stderr, "%s: %s\n", kProgramName, message);
- } else if (file == NULL) {
- fprintf(stderr, "%s: ", kProgramName);
- perror(message);
- } else {
- fprintf(stderr, "%s: %s: ", kProgramName, file);
- perror(message);
- }
-}
-
// Copied from libbase to avoid pulling in all of libbase just for libmetrics.
static int WriteFileDescriptor(const int fd, const char* data, int size) {
// Allow for partial writes.
@@ -84,10 +69,7 @@
return bytes_written_total;
}
-MetricsLibrary::MetricsLibrary()
- : uma_events_file_(NULL),
- consent_file_(kConsentFile) {}
-
+MetricsLibrary::MetricsLibrary() : consent_file_(kConsentFile) {}
MetricsLibrary::~MetricsLibrary() {}
// We take buffer and buffer_size as parameters in order to simplify testing
@@ -178,7 +160,7 @@
// still respect the consent file if it is present for migration purposes.
// TODO(pastarmovj)
if (!has_policy) {
- enabled = stat(consent_file_, &stat_buffer) >= 0;
+ enabled = stat(consent_file_.c_str(), &stat_buffer) >= 0;
}
if (enabled && !IsGuestMode())
@@ -189,14 +171,20 @@
return cached_enabled_;
}
-bool MetricsLibrary::SendMessageToChrome(int32_t length, const char* message) {
-
- int chrome_fd = HANDLE_EINTR(open(uma_events_file_,
+bool MetricsLibrary::SendMessageToChrome(const std::string& message) {
+ int size = static_cast<int>(message.size());
+ if (size > kBufferSize) {
+ LOG(ERROR) << "chrome message too big (" << size << " bytes)";
+ return false;
+ }
+ // Use libc here instead of chromium base classes because we need a UNIX fd
+ // for flock.
+ int chrome_fd = HANDLE_EINTR(open(uma_events_file_.c_str(),
O_WRONLY | O_APPEND | O_CREAT,
READ_WRITE_ALL_FILE_FLAGS));
// If we failed to open it, return.
if (chrome_fd < 0) {
- PrintError("open", uma_events_file_, errno);
+ PLOG(ERROR) << uma_events_file_ << ": open";
return false;
}
@@ -206,16 +194,16 @@
fchmod(chrome_fd, READ_WRITE_ALL_FILE_FLAGS);
// Grab an exclusive lock to protect Chrome from truncating
- // underneath us. Keep the file locked as briefly as possible.
+ // underneath us.
if (HANDLE_EINTR(flock(chrome_fd, LOCK_EX)) < 0) {
- PrintError("flock", uma_events_file_, errno);
+ PLOG(ERROR) << uma_events_file_ << ": flock";
IGNORE_EINTR(close(chrome_fd));
return false;
}
bool success = true;
- if (WriteFileDescriptor(chrome_fd, message, length) != length) {
- PrintError("write", uma_events_file_, errno);
+ if (WriteFileDescriptor(chrome_fd, message.c_str(), size) != size) {
+ PLOG(ERROR) << uma_events_file_ << ": write";
success = false;
}
@@ -224,44 +212,28 @@
return success;
}
-int32_t MetricsLibrary::FormatChromeMessage(int32_t buffer_size, char* buffer,
- const char* format, ...) {
- int32_t message_length;
- size_t len_size = sizeof(message_length);
-
- // Format the non-LENGTH contents in the buffer by leaving space for
- // LENGTH at the start of the buffer.
- va_list args;
- va_start(args, format);
- message_length = vsnprintf(&buffer[len_size], buffer_size - len_size,
- format, args);
- va_end(args);
-
- if (message_length < 0) {
- PrintError("chrome message format error", NULL, 0);
- return -1;
- }
-
- // +1 to account for the trailing \0.
- message_length += len_size + 1;
- if (message_length > buffer_size) {
- PrintError("chrome message too long", NULL, 0);
- return -1;
- }
-
- // Prepend LENGTH to the message.
- memcpy(buffer, &message_length, len_size);
- return message_length;
+const std::string MetricsLibrary::FormatChromeMessage(
+ const std::string& name,
+ const std::string& value) {
+ uint32 message_length =
+ sizeof(message_length) + name.size() + 1 + value.size() + 1;
+ std::string result;
+ result.reserve(message_length);
+ // Marshal the total message length in the native byte order.
+ result.assign(reinterpret_cast<char*>(&message_length),
+ sizeof(message_length));
+ result += name + '\0' + value + '\0';
+ return result;
}
void MetricsLibrary::Init() {
uma_events_file_ = kUMAEventsPath;
}
-bool MetricsLibrary::SendToAutotest(const string& name, int value) {
+bool MetricsLibrary::SendToAutotest(const std::string& name, int value) {
FILE* autotest_file = fopen(kAutotestPath, "a+");
if (autotest_file == NULL) {
- PrintError("fopen", kAutotestPath, errno);
+ PLOG(ERROR) << kAutotestPath << ": fopen";
return false;
}
@@ -270,74 +242,48 @@
return true;
}
-bool MetricsLibrary::SendToUMA(const string& name, int sample,
- int min, int max, int nbuckets) {
+bool MetricsLibrary::SendToUMA(const std::string& name,
+ int sample,
+ int min,
+ int max,
+ int nbuckets) {
// Format the message.
- char message[kBufferSize];
- int32_t message_length =
- FormatChromeMessage(kBufferSize, message,
- "histogram%c%s %d %d %d %d", '\0',
- name.c_str(), sample, min, max, nbuckets);
- if (message_length < 0)
- return false;
-
+ std::string value = base::StringPrintf("%s %d %d %d %d",
+ name.c_str(), sample, min, max, nbuckets);
+ std::string message = FormatChromeMessage("histogram", value);
// Send the message.
- return SendMessageToChrome(message_length, message);
+ return SendMessageToChrome(message);
}
bool MetricsLibrary::SendEnumToUMA(const std::string& name, int sample,
int max) {
// Format the message.
- char message[kBufferSize];
- int32_t message_length =
- FormatChromeMessage(kBufferSize, message,
- "linearhistogram%c%s %d %d", '\0',
- name.c_str(), sample, max);
- if (message_length < 0)
- return false;
-
+ std::string value = base::StringPrintf("%s %d %d", name.c_str(), sample, max);
+ std::string message = FormatChromeMessage("linearhistogram", value);
// Send the message.
- return SendMessageToChrome(message_length, message);
+ return SendMessageToChrome(message);
}
bool MetricsLibrary::SendSparseToUMA(const std::string& name, int sample) {
// Format the message.
- char message[kBufferSize];
- int32_t message_length =
- FormatChromeMessage(kBufferSize, message, "sparsehistogram%c%s %d",
- '\0', name.c_str(), sample);
- if (message_length < 0)
- return false;
-
+ std::string value = base::StringPrintf("%s %d", name.c_str(), sample);
+ std::string message = FormatChromeMessage("sparsehistogram", value);
// Send the message.
- return SendMessageToChrome(message_length, message);
+ return SendMessageToChrome(message);
}
bool MetricsLibrary::SendUserActionToUMA(const std::string& action) {
// Format the message.
- char message[kBufferSize];
- int32_t message_length =
- FormatChromeMessage(kBufferSize, message,
- "useraction%c%s", '\0', action.c_str());
- if (message_length < 0)
- return false;
-
+ std::string message = FormatChromeMessage("useraction", action);
// Send the message.
- return SendMessageToChrome(message_length, message);
+ return SendMessageToChrome(message);
}
bool MetricsLibrary::SendCrashToUMA(const char *crash_kind) {
// Format the message.
- char message[kBufferSize];
- int32_t message_length =
- FormatChromeMessage(kBufferSize, message,
- "crash%c%s", '\0', crash_kind);
-
- if (message_length < 0)
- return false;
-
+ std::string message = FormatChromeMessage("crash", crash_kind);
// Send the message.
- return SendMessageToChrome(message_length, message);
+ return SendMessageToChrome(message);
}
void MetricsLibrary::SetPolicyProvider(policy::PolicyProvider* provider) {
diff --git a/metrics/metrics_library.h b/metrics/metrics_library.h
index e9c6f4b..74f5de2 100644
--- a/metrics/metrics_library.h
+++ b/metrics/metrics_library.h
@@ -133,22 +133,16 @@
// Sends message of size |length| to Chrome for transport to UMA and
// returns true on success.
- bool SendMessageToChrome(int32_t length, const char* message);
+ bool SendMessageToChrome(const std::string& message);
- // Formats a name/value message for Chrome in |buffer| and returns the
- // length of the message or a negative value on error.
+ // Serializes a name/value pair into a message buffer.
//
- // Message format is: | LENGTH(binary) | NAME | \0 | VALUE | \0 |
+ // The serialized format is: | LENGTH | NAME | \0 | VALUE | \0 |
//
- // The arbitrary |format| argument covers the non-LENGTH portion of the
- // message. The caller is responsible to store the \0 character
- // between NAME and VALUE (e.g. "%s%c%d", name, '\0', value).
- //
- // Ideally we'd use "3, 4" here instead of "4, 5", but it seems clang/gcc
- // have an implicit first arg ("this"). http://crbug.com/329356
- __attribute__((__format__(__printf__, 4, 5)))
- int32_t FormatChromeMessage(int32_t buffer_size, char* buffer,
- const char* format, ...);
+ // where LENGTH is a 32-bit integer in native endianness, and NAME and VALUE
+ // are null-terminated strings (the zero bytes are explicitly shown above).
+ const std::string FormatChromeMessage(const std::string& name,
+ const std::string& value);
// This function is used by tests only to mock the device policies.
void SetPolicyProvider(policy::PolicyProvider* provider);
@@ -159,8 +153,8 @@
// Cached state of whether or not metrics were enabled.
static bool cached_enabled_;
- const char* uma_events_file_;
- const char* consent_file_;
+ std::string uma_events_file_;
+ std::string consent_file_;
scoped_ptr<policy::PolicyProvider> policy_provider_;
diff --git a/metrics/metrics_library_test.cc b/metrics/metrics_library_test.cc
index 0a0768b..f6cd7e7 100644
--- a/metrics/metrics_library_test.cc
+++ b/metrics/metrics_library_test.cc
@@ -29,10 +29,10 @@
class MetricsLibraryTest : public testing::Test {
protected:
virtual void SetUp() {
- EXPECT_EQ(NULL, lib_.uma_events_file_);
+ EXPECT_TRUE(lib_.uma_events_file_.empty());
lib_.Init();
- EXPECT_TRUE(NULL != lib_.uma_events_file_);
- lib_.uma_events_file_ = kTestUMAEventsFile.value().c_str();
+ EXPECT_FALSE(lib_.uma_events_file_.empty());
+ lib_.uma_events_file_ = kTestUMAEventsFile.value();
device_policy_ = new policy::MockDevicePolicy();
EXPECT_CALL(*device_policy_, LoadPolicy())
.Times(AnyNumber())
@@ -165,18 +165,12 @@
}
TEST_F(MetricsLibraryTest, FormatChromeMessage) {
- char buf[7];
- const int kLen = 6;
- EXPECT_EQ(kLen, lib_.FormatChromeMessage(7, buf, "%d", 1));
-
- char exp[kLen];
- sprintf(exp, "%c%c%c%c1", kLen, 0, 0, 0);
- EXPECT_EQ(0, memcmp(exp, buf, kLen));
-}
-
-TEST_F(MetricsLibraryTest, FormatChromeMessageTooLong) {
- char buf[7];
- EXPECT_EQ(-1, lib_.FormatChromeMessage(7, buf, "test"));
+ char kLen = 10;
+ char exp[] = { kLen, 0, 0, 0, 'f', 'o', 'o', 0, '1', 0 };
+ // The message is composed by a 4-byte length field, followed
+ // by two null-terminated strings (name/value).
+ EXPECT_EQ(sizeof(exp), kLen);
+ EXPECT_EQ(std::string(exp, kLen), lib_.FormatChromeMessage("foo", "1"));
}
TEST_F(MetricsLibraryTest, SendEnumToUMA) {
@@ -192,8 +186,8 @@
}
TEST_F(MetricsLibraryTest, SendMessageToChrome) {
- EXPECT_TRUE(lib_.SendMessageToChrome(4, "test"));
- EXPECT_TRUE(lib_.SendMessageToChrome(7, "content"));
+ EXPECT_TRUE(lib_.SendMessageToChrome(std::string("test")));
+ EXPECT_TRUE(lib_.SendMessageToChrome(std::string("content")));
std::string uma_events;
EXPECT_TRUE(base::ReadFileToString(kTestUMAEventsFile, &uma_events));
EXPECT_EQ("testcontent", uma_events);
@@ -204,8 +198,8 @@
// created.
static const char kDoesNotExistFile[] = "/does/not/exist";
lib_.uma_events_file_ = kDoesNotExistFile;
- static const char kDummyMessage[] = "Dummy Message";
- EXPECT_FALSE(lib_.SendMessageToChrome(strlen(kDummyMessage), kDummyMessage));
+ const std::string kDummyMessage = "Dummy Message";
+ EXPECT_FALSE(lib_.SendMessageToChrome(kDummyMessage));
base::DeleteFile(FilePath(kDoesNotExistFile), false);
}
@@ -258,10 +252,10 @@
virtual void SetUp() {
lib_ = CMetricsLibraryNew();
MetricsLibrary& ml = *reinterpret_cast<MetricsLibrary*>(lib_);
- EXPECT_EQ(NULL, ml.uma_events_file_);
+ EXPECT_TRUE(ml.uma_events_file_.empty());
CMetricsLibraryInit(lib_);
- EXPECT_TRUE(NULL != ml.uma_events_file_);
- ml.uma_events_file_ = kTestUMAEventsFile.value().c_str();
+ EXPECT_FALSE(ml.uma_events_file_.empty());
+ ml.uma_events_file_ = kTestUMAEventsFile.value();
device_policy_ = new policy::MockDevicePolicy();
EXPECT_CALL(*device_policy_, LoadPolicy())
.Times(AnyNumber())