From 77142b7d9aa5751c7a024e3da3995eff97d3a781 Mon Sep 17 00:00:00 2001
From: Bryan Henry <bryanhenry@google.com>
Date: Fri, 3 May 2019 03:44:02 +0000
Subject: [PATCH] [bootsvc] Support process arguments for bootsvc.next

Update bootsvc.next to support optionally specifying arguments to the
process launched by bootsvc, delimited by commas.

Note that the bootsvc tests still only run manually, though we are
investigating running them automated on CQ similar to the core-tests.
Tracked in ZX-4000.

ZX-3662 #done
ZX-4000 #comment

Test: fx --dir out/x64 build && ./zircon/scripts/run-zircon-x64 -o
out/x64.zircon -c "bootsvc.next=bin/bootsvc-tests,testargument"

Change-Id: Iecfdf22be93c8d0893ffc1b58d541a2b3ad7e97c
---
 zircon/docs/kernel_cmdline.md                 |  5 +
 zircon/scripts/run-zircon                     |  2 -
 zircon/system/core/bootsvc/TESTING            |  9 +-
 .../system/core/bootsvc/integration-test.cpp  | 97 ++++++++++++++++---
 zircon/system/core/bootsvc/main.cpp           | 22 ++++-
 zircon/system/core/bootsvc/util.cpp           | 19 +++-
 zircon/system/core/bootsvc/util.h             |  3 +
 7 files changed, 135 insertions(+), 22 deletions(-)

diff --git a/zircon/docs/kernel_cmdline.md b/zircon/docs/kernel_cmdline.md
index c80d609c628..1d15c6284ed 100644
--- a/zircon/docs/kernel_cmdline.md
+++ b/zircon/docs/kernel_cmdline.md
@@ -24,9 +24,14 @@ If this option is set, the system will not use Address Space Layout
 Randomization.
 
 ## bootsvc.next=\<bootfs path\>
+
 Controls what program is executed by bootsvc to continue the boot process.
 If this is not specified, the default next program will be used.
 
+Arguments to the program can optionally be specified using a comma separator
+between the program and individual arguments. For example,
+'bootsvc.next=bin/mybin,arg1,arg2'.
+
 ## devmgr\.epoch=\<seconds\>
 
 Sets the initial offset (from the Unix epoch, in seconds) for the UTC clock.
diff --git a/zircon/scripts/run-zircon b/zircon/scripts/run-zircon
index c3026edbb5e..948904279c4 100755
--- a/zircon/scripts/run-zircon
+++ b/zircon/scripts/run-zircon
@@ -422,8 +422,6 @@ CMDLINE+="kernel.entropy-mixin=$(head -c 32 /dev/urandom | shasum -a 256 | awk '
 # Don't 'reboot' the emulator if the kernel crashes
 CMDLINE+="kernel.halt-on-panic=true "
 
-CMDLINE="`echo "$CMDLINE" | sed 's/,/,,/g'`"
-
 # run qemu
 echo CMDLINE: $CMDLINE
 cd /
diff --git a/zircon/system/core/bootsvc/TESTING b/zircon/system/core/bootsvc/TESTING
index 7b0ab9d8cb1..9598538f4cc 100644
--- a/zircon/system/core/bootsvc/TESTING
+++ b/zircon/system/core/bootsvc/TESTING
@@ -1,7 +1,10 @@
 To run the bootsvc integration tests, boot with these boot cmdline arguments:
-`userboot=bin/bootsvc bootsvc.next=bin/bootsvc-tests`.  This will tell bootsvc
-to execute its test suite instead of the usual boot chain.  The expected output
-from a boot in this mode is a unittest suite report that says 0 failed tests.
+
+`userboot=bin/bootsvc bootsvc.next=bin/bootsvc-tests,testargument`.
+
+This will tell bootsvc to execute its test suite instead of the usual boot
+chain. The expected output from a boot in this mode is a unittest suite report
+that says 0 failed tests.
 
 To test that crashlogs are being added to bootfs, you need to add a CRASHLOG item
 to the boot zbi.  For example, to add the contents of README.md as the crashlog:
diff --git a/zircon/system/core/bootsvc/integration-test.cpp b/zircon/system/core/bootsvc/integration-test.cpp
index 24dc2dcc638..92f6ade329c 100644
--- a/zircon/system/core/bootsvc/integration-test.cpp
+++ b/zircon/system/core/bootsvc/integration-test.cpp
@@ -26,6 +26,17 @@
 
 #include "util.h"
 
