diff --git a/metrics/metrics_library.cc b/metrics/metrics_library.cc index 32bacf6a7..97cb278fd 100644 --- a/metrics/metrics_library.cc +++ b/metrics/metrics_library.cc @@ -4,6 +4,8 @@ #include "metrics_library.h" +#include +#include #include #include #include @@ -51,23 +53,6 @@ static const char *kCrosEventNames[] = { 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 @@ static int WriteFileDescriptor(const int fd, const char* data, int size) { 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 @@ bool MetricsLibrary::AreMetricsEnabled() { // 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 @@ bool MetricsLibrary::AreMetricsEnabled() { 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(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 @@ bool MetricsLibrary::SendMessageToChrome(int32_t length, const char* message) { 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 @@ bool MetricsLibrary::SendMessageToChrome(int32_t length, const char* message) { 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(&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 @@ bool MetricsLibrary::SendToAutotest(const string& name, int value) { 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 e9c6f4be4..74f5de226 100644 --- a/metrics/metrics_library.h +++ b/metrics/metrics_library.h @@ -133,22 +133,16 @@ class MetricsLibrary : public MetricsLibraryInterface { // 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 @@ class MetricsLibrary : public MetricsLibraryInterface { // 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_provider_; diff --git a/metrics/metrics_library_test.cc b/metrics/metrics_library_test.cc index 0a0768b21..f6cd7e7c3 100644 --- a/metrics/metrics_library_test.cc +++ b/metrics/metrics_library_test.cc @@ -29,10 +29,10 @@ ACTION_P(SetMetricsPolicy, enabled) { 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, AreMetricsEnabledCaching) { } 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, SendEnumToUMA) { } 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 @@ TEST_F(MetricsLibraryTest, SendMessageToChromeUMAEventsBadFileLocation) { // 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 @@ class CMetricsLibraryTest : public testing::Test { virtual void SetUp() { lib_ = CMetricsLibraryNew(); MetricsLibrary& ml = *reinterpret_cast(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())