logcat: expand -n, -r and -b

- Add property expansion to the -n/--rotate-count,
  -r/--rotate-kbytes and -b/--buffer parameters.
  e.g. -r '${logcat.rotate-count:-6}' will expand the property
  logcat.rotate-count, and if not present will default to 6
- Add gtest to confirm mid-stream expansion
- No longer support ",:;<whitespace>" for buffer tokenization,
  settling on only supporting ",".

Bug: 28120456
Change-Id: I7ebb6146f72047631536c457952fa50d0abb98ab

3 files changed, 105 insertions, 21 deletions

diff --git a/logcat/logcat.cpp b/logcat/logcat.cpp
index fe5da194c..52f49cc3d 100644
--- a/logcat/logcat.cpp
+++ b/logcat/logcat.cpp
@@ -27,6 +27,7 @@
 
 #include <android-base/file.h>
 #include <android-base/strings.h>
+#include <cutils/properties.h>
 #include <cutils/sched_policy.h>
 #include <cutils/sockets.h>
 #include <log/event_tag_map.h>
@@ -280,10 +281,10 @@ static void show_help(const char *cmd)
     fprintf(stderr, "options include:\n"
                     "  -s              Set default filter to silent. Equivalent to filterspec '*:S'\n"
                     "  -f <file>, --file=<file>               Log to file. Default is stdout\n"
-                    "  -r <kbytes>, --rotate-kbytes=<kbytes>\n"
-                    "                  Rotate log every kbytes. Requires -f option\n"
-                    "  -n <count>, --rotate-count=<count>\n"
-                    "                  Sets max number of rotated logs to <count>, default 4\n"
+                    "  -r <kbytes>, --rotate-kbytes=<kbytes>  Rotate log every kbytes. Requires -f\n"
+                    "                  option. Permits property expansion.\n"
+                    "  -n <count>, --rotate-count=<count>     Sets max number of rotated logs to\n"
+                    "                  <count>, default 4. Permits property expansion.\n"
                     "  -v <format>, --format=<format>\n"
                     "                  Sets the log print format, where <format> is:\n"
                     "                    brief color epoch long monotonic printable process raw\n"
@@ -317,6 +318,7 @@ static void show_help(const char *cmd)
                     "                  'system', 'radio', 'events', 'crash', 'default' or 'all'.\n"
                     "                  Multiple -b parameters or comma separated list of buffers are\n"
                     "                  allowed. Buffers interleaved. Default -b main,system,crash.\n"
+                    "                  Permits property expansion.\n"
                     "  -B, --binary    Output the log in binary.\n"
                     "  -S, --statistics                       Output statistics.\n"
                     "  -p, --prune     Print prune white and ~black list. Service is specified as\n"
@@ -334,7 +336,11 @@ static void show_help(const char *cmd)
                     "                  comes first. Improves efficiency of polling by providing\n"
                     "                  an about-to-wrap wakeup.\n");
 
