metrics cleanup and fixes.

- value is int now.
- add seconds to milliseconds option to metrics_client and use it
- chmod chronos/root fix
- style fixes

Review URL: http://codereview.chromium.org/1649007

metrics/metrics_client.cc

@@ -2,22 +2,16 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <errno.h>
-#include <sys/file.h>
-#include <string.h>
-#include <stdio.h>
-
+#include <cstdio>
 #include <cstdlib>
-#include <iostream>
 
 #include "metrics_library.h"
 
-using namespace std;
-
 // Usage:  metrics_client [-ab] metric_name metric_value
 int main(int argc, char** argv) {
   bool send_to_autotest = false;
   bool send_to_chrome = true;
+  bool secs_to_msecs = false;
   int metric_name_index = 1;
   int metric_value_index = 2;
   bool print_usage = false;
@@ -25,7 +19,7 @@ int main(int argc, char** argv) {
   if (argc >= 3) {
     // Parse arguments
     int flag;
-    while ((flag = getopt(argc, argv, "ab")) != -1) {
+    while ((flag = getopt(argc, argv, "abt")) != -1) {
       switch (flag) {
         case 'a':
           send_to_autotest = true;
@@ -35,6 +29,9 @@ int main(int argc, char** argv) {
           send_to_chrome = true;
           send_to_autotest = true;
           break;
+        case 't':
+          secs_to_msecs = true;
+          break;
         default:
           print_usage = true;
           break;
@@ -52,22 +49,31 @@ int main(int argc, char** argv) {
   }
 
   if (print_usage) {
-    cerr << "Usage:  metrics_client [-ab] name value" << endl;
-    cerr << endl;
-    cerr << "  default: send metric to chrome only" << endl;
-    cerr << "  -a: send metric to autotest only" << endl;
-    cerr << "  -b: send metric to both chrome and autotest" << endl;
+    fprintf(stderr,
+            "Usage:  metrics_client [-abt] name value\n"
+            "\n"
+            "  default: send metric with integer value to chrome only\n"
+            "  -a: send metric to autotest only\n"
+            "  -b: send metric to both chrome and autotest\n"
+            "  -t: convert value from float seconds to int milliseconds\n");
     return 1;
   }
 
+  const char* name = argv[metric_name_index];
+  int value;
+  if (secs_to_msecs) {
+    float secs = strtof(argv[metric_value_index], NULL);
+    value = static_cast<int>(secs * 1000.0f);
+  } else {
+    value = atoi(argv[metric_value_index]);
+  }
+
   // Send metrics
   if (send_to_autotest) {
-    MetricsLibrary::SendToAutotest(argv[metric_name_index],
-                                   argv[metric_value_index]);
+    MetricsLibrary::SendToAutotest(name, value);
   }
   if (send_to_chrome) {
-    MetricsLibrary::SendToChrome(argv[metric_name_index],
-                                 argv[metric_value_index]);
+    MetricsLibrary::SendToChrome(name, value);
   }
   return 0;
 }

metrics/metrics_daemon.cc

@@ -113,12 +113,10 @@ void MetricsDaemon::LogNetworkStateChange(const char* newstate) {
     if (diff.tv_sec >= INT_MAX / 1000) {
       diff_ms = INT_MAX;
     }
-    char buffer[100];
-    snprintf(buffer, sizeof(buffer), "%d", diff_ms);
     if (testing_) {
-      TestPublishMetric(network_states_[old_id].stat_name, buffer);
+      TestPublishMetric(network_states_[old_id].stat_name, diff_ms);
     } else {
-      ChromePublishMetric(network_states_[old_id].stat_name, buffer);
+      ChromePublishMetric(network_states_[old_id].stat_name, diff_ms);
     }
   }
   network_state_id_ = new_id;
@@ -135,10 +133,10 @@ MetricsDaemon::GetNetworkStateId(const char* state_name) {
   return static_cast<NetworkStateId>(-1);
 }
 
-void MetricsDaemon::ChromePublishMetric(const char* name, const char* value) {
+void MetricsDaemon::ChromePublishMetric(const char* name, int value) {
   MetricsLibrary::SendToChrome(name, value);
 }
 
-void MetricsDaemon::TestPublishMetric(const char* name, const char* value) {
+void MetricsDaemon::TestPublishMetric(const char* name, int value) {
   LOG(INFO) << "received metric: " << name << " " << value;
 }

metrics/metrics_daemon.h

@@ -64,10 +64,10 @@ class MetricsDaemon {
   NetworkStateId GetNetworkStateId(const char* state_name);
 
   // Sends a stat to Chrome for transport to UMA.
-  void ChromePublishMetric(const char* name, const char* value);
+  void ChromePublishMetric(const char* name, int value);
 
   // Prints a stat for testing.
-  void TestPublishMetric(const char* name, const char* value);
+  void TestPublishMetric(const char* name, int value);
 
 #if 0
   // Fetches a name-value hash table from DBus.

metrics/metrics_library.cc

@@ -13,21 +13,23 @@
 
 #include <errno.h>
 #include <sys/file.h>
-#include <string.h>
-#include <stdio.h>
+
+#include <cstdarg>
+#include <cstdio>
+#include <cstring>
 
 #define READ_WRITE_ALL_FILE_FLAGS \
   (S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH)
 
 static const char kAutotestPath[] = "/tmp/.chromeos-metrics-autotest";
 static const char kChromePath[] = "/tmp/.chromeos-metrics";
-static const int kBufferSize = 4096;
+static const int32_t kBufferSize = 1024;
 
 using namespace std;
 
 // TODO(sosa@chromium.org) - use Chromium logger instead of stderr
-void MetricsLibrary::PrintError(const char *message, const char *file,
-                               int code) {
+static void PrintError(const char *message, const char *file,
+                       int code) {
   const char *kProgramName = "metrics_library";
   if (code == 0) {
     fprintf(stderr, "%s: %s\n", kProgramName, message);
@@ -40,61 +42,105 @@ void MetricsLibrary::PrintError(const char *message, const char *file,
   }
 }
 
-void MetricsLibrary::SendToAutotest(string name, string value) {
-  FILE *autotest_file = fopen(kAutotestPath, "a+");
-  if (autotest_file == NULL) {
-    PrintError("fopen", kAutotestPath, errno);
-    return;
-  }
-
-  fprintf(autotest_file, "%s=%s\n", name.c_str(), value.c_str());
-  fclose(autotest_file);
-}
-
-void MetricsLibrary::SendToChrome(string name, string value) {
+// Sends message of size length to Chrome and returns true on success.
+static bool SendMessageToChrome(int32_t length, const char *message) {
   int chrome_fd = open(kChromePath,
                        O_WRONLY | O_APPEND | O_CREAT,
                        READ_WRITE_ALL_FILE_FLAGS);
-  // If we failed to open it, return
+  // If we failed to open it, return.
   if (chrome_fd < 0) {
     PrintError("open", kChromePath, errno);
-    return;
+    return false;
   }
 
-  // Need to chmod because open flags are anded with umask.
-  if (fchmod(chrome_fd, READ_WRITE_ALL_FILE_FLAGS) < 0) {
-    PrintError("fchmod", kChromePath, errno);
-    close(chrome_fd);
-    return;
-  }
+  // Need to chmod because open flags are anded with umask. Ignore the
+  // exit code -- a chronos process may fail chmoding because the file
+  // has been created by a root process but that should be OK.
+  fchmod(chrome_fd, READ_WRITE_ALL_FILE_FLAGS);
 
-  // Grab an exclusive lock to protect Chrome from truncating underneath us
+  // Grab an exclusive lock to protect Chrome from truncating
+  // underneath us. Keep the file locked as briefly as possible.
   if (flock(chrome_fd, LOCK_EX) < 0) {
     PrintError("flock", kChromePath, errno);
     close(chrome_fd);
-    return;
+    return false;
   }
 
-  // Message format is: LENGTH (binary), NAME, VALUE
-  char message[kBufferSize];
-  char *curr_ptr = message;
-  int32_t message_length =
-      name.length() + value.length() + 2 + sizeof(message_length);
-  if (message_length > static_cast<int32_t>(sizeof(message)))
-    PrintError("name/value too long", NULL, 0);
-
-  // Make sure buffer is blanked
-  memset(message, 0, sizeof(message));
-  memcpy(curr_ptr, &message_length, sizeof(message_length));
-  curr_ptr += sizeof(message_length);
-  strncpy(curr_ptr, name.c_str(), name.length());
-  curr_ptr += name.length() + 1;
-  strncpy(curr_ptr, value.c_str(), value.length());
-  if (write(chrome_fd, message, message_length) != message_length)
+  bool success = true;
+  if (write(chrome_fd, message, length) != length) {
     PrintError("write", kChromePath, errno);
+    success = false;
+  }
 
-  // Release the file lock and close file
-  if (flock(chrome_fd, LOCK_UN) < 0)
+  // Release the file lock and close file.
+  if (flock(chrome_fd, LOCK_UN) < 0) {
     PrintError("unlock", kChromePath, errno);
+    success = false;
+  }
   close(chrome_fd);
+  return success;
+}
+
+// Formats a name/value message for Chrome in buffer and returns the
+// length of the message or a negative value on error.
+//
+// Message format is: | LENGTH(binary) | 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).
+static int32_t 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;
+}
+
+bool MetricsLibrary::SendToAutotest(string name, int value) {
+  FILE *autotest_file = fopen(kAutotestPath, "a+");
+  if (autotest_file == NULL) {
+    PrintError("fopen", kAutotestPath, errno);
+    return false;
+  }
+
+  fprintf(autotest_file, "%s=%d\n", name.c_str(), value);
+  fclose(autotest_file);
+  return true;
+}
+
+bool MetricsLibrary::SendToChrome(string name, int value) {
+  // Format the message.
+  char message[kBufferSize];
+  int32_t message_length =
+      FormatChromeMessage(kBufferSize, message,
+                          "%s%c%d", name.c_str(), '\0', value);
+
+  if (message_length < 0)
+    return false;
+
+  // Send the message.
+  return SendMessageToChrome(message_length, message);
 }

metrics/metrics_library.h

@@ -12,7 +12,6 @@
 #ifndef METRICS_LIBRARY_H_
 #define METRICS_LIBRARY_H_
 
-#include <stdio.h>
 #include <string>
 
 // TODO(sosa@chromium.org): Add testing for send methods
@@ -20,14 +19,10 @@
 // Library used to send metrics both Autotest and Chrome
 class MetricsLibrary {
  public:
-  // Sends histogram data to Chrome.
-  static void SendToChrome(std::string name, std::string value);
-  // Sends to Autotest.
-  static void SendToAutotest(std::string name, std::string value);
-
- private:
-  // Prints message to stderr
-  static void PrintError(const char *message, const char *file, int code);
+  // Sends histogram data to Chrome and returns true on success.
+  static bool SendToChrome(std::string name, int value);
+  // Sends to Autotest and returns true on success.
+  static bool SendToAutotest(std::string name, int value);
 };
 
 #endif /* METRICS_LIBRARY_H_ */