+static fbl::Vector<fbl::String> arguments;
+
+int main(int argc, char** argv) {
+    // Copy arguments for later use in tests.
+    for (int i = 0; i < argc; ++i) {
+        arguments.push_back(fbl::String(argv[i]));
+    }
+
+    return unittest_run_all_tests(argc, argv) ? 0 : -1;
+}
+
 namespace {
 
 constexpr char kArgumentsPath[] = "/bootsvc/" fuchsia_boot_Arguments_Name;
@@ -44,7 +55,7 @@ bool TestLoader() {
     END_TEST;
 }
 
-// Make sure that bootsvc gave us a namespace with only /boot
+// Make sure that bootsvc gave us a namespace with only /boot and /bootsvc.
 bool TestNamespace() {
     BEGIN_TEST;
 
@@ -57,13 +68,73 @@ bool TestNamespace() {
     }
 
     ASSERT_EQ(ns->count, 2);
-    ASSERT_STR_EQ(ns->path[0], "/boot");
-    ASSERT_STR_EQ(ns->path[1], "/bootsvc");
+    EXPECT_STR_EQ(ns->path[0], "/boot");
+    EXPECT_STR_EQ(ns->path[1], "/bootsvc");
 
     free(ns);
     END_TEST;
 }
 
+// Make sure that bootsvc passed along program arguments from bootsvc.next
+// correctly.
+//
+// As documented in TESTING, this test relies on these tests being run by using
+// a boot cmdline that includes 'bootsvc.next=bin/bootsvc-tests,testargument' so
+// that we can test the parsing on bootsvc.next.
+bool TestArguments() {
+    BEGIN_TEST;
+
+    ASSERT_EQ(arguments.size(), 2);
+    EXPECT_STR_EQ(arguments[0].c_str(), "bin/bootsvc-tests");
+    EXPECT_STR_EQ(arguments[1].c_str(), "testargument");
+
+    END_TEST;
+}
+
+struct SplitStringTestCase {
+    const char* input;
+    char delimiter;
+};
+
+// Test the SplitString util individually.
+bool TestSplitString() {
+    BEGIN_TEST;
+
+    // Makeshift parametrized test.
+    struct {
+        const char* input;
+        char delimiter;
+    } cases[] = {
+        {.input = "", .delimiter = ','},
+        {.input = "abcd", .delimiter = ','},
+        {.input = "a,b c,d", .delimiter = ','},
+        {.input = "a,b c,d", .delimiter = ' '},
+        {.input = "::a:", .delimiter = ':'},
+    };
+    fbl::Vector<fbl::String> expected[] = {
+        {},
+        {"abcd"},
+        {"a", "b c", "d"},
+        {"a,b", "c,d"},
+        {"", "", "a", ""},
+    };
+    for (size_t n = 0; n < sizeof(cases) / sizeof(*cases); ++n) {
+        fbl::String case_msg = fbl::StringPrintf("Test Case %zu", n);
+        auto result = bootsvc::SplitString(cases[n].input, cases[n].delimiter);
+
+        // We use EXPECT_EQ rather than ASSERT_EQ so that we can continue with
+        // other test cases if one fails.
+        EXPECT_EQ(result.size(), expected[n].size(), case_msg.c_str());
+        if (result.size() == expected[n].size()) {
+            for (size_t i = 0; i < result.size(); ++i) {
+                EXPECT_STR_EQ(result[i].c_str(), expected[n][i].c_str(), case_msg.c_str());
+            }
+        }
+    }
+
+    END_TEST;
+}
+
 // Make sure that we can parse boot args from a configuration string
 bool TestParseBootArgs() {
     BEGIN_TEST;
@@ -92,8 +163,8 @@ key=value
     END_TEST;
 }
 
-// Make sure the Arguments service works
-bool TestArguments() {
+// Make sure the fuchsia.boot.Arguments service works
+bool TestBootArguments() {
     BEGIN_TEST;
 
     zx::channel local, remote;
@@ -122,8 +193,8 @@ bool TestArguments() {
     END_TEST;
 }
 
-// Make sure the Items service works
-bool TestItems() {
+// Make sure the fuchsia.boot.Items service works
+bool TestBootItems() {
     BEGIN_TEST;
 
     zx::channel local, remote;
@@ -166,8 +237,8 @@ bool TestItems() {
     END_TEST;
 }
 
-// Make sure the RootResource service works
-bool TestRootResource() {
+// Make sure the fuchsia.boot.RootResource service works
+bool TestBootRootResource() {
     BEGIN_TEST;
 
     zx::channel local, remote;
@@ -219,9 +290,11 @@ bool TestVdsosPresent() {
 BEGIN_TEST_CASE(bootsvc_integration_tests)
 RUN_TEST(TestLoader)
 RUN_TEST(TestNamespace)
-RUN_TEST(TestParseBootArgs)
 RUN_TEST(TestArguments)
-RUN_TEST(TestItems)
-RUN_TEST(TestRootResource)
+RUN_TEST(TestParseBootArgs)
+RUN_TEST(TestSplitString)
+RUN_TEST(TestBootArguments)
+RUN_TEST(TestBootItems)
+RUN_TEST(TestBootRootResource)
 RUN_TEST(TestVdsosPresent)
 END_TEST_CASE(bootsvc_integration_tests)
diff --git a/zircon/system/core/bootsvc/main.cpp b/zircon/system/core/bootsvc/main.cpp
index 9a976592692..e681897ebfe 100644
--- a/zircon/system/core/bootsvc/main.cpp
+++ b/zircon/system/core/bootsvc/main.cpp
@@ -3,9 +3,12 @@
 // found in the LICENSE file.
 
 #include <ctype.h>
+#include <sstream>
 #include <stdio.h>
 #include <utility>
 
+#include <fbl/string.h>
+#include <fbl/vector.h>
 #include <fuchsia/boot/c/fidl.h>
 #include <launchpad/launchpad.h>
 #include <lib/async-loop/cpp/loop.h>
@@ -109,14 +112,20 @@ struct LaunchNextProcessArgs {
 int LaunchNextProcess(void* raw_ctx) {
     fbl::unique_ptr<LaunchNextProcessArgs> args(static_cast<LaunchNextProcessArgs*>(raw_ctx));
 
-    const char* next_program = getenv("bootsvc.next");
-    if (next_program == nullptr) {
-        next_program = "bin/devcoordinator";
+    const char* bootsvc_next = getenv("bootsvc.next");
+    if (bootsvc_next == nullptr) {
+        bootsvc_next = "bin/devcoordinator";
     }
 
+    // Split the bootsvc.next value into 1 or more arguments using ',' as a
+    // delimiter.
+    printf("bootsvc: bootsvc.next = %s\n", bootsvc_next);
+    fbl::Vector<fbl::String> next_args = bootsvc::SplitString(bootsvc_next, ',');
+
     // Open the executable we will start next
     zx::vmo program;
     uint64_t file_size;
+    const char* next_program = next_args[0].c_str();
     zx_status_t status = args->bootfs->Open(next_program, &program, &file_size);
     ZX_ASSERT_MSG(status == ZX_OK, "bootsvc: failed to open '%s': %s\n", next_program,
                   zx_status_get_string(status));
@@ -146,6 +155,13 @@ int LaunchNextProcess(void* raw_ctx) {
     launchpad_add_handle(lp, svcfs_conn.release(), PA_HND(PA_NS_DIR, count));
     nametable[count++] = "/bootsvc";
 
+    int argc = static_cast<int>(next_args.size());
+    const char* argv[argc];
+    for (int i = 0; i < argc; ++i) {
+        argv[i] = next_args[i].c_str();
+    }
+    launchpad_set_args(lp, argc, argv);
+
     ZX_ASSERT(count <= fbl::count_of(nametable));
     launchpad_set_nametable(lp, count, nametable);
 
diff --git a/zircon/system/core/bootsvc/util.cpp b/zircon/system/core/bootsvc/util.cpp
index e3d4a750651..f3bfa282d76 100644
--- a/zircon/system/core/bootsvc/util.cpp
+++ b/zircon/system/core/bootsvc/util.cpp
@@ -166,8 +166,8 @@ zx_status_t CreateVnodeConnection(fs::Vfs* vfs, fbl::RefPtr<fs::Vnode> vnode, zx
 
     auto conn = std::make_unique<fs::Connection>(vfs, vnode, std::move(local),
                                                  ZX_FS_FLAG_DIRECTORY |
-                                                 ZX_FS_RIGHT_READABLE |
-                                                 ZX_FS_RIGHT_WRITABLE);
+                                                     ZX_FS_RIGHT_READABLE |
+                                                     ZX_FS_RIGHT_WRITABLE);
     status = vfs->ServeConnection(std::move(conn));
     if (status != ZX_OK) {
         return status;
@@ -176,4 +176,19 @@ zx_status_t CreateVnodeConnection(fs::Vfs* vfs, fbl::RefPtr<fs::Vnode> vnode, zx
     return ZX_OK;
 }
 
+fbl::Vector<fbl::String> SplitString(fbl::String input, char delimiter) {
+    fbl::Vector<fbl::String> result;
+
+    // No fbl::String::find, do it ourselves.
+    const char* start = input.begin();
+    for (auto end = start; end != input.end(); start = end + 1) {
+        end = start;
+        while (end != input.end() && *end != delimiter) {
+            ++end;
+        }
+        result.push_back(fbl::String(start, end - start));
+    }
+    return result;
+}
+
 } // namespace bootsvc
diff --git a/zircon/system/core/bootsvc/util.h b/zircon/system/core/bootsvc/util.h
index 91c3e04ff46..b0a70a73492 100644
--- a/zircon/system/core/bootsvc/util.h
+++ b/zircon/system/core/bootsvc/util.h
@@ -7,6 +7,7 @@
 #include <map>
 #include <string_view>
 
+#include <fbl/string.h>
 #include <fbl/vector.h>
 #include <fs/vfs.h>
 #include <lib/zx/channel.h>
@@ -49,4 +50,6 @@ zx_status_t CreateVnodeConnection(fs::Vfs* vfs, fbl::RefPtr<fs::Vnode> vnode, zx
 // Path relative to /boot used for crashlogs.
 extern const char* const kLastPanicFilePath;
 
+fbl::Vector<fbl::String> SplitString(fbl::String input, char delimiter);
+
 } // namespace bootsvc
-- 
GitLab