-    fprintf(stderr,"\nfilterspecs are a series of \n"
+    fprintf(stderr,"\nProperty expansion where available, may need to be single quoted to prevent\n"
+                   "shell expansion:\n"
+                   "  ${key}          - Expand string with property value associated with key\n"
+                   "  ${key:-default} - Expand, if property key value clear, use default\n"
+                   "\nfilterspecs are a series of \n"
                    "  <tag>[:priority]\n\n"
                    "where <tag> is a log component tag (or * for all) and priority is:\n"
                    "  V    Verbose (default for <tag>)\n"
@@ -391,7 +397,7 @@ static const char *multiplier_of_size(unsigned long value)
 }
 
 /*String to unsigned int, returns -1 if it fails*/
-static bool getSizeTArg(char *ptr, size_t *val, size_t min = 0,
+static bool getSizeTArg(const char *ptr, size_t *val, size_t min = 0,
                         size_t max = SIZE_MAX)
 {
     if (!ptr) {
@@ -532,6 +538,49 @@ static log_time lastLogTime(char *outputFileName) {
     return retval;
 }
 
+// Expand multiple flat property references ${<tag>:-default} or ${tag}.
+//
+// ToDo: Do we permit nesting?
+//   ${persist.logcat.something:-${ro.logcat.something:-maybesomething}}
+// For now this will result in a syntax error for caller and is acceptable.
+//
+std::string expand(const char *str)
+{
+  std::string retval(str);
+
+  // Caller has no use for ${, } or :- as literals so no use for escape
+  // character. Result expectations are a number or a string, with validity
+  // checking for both in caller. Recursive expansion or other syntax errors
+  // will result in content caller can not obviously tolerate, error must
+  // report substring if applicable, expanded and original content (if
+  // different) so that it will be clear to user what they did wrong.
+  for (size_t pos; (pos = retval.find("${")) != std::string::npos; ) {
+    size_t epos = retval.find("}", pos + 2);
+    if (epos == std::string::npos) {
+      break; // Caller will error out, showing this unexpanded.
+    }
+    size_t def = retval.find(":-", pos + 2);
+    if (def >= epos) {
+      def = std::string::npos;
+    }
+    std::string default_value("");
+    std::string key;
+    if (def == std::string::npos) {
+      key = retval.substr(pos + 2, epos - (pos + 2));
+    } else {
+      key = retval.substr(pos + 2, def - (pos + 2));
+      default_value = retval.substr(def + 2, epos - (def + 2));
+    }
+    char value[PROPERTY_VALUE_MAX];
+    property_get(key.c_str(), value, default_value.c_str());
+    // Caller will error out, syntactically empty content at this point
+    // will not be tolerated as expected.
+    retval.replace(pos, epos - pos + 1, value);
+  }
+
+  return retval;
+}
+
 } /* namespace android */
 
 
@@ -763,23 +812,35 @@ int main(int argc, char **argv)
 
             case 'b': {
                 unsigned idMask = 0;
-                while ((optarg = strtok(optarg, ",:; \t\n\r\f")) != NULL) {
-                    if (strcmp(optarg, "default") == 0) {
+                std::string expanded = expand(optarg);
+                std::istringstream copy(expanded);
+                std::string token;
+                // wish for strtok and ",:; \t\n\r\f" for hidden flexibility
+                while (std::getline(copy, token, ',')) { // settle for ","
+                    if (token.compare("default") == 0) {
                         idMask |= (1 << LOG_ID_MAIN) |
                                   (1 << LOG_ID_SYSTEM) |
                                   (1 << LOG_ID_CRASH);
-                    } else if (strcmp(optarg, "all") == 0) {
+                    } else if (token.compare("all") == 0) {
                         idMask = (unsigned)-1;
                     } else {
-                        log_id_t log_id = android_name_to_log_id(optarg);
+                        log_id_t log_id = android_name_to_log_id(token.c_str());
                         const char *name = android_log_id_to_name(log_id);
 
-                        if (strcmp(name, optarg) != 0) {
-                            logcat_panic(true, "unknown buffer %s\n", optarg);
+                        if (token.compare(name) != 0) {
+                            bool strDifferent = expanded.compare(token);
+                            if (expanded.compare(optarg)) {
+                                expanded += " expanded from ";
+                                expanded += optarg;
+                            }
+                            if (strDifferent) {
+                                expanded = token + " within " + expanded;
+                            }
+                            logcat_panic(true, "unknown buffer -b %s\n",
+                                         expanded.c_str());
                         }
                         idMask |= (1 << log_id);
                     }
-                    optarg = NULL;
                 }
 
                 for (int i = LOG_ID_MIN; i < LOG_ID_MAX; ++i) {
@@ -834,22 +895,36 @@ int main(int argc, char **argv)
                 g_outputFileName = optarg;
             break;
 
-            case 'r':
-                if (!getSizeTArg(optarg, &g_logRotateSizeKBytes, 1)) {
-                    logcat_panic(true, "Invalid parameter %s to -r\n", optarg);
+            case 'r': {
+                std::string expanded = expand(optarg);
+                if (!getSizeTArg(expanded.c_str(), &g_logRotateSizeKBytes, 1)) {
+                    if (expanded.compare(optarg)) {
+                        expanded += " expanded from ";
+                        expanded += optarg;
+                    }
+                    logcat_panic(true, "Invalid parameter -r %s\n",
+                                 expanded.c_str());
                 }
+            }
             break;
 
-            case 'n':
-                if (!getSizeTArg(optarg, &g_maxRotatedLogs, 1)) {
-                    logcat_panic(true, "Invalid parameter %s to -n\n", optarg);
+            case 'n': {
+                std::string expanded = expand(optarg);
+                if (!getSizeTArg(expanded.c_str(), &g_maxRotatedLogs, 1)) {
+                    if (expanded.compare(optarg)) {
+                        expanded += " expanded from ";
+                        expanded += optarg;
+                    }
+                    logcat_panic(true, "Invalid parameter -n %s\n",
+                                 expanded.c_str());
                 }
+            }
             break;
 
             case 'v':
                 err = setLogFormat (optarg);
                 if (err < 0) {
-                    logcat_panic(true, "Invalid parameter %s to -v\n", optarg);
+                    logcat_panic(true, "Invalid parameter -v %s\n", optarg);
                 }
                 hasSetLogFormat |= err;
             break;
diff --git a/logcat/tests/Android.mk b/logcat/tests/Android.mk
index a28664e30..3bf8a0bf9 100644
--- a/logcat/tests/Android.mk
+++ b/logcat/tests/Android.mk
@@ -56,6 +56,6 @@ include $(CLEAR_VARS)
 LOCAL_MODULE := $(test_module_prefix)unit-tests
 LOCAL_MODULE_TAGS := $(test_tags)
 LOCAL_CFLAGS += $(test_c_flags)
-LOCAL_SHARED_LIBRARIES := liblog
+LOCAL_SHARED_LIBRARIES := liblog libcutils
 LOCAL_SRC_FILES := $(test_src_files)
 include $(BUILD_NATIVE_TEST)
diff --git a/logcat/tests/logcat_test.cpp b/logcat/tests/logcat_test.cpp
index 2c3579d71..997e68213 100644
--- a/logcat/tests/logcat_test.cpp
+++ b/logcat/tests/logcat_test.cpp
@@ -25,6 +25,7 @@
 
 #include <memory>
 
+#include <cutils/properties.h>
 #include <gtest/gtest.h>
 #include <log/log.h>
 #include <log/logger.h>
@@ -425,6 +426,14 @@ TEST(logcat, multiple_buffer) {
       "logcat -v brief -b radio,events,system,main -g 2>/dev/null"));
 }
 
+// duplicate test for get_size, but use test.logcat.buffer property
+TEST(logcat, property_expand) {
+    property_set("test.logcat.buffer", "radio,events");
+    EXPECT_EQ(4, get_groups(
+      "logcat -v brief -b 'system,${test.logcat.buffer:-bogo},main' -g 2>/dev/null"));
+    property_set("test.logcat.buffer", "");
+}
+
 TEST(logcat, bad_buffer) {
     ASSERT_EQ(0, get_groups(
       "logcat -v brief -b radio,events,bogo,system,main -g 2>/dev/null"));