[PATCH 0/4] Replace boost program options with custom light alternative functions

[Waldemar Kozaczuk]

This series of 4 patches implements simple utility functions
to help parse command line options. It also replaces boost
program options with custom alternative in kernel loader, cpiod
and httpserver.

Waldemar Kozaczuk (4):
options: implement light and simple alternative to boost program
options
loader: replace boost program_options with light options utility
functions
cpiod: reduce cpiod size by replacing boost program options with the
custom light alternative
httpserver: replace boost program_options with light options utility
functions

Makefile | 1 +
core/options.cc | 171 ++++++++++++++
include/osv/options.hh | 56 +++++
loader.cc | 178 +++++++-------
modules/httpserver-api/global_server.cc | 30 ++-
modules/httpserver-api/global_server.hh | 9 +-
modules/httpserver-api/main.cc | 52 +++--
modules/httpserver-api/request_handler.cc | 7 +-
modules/httpserver-api/request_handler.hh | 5 +-
modules/httpserver-api/routes.hh | 1 -
modules/httpserver-api/server.cc | 34 ++-
modules/httpserver-api/server.hh | 3 +-
modules/tests/Makefile | 2 +-
tests/tst-options.cc | 269 ++++++++++++++++++++++
tools/cpiod/cpiod.cc | 69 ++++--
15 files changed, 722 insertions(+), 165 deletions(-)
create mode 100644 core/options.cc
create mode 100644 include/osv/options.hh
create mode 100644 tests/tst-options.cc

--
2.20.1

[Waldemar Kozaczuk]

This patch provides simple and light alternative to boost program options
library. It adds handful of utility functions that are aimed
to help parse command line options and intended to be used
by kernel loader and the utility apps like cpiod, httpserver
and cloud init.

For more details on what particular functions do
look at the comments in options.hh and options.cc.

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
---
Makefile | 1 +
core/options.cc | 171 ++++++++++++++++++++++++++
include/osv/options.hh | 56 +++++++++
modules/tests/Makefile | 2 +-
tests/tst-options.cc | 269 +++++++++++++++++++++++++++++++++++++++++
5 files changed, 498 insertions(+), 1 deletion(-)

create mode 100644 core/options.cc
create mode 100644 include/osv/options.hh
create mode 100644 tests/tst-options.cc

diff --git a/Makefile b/Makefile
index b421f4bf..348e3747 100644
--- a/Makefile
+++ b/Makefile
@@ -955,6 +955,7 @@ objects += core/app.o
objects += core/libaio.o
objects += core/osv_execve.o
objects += core/osv_c_wrappers.o
+objects += core/options.o

#include $(src)/libc/build.mk:
libc =
diff --git a/core/options.cc b/core/options.cc
new file mode 100644
index 00000000..47ea5f96
--- /dev/null
+++ b/core/options.cc
@@ -0,0 +1,171 @@
+/*
+ * Copyright (C) 2019 Waldemar Kozaczuk
+ *
+ * This work is open source software, licensed under the terms of the
+ * BSD license as described in the LICENSE file in the top-level directory.
+ */
+
+#include <iostream>
+#include <functional>
+#include <cassert>
+#include <osv/options.hh>
+
+namespace options {
+using namespace std;
+//
+// Expects argv to contain individual arguments that are in one of the three forms:
+// 1) '--key' or
+// 2) '--key=value' or
+// 3) 'value'
+//
+// If 'allow_separate_values' is false, then only first 2 forms are valid and
+// the '--key' arguments are identified as 'flag' options (like '--enabled')
+// and '--key=value' arguments are treated as key, value pairs
+//
+// If 'allow_separate_values' is true, then all 3 forms are valid and
+// the '--key' arguments NOT followed by 'value' are identified as 'flag' options (like '--enabled')
+// and '--key=value' arguments are treated as key, value pairs
+// and '--key value' arguments are treated as key, value pairs as well
+map<string,vector<string>> parse_options_values(int argc, char** argv,
+ function<void (const string&)> error_handler,
+ bool allow_separate_values)
+{
+ map<string,vector<string>> options_values;
+
+ string key("");
+ for (int i = 0; i < argc; i++) {
+ string arg(argv[i]);
+ //
+ // Verify if is a 'long' option (starts with '--')
+ if (arg.find("--") != 0) {
+ // Not an option
+ if (allow_separate_values && !key.empty()) {
+ // Treat this arg as a value of the option specified by last key (for example: '--count 5')
+ options_values[key].push_back(arg);
+ key = "";
+ continue;
+ }
+ else {
+ // Separate option values (like '--count 5') are not allowed
+ error_handler(string("not an option: ") + arg);
+ return options_values;
+ }
+ }
+ //
+ // Parse out value if --key=value
+ size_t pos = arg.find('=');
+ if (string::npos == pos) {
+ // Treat as a 'flag' option like for example '--enableXyz'
+ key = arg.substr(2);
+ // Verify if a non-flag option (non-empty vector) DOES NOT exists already
+ auto it = options_values.find(key);
+ if (it != options_values.end() && !it->second.empty()) {
+ error_handler(string("duplicate option: ") + arg);
+ return options_values;
+ }
+ // Flag: add empty vector to the map
+ options_values[key] = vector<string>();
+ }
+ else {
+ // Treat as an option value like for example '--count=5' (single value)
+ // or multi-value like '--env=A --env=B=1'
+ key = arg.substr(2, pos - 2);
+ // Verify if a flag option (empty vector) DOES NOT exists already
+ auto it = options_values.find(key);
+ if (it != options_values.end() && it->second.empty()) {
+ error_handler(string("duplicate option: ") + arg);
+ return options_values;
+ }
+
+ auto value = arg.substr(pos + 1);
+ if (value.empty()) {
+ error_handler(string("the required argument for option '--") + key + "' is missing");
+ return options_values;
+ }
+ // (Key,Value) or (Key,[Val1,Val2,..]) - add value to the vector
+ options_values[key].push_back(value);
+ key = "";
+ }
+ }
+
+ return options_values;
+}
+
+bool extract_option_flag(map<string,vector<string>> &options_values, const string &name, function<void (const string&)> error_handler)
+{
+ auto it = options_values.find(name);
+ if (it != options_values.end()) {
+ if (!it->second.empty()) {
+ error_handler(string("option '--") + name + "' does not take any arguments");
+ return false;
+ }
+
+ options_values.erase(it);
+ return true;
+ } else {
+ return false;
+ }
+}
+
+bool option_value_exists(const map<string,vector<string>> &options_values, const string &name)
+{
+ auto it = options_values.find(name);
+ return it != options_values.end() && !it->second.empty();
+}
+
+string extract_option_value(map<string,vector<string>> &options_values, const string &name)
+{
+ return extract_option_values(options_values, name)[0];
+}
+
+static void handle_invalid_argument(const string &name, const string &value, function<void (const string&)> error_handler)
+{
+ error_handler(string("the argument ('") + value + "') for option '--" + name + "' is invalid");
+}
+
+int extract_option_int_value(map<string,vector<string>> &options_values, const string &name, function<void (const string&)> error_handler)
+{
+ auto value_str = extract_option_values(options_values, name)[0];
+ size_t pos;
+ int value = 0;
+
+ try {
+ value = std::stoi(value_str, &pos);
+ if (pos < value_str.length()) {
+ handle_invalid_argument(name, value_str, error_handler);
+ }
+ }
+ catch (const invalid_argument& ia) {
+ handle_invalid_argument(name, value_str, error_handler);
+ }
+ return value;
+}
+
+float extract_option_float_value(map<string,vector<string>> &options_values, const string &name, function<void (const string&)> error_handler)
+{
+ auto value_str = extract_option_values(options_values, name)[0];
+ size_t pos;
+ float value = 0.0f;
+
+ try {
+ value = std::stof(value_str, &pos);
+ if (pos < value_str.length()) {
+ handle_invalid_argument(name, value_str, error_handler);
+ }
+ }
+ catch (const invalid_argument& ia) {
+ handle_invalid_argument(name, value_str, error_handler);
+ }
+ return value;
+}
+
+vector<string> extract_option_values(map<string,vector<string>> &options_values, const string &name)
+{
+ auto it = options_values.find(name);
+ assert(it != options_values.end() && !it->second.empty());
+ auto values = it->second;
+ options_values.erase(it);
+ return values;
+}
+
+};
diff --git a/include/osv/options.hh b/include/osv/options.hh
new file mode 100644
index 00000000..f352b39d
--- /dev/null
+++ b/include/osv/options.hh
@@ -0,0 +1,56 @@
+/*
+ * Copyright (C) 2019 Waldemar Kozaczuk
+ *
+ * This work is open source software, licensed under the terms of the
+ * BSD license as described in the LICENSE file in the top-level directory.
+ */
+
+#ifndef OSV_OPTIONS_HH
+#define OSV_OPTIONS_HH
+
+#include <string>
+#include <vector>
+#include <map>
+
+//
+// The methods in 'options' namespace provide basic functionality intended to help
+// parse options by the kernel loader and utility apps like cpiod, httpserver and cloud-init.
+namespace options {
+
+using namespace std;
+
+// Iterates over supplied array of arguments and collects them into a map
+// where key is an option name and the value is a vector of 0 or more values
+// It recognizes only so called 'long' options that start with '--' (double dash) prefix
+map<string,vector<string>> parse_options_values(int argc, char** argv,
+ function<void (const string&)> error_handler, bool allow_separate_values = true);
+
+// Checks if options_values map contains a 'flag' option of '--<flag>' format
+// and returns true if the option found and removes that option from the map
+bool extract_option_flag(map<string,vector<string>> &options_values, const string &name,
+ function<void (const string&)> error_handler);
+
+// Returns true if options_values contains single-value option (--<key>=<value>) or
+// multi-value one (--<key>=<val1>, --<key>=<val2>)
+bool option_value_exists(const map<string,vector<string>> &options_values, const string &name);
+
+// Returns the value of a single-value option (--<name>=<key>) and removes it from the options_values
+string extract_option_value(map<string,vector<string>> &options_values, const string &name);
+
+// Returns the value of a single-value option (--<name>=<key>), tries to convert to an integer
+// and removes it from the options_values
+int extract_option_int_value(map<string,vector<string>> &options_values, const string &name,
+ function<void (const string&)> error_handler);
+
+// Returns the value of a single-value option (--<name>=<key>), tries to convert to a float
+// and removes it from the options_values
+float extract_option_float_value(map<string,vector<string>> &options_values, const string &name,
+ function<void (const string&)> error_handler);
+
+// Returns the values of a multi-value option (--<key>=<val1>, --<key>=<val2>) and removes
+// them from the options_values
+vector<string> extract_option_values(map<string,vector<string>> &options_values, const string &name);
+
+};
+
+#endif //OSV_OPTIONS_HH
diff --git a/modules/tests/Makefile b/modules/tests/Makefile
index f8283a39..647dfaa4 100644
--- a/modules/tests/Makefile
+++ b/modules/tests/Makefile
@@ -109,7 +109,7 @@ tests := tst-pthread.so misc-ramdisk.so tst-vblk.so tst-bsd-evh.so \
tst-dns-resolver.so tst-kill.so tst-truncate.so \
misc-panic.so tst-utimes.so tst-utimensat.so tst-futimesat.so \
misc-tcp.so tst-strerror_r.so misc-random.so misc-urandom.so \
- tst-commands.so tst-threadcomplete.so tst-timerfd.so \
+ tst-commands.so tst-options.so tst-threadcomplete.so tst-timerfd.so \
tst-nway-merger.so tst-memmove.so tst-pthread-clock.so misc-procfs.so \
tst-chdir.so tst-chmod.so tst-hello.so misc-concurrent-io.so \
tst-concurrent-init.so tst-ring-spsc-wraparound.so tst-shm.so \
diff --git a/tests/tst-options.cc b/tests/tst-options.cc
new file mode 100644
index 00000000..7bc38fdd
--- /dev/null
+++ b/tests/tst-options.cc
@@ -0,0 +1,269 @@
+/* OSv command line options parsing tests
+ *
+ * Copyright (C) 2019 Waldemar Kozaczuk
+ *
+ * This work is open source software, licensed under the terms of the
+ * BSD license as described in the LICENSE file in the top-level directory.
+ */
+
+#include <cstdio>
+
+#include <fstream>
+#include <map>
+#include <string.h>
+#include <iostream>
+#include <functional>
+#include <osv/options.hh>
+#include <cassert>
+
+static int tests = 0, fails = 0;
+
+using namespace std;
+using namespace options;
+
+static void report(bool ok, const char* msg)
+{
+ ++tests;
+ fails += !ok;
+ printf("%s: %s\n", (ok ? "PASS" : "FAIL"), msg);
+}
+
+static void handle_parse_error(const string &message)
+{
+ cout << message << endl;
+ abort();
+}
+
+static bool test_parse_empty()
+{
+ vector<const char*> argv = {};
+ auto options = parse_options_values(argv.size(), (char**)argv.data(), handle_parse_error, false);
+ return options.empty();
+}
+
+static bool test_parse_non_option()
+{
+ vector<const char*> argv = {"--verbose", "olo"};
+
+ bool non_option_detected = false;
+ parse_options_values(argv.size(), (char**)argv.data(),[&non_option_detected](const std::string &message) {
+ assert(message == "not an option: olo");
+ non_option_detected = true;
+ }, false);
+
+ return non_option_detected;
+}
+
+static bool test_parse_single_option_flag()
+{
+ vector<const char*> argv = {"--verbose"};
+
+ auto options = parse_options_values(argv.size(), (char**)argv.data(), handle_parse_error, false);
+ assert(options.size() == 1);
+
+ assert(!options::extract_option_flag(options, "bogus", handle_parse_error));
+ assert(options::extract_option_flag(options, "verbose", handle_parse_error));
+
+ return options.empty();
+}
+
+static bool test_parse_and_extract_non_flag_option()
+{
+ vector<const char*> argv = {"--enabled=1"};
+
+ auto options = parse_options_values(argv.size(), (char**)argv.data(), handle_parse_error, false);
+ assert(options.size() == 1);
+ assert(options::option_value_exists(options, "enabled"));
+
+ bool non_flag_detected = false;
+ options::extract_option_flag(options, "enabled", [&non_flag_detected](const std::string &message) {
+ assert(message == "option '--enabled' does not take any arguments");
+ non_flag_detected = true;
+ });
+
+ return non_flag_detected;
+}
+
+static bool test_parse_single_option_value()
+{
+ vector<const char*> argv = {"--console=bla"};
+
+ auto options = parse_options_values(argv.size(), (char**)argv.data(), handle_parse_error, false);
+ assert(options.size() == 1);
+
+ assert(options::option_value_exists(options, "console"));
+ assert(options::extract_option_value(options, "console") == "bla");
+
+ return options.empty();
+}
+
+static bool test_parse_option_with_missing_value()
+{
+ vector<const char*> argv = {"--console="};
+
+ bool missing_detected = false;
+ parse_options_values(argv.size(), (char**)argv.data(), [&missing_detected](const std::string &message) {
+ assert(message == "the required argument for option '--console' is missing");
+ missing_detected = true;
+ }, false);
+
+ return missing_detected;
+}
+
+static bool test_parse_single_option_multiple_values()
+{
+ vector<const char*> argv = {"--env=bla1", "--env=A=bla2"};
+
+ auto options = parse_options_values(argv.size(), (char**)argv.data(), handle_parse_error, false);
+ assert(options.size() == 1);
+ assert(options["env"].size() == 2);
+
+ assert(options::option_value_exists(options, "env"));
+ auto values = options::extract_option_values(options, "env");
+ assert(values.size() == 2);
+ assert(values[0] == "bla1");
+ assert(values[1] == "A=bla2");
+
+ return options.empty();
+}
+
+static bool test_parse_multiple_options()
+{
+ vector<const char*> argv = {"--env=bla1", "--console=bla", "--env=A=bla2", "--verbose"};
+
+ auto options = parse_options_values(argv.size(), (char**)argv.data(), handle_parse_error, false);
+ assert(options.size() == 3);
+ assert(options["env"].size() == 2);
+
+ assert(options::extract_option_flag(options, "verbose", handle_parse_error));
+
+ assert(options::option_value_exists(options, "env"));
+ auto values = options::extract_option_values(options, "env");
+ assert(values.size() == 2);
+ assert(values[0] == "bla1");
+ assert(values[1] == "A=bla2");
+
+ assert(options::option_value_exists(options, "console"));
+ assert(options::extract_option_value(options, "console") == "bla");
+
+ return options.empty();
+}
+
+static bool test_parse_option_flag_conflict()
+{
+ vector<const char*> argv = {"--verbose", "--verbose=bla"};
+
+ bool conflict_detected = false;
+ parse_options_values(argv.size(), (char**)argv.data(),[&conflict_detected](const std::string &message) {
+ assert(message == "duplicate option: --verbose=bla");
+ conflict_detected = true;
+ }, false);
+
+ return conflict_detected;
+}
+
+static bool test_parse_option_flag_conflict2()
+{
+ vector<const char*> argv = {"--verbose=bla", "--verbose" };
+
+ bool conflict_detected = false;
+ parse_options_values(argv.size(), (char**)argv.data(),[&conflict_detected](const std::string &message) {
+ assert(message == "duplicate option: --verbose");
+ conflict_detected = true;
+ }, false);
+
+ return conflict_detected;
+}
+
+static bool test_parse_single_int_option_value()
+{
+ vector<const char*> argv = {"--delay=15"};
+
+ auto options = parse_options_values(argv.size(), (char**)argv.data(), handle_parse_error, false);
+ assert(options.size() == 1);
+
+ assert(options::option_value_exists(options, "delay"));
+ assert(options::extract_option_int_value(options, "delay", handle_parse_error) == 15);
+
+ return options.empty();
+}
+
+static bool test_parse_invalid_int_option_value()
+{
+ vector<const char*> argv = {"--delay=ola"};
+
+ auto options = parse_options_values(argv.size(), (char**)argv.data(), handle_parse_error, false);
+ assert(options.size() == 1);
+
+ bool invalid_int_detected = false;
+ options::extract_option_int_value(options, "delay", [&invalid_int_detected](const std::string &message) {
+ assert(message == "the argument ('ola') for option '--delay' is invalid");
+ invalid_int_detected = true;
+ });
+
+ return invalid_int_detected;
+}
+
+static bool test_parse_single_float_option_value()
+{
+ vector<const char*> argv = {"--var=1.05"};
+
+ auto options = parse_options_values(argv.size(), (char**)argv.data(), handle_parse_error, false);
+ assert(options.size() == 1);
+
+ assert(options::option_value_exists(options, "var"));
+ assert(options::extract_option_float_value(options, "var", handle_parse_error) == 1.05f);
+
+ return options.empty();
+}
+
+static bool test_parse_multiple_options_with_separate_value()
+{
+ vector<const char*> argv =
+ {"--log", "debug", "--env=bla1", "--console=bla", "--env=A=bla2", "--verbose", "--on", "--count", "1"};
+
+ auto options = parse_options_values(argv.size(), (char**)argv.data(), handle_parse_error, true);
+ assert(options.size() == 6);
+ assert(options["env"].size() == 2);
+
+ assert(options::extract_option_flag(options, "verbose", handle_parse_error));
+ assert(options::extract_option_flag(options, "on", handle_parse_error));
+
+ assert(options::option_value_exists(options, "env"));
+ auto values = options::extract_option_values(options, "env");
+ assert(values.size() == 2);
+ assert(values[0] == "bla1");
+ assert(values[1] == "A=bla2");
+
+ assert(options::option_value_exists(options, "log"));
+ assert(options::extract_option_value(options, "log") == "debug");
+
+ assert(options::option_value_exists(options, "count"));
+ assert(options::extract_option_int_value(options, "count", handle_parse_error) == 1);
+
+ assert(options::option_value_exists(options, "console"));
+ assert(options::extract_option_value(options, "console") == "bla");
+
+ return options.empty();
+}
+
+int main(int argc, char *argv[])
+{
+ report(test_parse_empty(), "empty string");
+ report(test_parse_non_option(), "non option");
+ report(test_parse_single_option_flag(), "single option flag");
+ report(test_parse_and_extract_non_flag_option(), "non-flag option with value");
+ report(test_parse_single_option_value(), "single option value");
+ report(test_parse_option_with_missing_value(), "single option with missing value");
+ report(test_parse_single_option_multiple_values(), "single option multiple values");
+ report(test_parse_multiple_options(), "multiple options");
+ report(test_parse_option_flag_conflict(), "option flag conflict");
+ report(test_parse_option_flag_conflict2(), "2nd option flag conflict");
+ report(test_parse_single_int_option_value(), "single int option value");
+ report(test_parse_single_float_option_value(), "single float option value");
+ report(test_parse_multiple_options_with_separate_value(), "multiple options with separated values");
+ report(test_parse_invalid_int_option_value(), "invalid int option");
+
+ printf("SUMMARY: %d tests, %d failures\n", tests, fails);
+ return 0;
+}
--
2.20.1

[Waldemar Kozaczuk]

This patch changes loader.cc to use light options utility
instead of boost program options.

This patch partially addresses #1014 and together with one
of the previous patches sent to update core/commands.cc
to remove boost program options, fully addresses #1014.

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
---

loader.cc | 178 +++++++++++++++++++++++++++++-------------------------
1 file changed, 95 insertions(+), 83 deletions(-)

diff --git a/loader.cc b/loader.cc
index a68451d3..324d30c1 100644
--- a/loader.cc
+++ b/loader.cc
@@ -9,7 +9,6 @@
#include <bsd/init.hh>
#include <bsd/net.hh>
#include <boost/format.hpp>
-#include <boost/program_options.hpp>
#include <boost/algorithm/string.hpp>
#include <cctype>
#include <osv/elf.hh>
@@ -45,6 +44,7 @@
#include <osv/app.hh>
#include <osv/firmware.hh>
#include <osv/xen.hh>
+#include <osv/options.hh>
#include <dirent.h>
#include <iostream>
#include <fstream>
@@ -150,100 +150,98 @@ bool opt_pci_disabled = false;
static int sampler_frequency;
static bool opt_enable_sampler = false;

-void parse_options(int loader_argc, char** loader_argv)
+static void usage()
{
- namespace bpo = boost::program_options;
- namespace bpos = boost::program_options::command_line_style;
-
- std::vector<const char*> args = { "osv" };
- std::copy(loader_argv, loader_argv + loader_argc, std::back_inserter(args));
-
- bpo::options_description desc("OSv options");
- desc.add_options()
- ("help", "show help text")
- ("sampler", bpo::value<int>(), "start stack sampling profiler")
- ("trace", bpo::value<std::vector<std::string>>(), "tracepoints to enable")
- ("trace-backtrace", "log backtraces in the tracepoint log")
- ("leak", "start leak detector after boot")
- ("nomount", "don't mount the ZFS file system")
- ("nopivot", "do not pivot the root from bootfs to the ZFS")
- ("assign-net", "assign virtio network to the application")
- ("maxnic", bpo::value<int>(), "maximum NIC number")
- ("norandom", "don't initialize any random device")
- ("noshutdown", "continue running after main() returns")
- ("power-off-on-abort", "use poweroff instead of halt if it's aborted")
- ("noinit", "don't run commands from /init")
- ("verbose", "be verbose, print debug messages")
- ("console", bpo::value<std::vector<std::string>>(), "select console driver")
- ("env", bpo::value<std::vector<std::string>>(), "set Unix-like environment variable (putenv())")
- ("cwd", bpo::value<std::vector<std::string>>(), "set current working directory")
- ("bootchart", "perform a test boot measuring a time distribution of the various operations\n")
- ("ip", bpo::value<std::vector<std::string>>(), "set static IP on NIC")
- ("defaultgw", bpo::value<std::string>(), "set default gateway address")
- ("nameserver", bpo::value<std::string>(), "set nameserver address")
- ("delay", bpo::value<float>()->default_value(0), "delay in seconds before boot")
- ("redirect", bpo::value<std::string>(), "redirect stdout and stderr to file")
- ("disable_rofs_cache", "disable ROFS memory cache")
- ("nopci", "disable PCI enumeration")
- ;
- bpo::variables_map vars;
- // don't allow --foo bar (require --foo=bar) so we can find the first non-option
- // argument
- int style = bpos::unix_style & ~(bpos::long_allow_next | bpos::short_allow_next);
- try {
- bpo::store(bpo::parse_command_line(args.size(), args.data(), desc, style), vars);
- } catch(std::exception &e) {
- std::cout << e.what() << '\n';
- std::cout << desc << '\n';
- osv::poweroff();
- }
- bpo::notify(vars);
+ std::cout << "OSv options:\n";
+ std::cout << " --help show help text\n";
+ std::cout << " --sampler=arg start stack sampling profiler\n";
+ std::cout << " --trace=arg tracepoints to enable\n";
+ std::cout << " --trace-backtrace log backtraces in the tracepoint log\n";
+ std::cout << " --leak start leak detector after boot\n";
+ std::cout << " --nomount don't mount the ZFS file system\n";
+ std::cout << " --nopivot do not pivot the root from bootfs to the ZFS\n";
+ std::cout << " --assign-net assign virtio network to the application\n";
+ std::cout << " --maxnic=arg maximum NIC number\n";
+ std::cout << " --norandom don't initialize any random device\n";
+ std::cout << " --noshutdown continue running after main() returns\n";
+ std::cout << " --power-off-on-abort use poweroff instead of halt if it's aborted\n";
+ std::cout << " --noinit don't run commands from /init\n";
+ std::cout << " --verbose be verbose, print debug messages\n";
+ std::cout << " --console=arg select console driver\n";
+ std::cout << " --env=arg set Unix-like environment variable (putenv())\n";
+ std::cout << " --cwd=arg set current working directory\n";
+ std::cout << " --bootchart perform a test boot measuring a time distribution of\n";
+ std::cout << " the various operations\n\n";
+ std::cout << " --ip=arg set static IP on NIC\n";
+ std::cout << " --defaultgw=arg set default gateway address\n";
+ std::cout << " --nameserver=arg set nameserver address\n";
+ std::cout << " --delay=arg (=0) delay in seconds before boot\n";
+ std::cout << " --redirect=arg redirect stdout and stderr to file\n";
+ std::cout << " --disable_rofs_cache disable ROFS memory cache\n";
+ std::cout << " --nopci disable PCI enumeration\n\n";
+}
+
+static void handle_parse_error(const std::string &message)
+{
+ std::cout << message << std::endl;
+ usage();
+ osv::poweroff();
+}

- if (vars.count("help")) {
- std::cout << desc << "\n";
+static bool extract_option_flag(std::map<std::string,std::vector<std::string>> &options_values, const std::string &name)
+{
+ return options::extract_option_flag(options_values, name, handle_parse_error);
+}
+
+static void parse_options(int loader_argc, char** loader_argv)
+{
+ auto options_values = options::parse_options_values(loader_argc, loader_argv, handle_parse_error, false);
+
+ if (extract_option_flag(options_values, "help")) {
+ usage();
}

- if (vars.count("leak")) {
+ if (extract_option_flag(options_values, "leak")) {
opt_leak = true;
}

- if (vars.count("disable_rofs_cache")) {
+ if (extract_option_flag(options_values, "disable_rofs_cache")) {
opt_disable_rofs_cache = true;
}

- if (vars.count("noshutdown")) {
+ if (extract_option_flag(options_values, "noshutdown")) {
opt_noshutdown = true;
}

- if (vars.count("power-off-on-abort")) {
+ if (extract_option_flag(options_values, "power-off-on-abort")) {
opt_power_off_on_abort = true;
}

- if (vars.count("maxnic")) {
+ if (options::option_value_exists(options_values, "maxnic")) {
opt_maxnic = true;
- maxnic = vars["maxnic"].as<int>();
+ maxnic = options::extract_option_int_value(options_values, "maxnic", handle_parse_error);
}

- if (vars.count("trace-backtrace")) {
+ if (extract_option_flag(options_values, "trace-backtrace")) {
opt_log_backtrace = true;
}

- if (vars.count("verbose")) {
+ if (extract_option_flag(options_values, "verbose")) {
opt_verbose = true;
enable_verbose();
}

- if (vars.count("sampler")) {
- sampler_frequency = vars["sampler"].as<int>();
+ if (options::option_value_exists(options_values, "sampler")) {
+ sampler_frequency = options::extract_option_int_value(options_values, "sampler", handle_parse_error);
opt_enable_sampler = true;
}

- if (vars.count("bootchart")) {
+ if (extract_option_flag(options_values, "bootchart")) {
opt_bootchart = true;
}

- if (vars.count("trace")) {
- auto tv = vars["trace"].as<std::vector<std::string>>();
+ if (options::option_value_exists(options_values, "trace")) {
+ auto tv = options::extract_option_values(options_values, "trace");
for (auto t : tv) {
std::vector<std::string> tmp;
boost::split(tmp, t, boost::is_any_of(" ,"), boost::token_compress_on);
@@ -252,13 +250,14 @@ void parse_options(int loader_argc, char** loader_argv)
}
}
}
- opt_mount = !vars.count("nomount");
- opt_pivot = !vars.count("nopivot");
- opt_random = !vars.count("norandom");
- opt_init = !vars.count("noinit");

- if (vars.count("console")) {
- auto v = vars["console"].as<std::vector<std::string>>();
+ opt_mount = !extract_option_flag(options_values, "nomount");
+ opt_pivot = !extract_option_flag(options_values, "nopivot");
+ opt_random = !extract_option_flag(options_values, "norandom");
+ opt_init = !extract_option_flag(options_values, "noinit");
+
+ if (options::option_value_exists(options_values, "console")) {
+ auto v = options::extract_option_values(options_values, "console");
if (v.size() > 1) {
printf("Ignoring '--console' options after the first.");
}
@@ -266,42 +265,55 @@ void parse_options(int loader_argc, char** loader_argv)
debug("console=%s\n", opt_console);
}

- if (vars.count("env")) {
- for (auto t : vars["env"].as<std::vector<std::string>>()) {
+ if (options::option_value_exists(options_values, "env")) {
+ for (auto t : options::extract_option_values(options_values, "env")) {
debug("Setting in environment: %s\n", t);
putenv(strdup(t.c_str()));
}
}

- if (vars.count("cwd")) {
- auto v = vars["cwd"].as<std::vector<std::string>>();
+ if (options::option_value_exists(options_values, "cwd")) {
+ auto v = options::extract_option_values(options_values, "cwd");
if (v.size() > 1) {
printf("Ignoring '--cwd' options after the first.");
}
opt_chdir = v.front();
}

- if (vars.count("ip")) {
- opt_ip = vars["ip"].as<std::vector<std::string>>();
+ if (options::option_value_exists(options_values, "ip")) {
+ opt_ip = options::extract_option_values(options_values, "ip");
}

- if (vars.count("defaultgw")) {
- opt_defaultgw = vars["defaultgw"].as<std::string>();
+ if (options::option_value_exists(options_values, "defaultgw")) {
+ opt_defaultgw = options::extract_option_value(options_values, "defaultgw");
}

- if (vars.count("nameserver")) {
- opt_nameserver = vars["nameserver"].as<std::string>();
+ if (options::option_value_exists(options_values, "nameserver")) {
+ opt_nameserver = options::extract_option_value(options_values, "nameserver");
}

- if (vars.count("redirect")) {
- opt_redirect = vars["redirect"].as<std::string>();
+ if (options::option_value_exists(options_values, "redirect")) {
+ opt_redirect = options::extract_option_value(options_values, "redirect");
}

- boot_delay = std::chrono::duration_cast<std::chrono::nanoseconds>(1_s * vars["delay"].as<float>());
+ if (options::option_value_exists(options_values, "delay")) {
+ boot_delay = std::chrono::duration_cast<std::chrono::nanoseconds>(1_s * options::extract_option_float_value(options_values, "delay", handle_parse_error));
+ } else {
+ boot_delay = std::chrono::duration_cast<std::chrono::nanoseconds>(1_s * 0.0f);
+ }

- if (vars.count("nopci")) {
+ if (extract_option_flag(options_values, "nopci")) {
opt_pci_disabled = true;
}
+
+ if (!options_values.empty()) {
+ for (auto other_option : options_values) {
+ std::cout << "unrecognized option: " << other_option.first << std::endl;
+ }
+
+ usage();
+ osv::poweroff();
+ }
}

// return the std::string and the commands_args poiting to them as a move
--
2.20.1

[Waldemar Kozaczuk]

Fixes #980

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
---

tools/cpiod/cpiod.cc | 69 +++++++++++++++++++++++++++++---------------
1 file changed, 46 insertions(+), 23 deletions(-)

diff --git a/tools/cpiod/cpiod.cc b/tools/cpiod/cpiod.cc
index 5305b94c..59593625 100644
--- a/tools/cpiod/cpiod.cc
+++ b/tools/cpiod/cpiod.cc
@@ -11,17 +11,16 @@
#include <string>
#include <system_error>
#include <boost/asio.hpp>
-#include <boost/program_options.hpp>
#include "cpio.hh"
#include <sys/mount.h>
#include <errno.h>
#include <stdio.h>
#include <sys/stat.h>
+#include <osv/options.hh>

using namespace osv;
using namespace std;
using namespace boost::asio::ip;
-namespace po = boost::program_options;

// Want to use boost::filesystem, but too much effort to integrate
extern "C" { int mkdirp(const char *d, mode_t mode); }
@@ -127,31 +126,55 @@ private:
}
};

+static void usage()
+{
+ std::cout << "Allowed options:\n";
+ std::cout << " --help produce help message\n";
+ std::cout << " --port arg (=10000) set listening port\n";
+ std::cout << " --verbose arg (=1) disable verbose output\n";
+ std::cout << " --prefix arg (=/) set prefix\n\n";

+}
+
+static void handle_parse_error(const std::string &message)
+{
+ std::cout << message << std::endl;
+ usage();

+ exit(1);
+}
+
int main(int ac, char** av)
{
- int port;
- std::string prefix;
- bool verbose;
- // Declare the supported options.
- po::options_description desc("Allowed options");
- desc.add_options()
- ("help", "produce help message")
- ("port", po::value<int>()->default_value(10000), "set listening port")
- ("verbose", po::value<bool>()->default_value(true), "disable verbose output")
- ("prefix", po::value<std::string>()->default_value(std::string("/")), "set prefix");
-
- po::variables_map vm;
- po::store(po::parse_command_line(ac, av, desc), vm);
- po::notify(vm);
-
- if (vm.count("help")) {
- cout << desc << "\n";
- return 1;
+ int port = 10000;
+ std::string prefix("/");
+ bool verbose = true;
+
+ auto options_values = options::parse_options_values(ac - 1, av + 1, handle_parse_error);
+
+ if (options::extract_option_flag(options_values, "help", handle_parse_error)) {
+ usage();
+ return 1;
}

- port = vm["port"].as<int>();
- prefix = vm["prefix"].as<std::string>();
- verbose = vm["verbose"].as<bool>();
+ if (options::option_value_exists(options_values, "port")) {
+ port = options::extract_option_int_value(options_values, "port", handle_parse_error);
+ }
+
+ if (options::option_value_exists(options_values, "prefix")) {
+ prefix = options::extract_option_value(options_values, "prefix");
+ }
+
+ if (options::option_value_exists(options_values, "verbose")) {
+ verbose = options::extract_option_int_value(options_values, "verbose", handle_parse_error) != 0;
+ }

+
+ if (!options_values.empty()) {
+ for (auto other_option : options_values) {

+ std::cout << "Unrecognized option: " << other_option.first << std::endl;
+ }
+
+ usage();
+ return 1;
+ }

boost::asio::io_service io_service;
tcp::acceptor acceptor(io_service, tcp::endpoint(tcp::v4(), port));
--
2.20.1

[Waldemar Kozaczuk]

Fixes #1053

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
---

modules/httpserver-api/global_server.cc | 30 ++++++++-----
modules/httpserver-api/global_server.hh | 9 ++--
modules/httpserver-api/main.cc | 52 ++++++++++++-----------
modules/httpserver-api/request_handler.cc | 7 +--
modules/httpserver-api/request_handler.hh | 5 +--
modules/httpserver-api/routes.hh | 1 -
modules/httpserver-api/server.cc | 34 +++++++++++----
modules/httpserver-api/server.hh | 3 +-
8 files changed, 83 insertions(+), 58 deletions(-)

diff --git a/modules/httpserver-api/global_server.cc b/modules/httpserver-api/global_server.cc
index 96965744..f56f4ed4 100644
--- a/modules/httpserver-api/global_server.cc
+++ b/modules/httpserver-api/global_server.cc
@@ -17,6 +17,7 @@
#include "json/api_docs.hh"
#include <osv/debug.h>
#include "transformers.hh"
+#include <osv/options.hh>

namespace httpserver {

@@ -30,12 +31,15 @@ global_server& global_server::get()
return *instance;
}

-bool global_server::run(po::variables_map& _config)
+bool global_server::run(std::map<std::string,std::vector<std::string>>& _config)
{
if (get().s != nullptr) {
return false;
}
- std::string config_file_path = _config["config-file"].as<std::string>();
+ std::string config_file_path = "/tmp/httpserver.conf";
+ if (options::option_value_exists(_config, "config-file")) {
+ config_file_path = options::extract_option_value(_config, "config-file");
+ }
std::ifstream f(config_file_path);
if (f.is_open()) {
try {
@@ -79,11 +83,15 @@ bool global_server::run(po::variables_map& _config)
set(_config);
get().set("ipaddress", "0.0.0.0");
get().set("port", "8000");
- get().set("cert", "/etc/pki/server.pem");
- get().set("key", "/etc/pki/private/server.key");
- get().set("cacert", "/etc/pki/CA/cacert.pem");

- get().s = new http::server::server(&get().config, &get()._routes);
+ if (get().config.count("ssl")) {
+ get().set("cert", "/etc/pki/server.pem");
+ get().set("key", "/etc/pki/private/server.key");
+ get().set("cacert", "/etc/pki/CA/cacert.pem");
+ }
+
+ auto port = get().config["port"][0];
+ get().s = new http::server::server(get().config, &get()._routes);

osv::this_application::on_termination_request([&] {
get().s->close();
@@ -92,7 +100,7 @@ bool global_server::run(po::variables_map& _config)
}
});

- std::cout << "Rest API server running on port " << get().config["port"].as<std::string>() << std::endl;
+ std::cout << "Rest API server running on port " << port << std::endl;
get().s->run();
return true;
}
@@ -153,7 +161,7 @@ global_server::global_server()
set_routes();
}

-global_server& global_server::set(po::variables_map& _config)
+global_server& global_server::set(std::map<std::string,std::vector<std::string>> &_config)
{
for (auto i : _config) {
get().config.insert(std::make_pair(i.first, i.second));
@@ -164,9 +172,9 @@ global_server& global_server::set(po::variables_map& _config)
global_server& global_server::set(const std::string& key,
const std::string& _value)
{
- boost::any val(_value);
- boost::program_options::variable_value v(val, false);
- config.insert(std::make_pair(std::string(key), v));
+ std::vector<std::string> values;
+ values.push_back(_value);
+ config.insert(std::make_pair(std::string(key), values));
return *this;
}

diff --git a/modules/httpserver-api/global_server.hh b/modules/httpserver-api/global_server.hh
index 7a8af11a..dc3c2d4a 100644
--- a/modules/httpserver-api/global_server.hh
+++ b/modules/httpserver-api/global_server.hh
@@ -10,13 +10,10 @@
#include "routes.hh"
#include "server.hh"
#include <vector>
-#include <boost/program_options/variables_map.hpp>
#include <mutex>
#include <condition_variable>
#include <external/x64/misc.bin/usr/include/yaml-cpp/node/iterator.h>

-namespace po = boost::program_options;

-
namespace httpserver {
/**
* Global server is a singleton class that controls
@@ -46,7 +43,7 @@ public:
*
* @return false if the server already running
*/
- static bool run(po::variables_map& config);
+ static bool run(std::map<std::string,std::vector<std::string>>& config);

/**
* set an entry in the configuration
@@ -73,7 +70,7 @@ private:
static global_server* instance;
routes _routes;
http::server::server* s;
- po::variables_map config;
+ std::map<std::string,std::vector<std::string>> config;
std::vector<void*> plugins;

/**
@@ -81,7 +78,7 @@ private:
* @param config a variable map
* @return a reference to the server
*/
- static global_server& set(po::variables_map& config);
+ static global_server& set(std::map<std::string,std::vector<std::string>>& config);

};

diff --git a/modules/httpserver-api/main.cc b/modules/httpserver-api/main.cc
index 7eb9c058..2eca5076 100644
--- a/modules/httpserver-api/main.cc
+++ b/modules/httpserver-api/main.cc
@@ -10,43 +10,47 @@

#include "server.hh"

-#include <boost/program_options/options_description.hpp>
-#include <boost/program_options/variables_map.hpp>
-#include <boost/program_options/parsers.hpp>
#include <boost/asio.hpp>
#include "global_server.hh"
#include <osv/exception_utils.hh>
+#include <osv/options.hh>

using namespace httpserver;

-namespace po = boost::program_options;

+static void usage()
+{
+ std::cout << "Allowed options:\n";
+ std::cout << " --help produce help message\n";

+ std::cout << " --config-file arg (=/tmp/httpserver.conf)\n";
+ std::cout << " configuration file path\n";
+ std::cout << " --access-allow arg Set the Access-Control-Allow-Origin to\n";
+ std::cout << " *. Note the security risk\n";
+ std::cout << " --ipaddress arg set the ip address\n";
+ std::cout << " --port arg set the port\n";
+ std::cout << " --cert arg path to server's SSL certificate\n";
+ std::cout << " --key arg path to server's private key\n";
+ std::cout << " --cacert arg path to CA certificate\n";
+ std::cout << " --ssl enable SSL\n\n";

+}
+
+static void handle_parse_error(const std::string &message)
+{
+ std::cout << message << std::endl;
+ usage();
+ exit(1);
+}

int main(int argc, char* argv[])

{

- po::options_description desc("Allowed options");
- desc.add_options()
- ("help", "produce help message")

- ("config-file", po::value<std::string>()->default_value("/tmp/httpserver.conf"), "configuration file path")
- ("access-allow", po::value<std::string>(),
- "Set the Access-Control-Allow-Origin to *. Note the security risk")
- ("ipaddress", po::value<std::string>(), "set the ip address")
- ("port", po::value<std::string>(), "set the port")
- ("cert", po::value<std::string>(), "path to server's SSL certificate")
- ("key", po::value<std::string>(), "path to server's private key")
- ("cacert", po::value<std::string>(), "path to CA certificate")
- ("ssl", "enable SSL");
-
- po::variables_map config;
- po::store(po::parse_command_line(argc, argv, desc), config);
- po::notify(config);
-
- if (config.count("help")) {
- std::cerr << desc << "\n";
+ auto options_values = options::parse_options_values(argc - 1, argv + 1, handle_parse_error);

+
+ if (options::extract_option_flag(options_values, "help", handle_parse_error)) {
+ usage();

return 1;
}

try {
- global_server::run(config);
+ global_server::run(options_values);
} catch (...) {
std::cerr << "httpserver failed: " << current_what() << std::endl;
return 1;
diff --git a/modules/httpserver-api/request_handler.cc b/modules/httpserver-api/request_handler.cc
index e03222ff..8272da80 100644
--- a/modules/httpserver-api/request_handler.cc
+++ b/modules/httpserver-api/request_handler.cc
@@ -20,6 +20,7 @@
#include <fstream>
#include <sstream>
#include <string>
+#include <osv/options.hh>

namespace http {

@@ -62,13 +63,13 @@ size_t request_handler::update_parameters(request& req)
return par;
}

-request_handler::request_handler(httpserver::routes* routes, const boost::program_options::variables_map& _config)
+request_handler::request_handler(httpserver::routes* routes, std::map<std::string,std::vector<std::string>>& _config)
: routes(routes),
config(_config)

{
- if (config.count("access-allow")) {
- const auto s = config["access-allow"].as<std::string>();
+ if (options::option_value_exists(_config, "access-allow")) {
+ const auto s = options::extract_option_value(_config, "access-allow");

std::string::size_type b = 0;
do {
diff --git a/modules/httpserver-api/request_handler.hh b/modules/httpserver-api/request_handler.hh
index 180782a3..7e055c16 100644
--- a/modules/httpserver-api/request_handler.hh
+++ b/modules/httpserver-api/request_handler.hh
@@ -15,7 +15,6 @@

#include "routes.hh"

-#include <boost/program_options/variables_map.hpp>
#include <string>

namespace http {
@@ -40,7 +39,7 @@ public:
*
* @param routes the routes object
*/
- explicit request_handler(httpserver::routes* routes, const boost::program_options::variables_map& _config);
+ explicit request_handler(httpserver::routes* routes, std::map<std::string,std::vector<std::string>>& _config);

request_handler(const request_handler&) = delete;

@@ -74,7 +73,7 @@ public:
private:
httpserver::routes* routes;
std::vector<std::string> allowed_domains;
- const boost::program_options::variables_map& config;
+ const std::map<std::string,std::vector<std::string>>& config;

};

diff --git a/modules/httpserver-api/routes.hh b/modules/httpserver-api/routes.hh
index 236940cd..7cff5dbb 100644
--- a/modules/httpserver-api/routes.hh
+++ b/modules/httpserver-api/routes.hh
@@ -14,7 +14,6 @@
#include "common.hh"
#include "reply.hh"

-#include <boost/program_options/variables_map.hpp>
#include <unordered_map>
#include <vector>

diff --git a/modules/httpserver-api/server.cc b/modules/httpserver-api/server.cc
index 4e8f18a9..709972b2 100644
--- a/modules/httpserver-api/server.cc
+++ b/modules/httpserver-api/server.cc
@@ -17,6 +17,7 @@

#include <utility>
#include <openssl/ssl.h>
+#include <osv/options.hh>

namespace http {

@@ -28,17 +29,17 @@ static bool exists(const std::string& path)
return stat(path.c_str(), &s) == 0;
}

-server::server(const boost::program_options::variables_map* config,
+server::server(std::map<std::string,std::vector<std::string>> &config,
httpserver::routes* routes)
: io_service_()
, connection_manager_()
- , request_handler_(routes, *config)
+ , request_handler_(routes, config)
{
// Open the acceptor with the option to reuse the address (i.e. SO_REUSEADDR).
boost::asio::ip::tcp::resolver resolver(io_service_);
boost::asio::ip::tcp::endpoint endpoint = *resolver.resolve({
- (*config)["ipaddress"].as<std::string>(),
- (*config)["port"].as<std::string>()
+ options::extract_option_value(config, "ipaddress"),
+ options::extract_option_value(config, "port")
});

tcp::acceptor tcp_acceptor(io_service_);
@@ -47,12 +48,22 @@ server::server(const boost::program_options::variables_map* config,
tcp_acceptor.bind(endpoint);
tcp_acceptor.listen();

- if (config->count("ssl")) {
+ if (options::extract_option_flag(config, "ssl", [](const std::string &message) {
+ std::cerr << message << std::endl;
+ throw std::runtime_error("invalid configuration");
+ })) {
ensure_openssl_initialized();

- auto ca_cert_path = (*config)["cacert"].as<std::string>();
- auto cert_path = (*config)["cert"].as<std::string>();
- auto key_path = (*config)["key"].as<std::string>();
+ auto ca_cert_path = options::extract_option_value(config,"cacert");
+ auto cert_path = options::extract_option_value(config,"cert");
+ auto key_path = options::extract_option_value(config,"key");
+
+ if (!config.empty()) {
+ for (auto option : config) {
+ std::cout << "Unrecognized option: " << option.first << std::endl;
+ }
+ throw std::runtime_error("invalid configuration");
+ }

bool valid = true;
for (auto& path : {ca_cert_path, cert_path, key_path}) {
@@ -70,6 +81,13 @@ server::server(const boost::program_options::variables_map* config,
ssl::context ctx = make_ssl_context(ca_cert_path, cert_path, key_path);
acceptor_.reset(new ssl_acceptor(io_service_, std::move(ctx), std::move(tcp_acceptor)));
} else {
+ if (!config.empty()) {
+ for (auto option : config) {
+ std::cout << "Unrecognized option: " << option.first << std::endl;
+ }
+ throw std::runtime_error("invalid configuration");
+ }
+
acceptor_.reset(new plain_acceptor(io_service_, std::move(tcp_acceptor)));
}

diff --git a/modules/httpserver-api/server.hh b/modules/httpserver-api/server.hh
index 94f414a0..2da4f574 100644
--- a/modules/httpserver-api/server.hh
+++ b/modules/httpserver-api/server.hh
@@ -17,7 +17,6 @@
#include "connection.hh"
#include "transport.hh"

-#include <boost/program_options/variables_map.hpp>
#include <boost/asio.hpp>
#include <string>
#include <memory>
@@ -43,7 +42,7 @@ public:
* @param config a configuration object
* @param routes the routes object
*/
- explicit server(const boost::program_options::variables_map* config,
+ explicit server(std::map<std::string,std::vector<std::string>> &config,
httpserver::routes* routes);

server(const server&) = delete;
--
2.20.1

[Nadav Har'El]

On Thu, Oct 3, 2019 at 7:24 AM Waldemar Kozaczuk <jwkoz...@gmail.com> wrote:

This patch provides simple and light alternative to boost program options
library. It adds handful of utility functions that are aimed
to help parse command line options and intended to be used
by kernel loader and the utility apps like cpiod, httpserver
and cloud init.

For more details on what particular functions do
look at the comments in options.hh and options.cc.

Very nice. I only have some very small comments below.

Looking below, I think you also support "--key value" with a space instead of  of an equal sign?

If this is true, then this case is missing from this part of the comment - but appears below, which

may be a sign that this comment is too repetitous lists the various options twice.

+// 3) 'value'
+//
+// If 'allow_separate_values' is false, then only first 2 forms are valid

 I don't understand what 3 means. I *thought* it means you also allow just normal words - e.g., in "ls xyz", the "xyz". But it doesn't seem you allow this at all below.

 

and
+// the '--key' arguments are identified as 'flag' options (like '--enabled')
+// and '--key=value' arguments are treated as key, value pairs
+//
+// If 'allow_separate_values' is true, then all 3 forms are valid and
+// the '--key' arguments NOT followed by 'value' are identified as 'flag' options (like '--enabled')
+// and '--key=value' arguments are treated as key, value pairs
+// and '--key value' arguments are treated as key, value pairs as well
+map<string,vector<string>> parse_options_values(int argc, char** argv,
+                                                function<void (const string&)> error_handler,
+                                                bool allow_separate_values)
+{
+    map<string,vector<string>> options_values;
+
+    string key("");
+    for (int i = 0; i < argc; i++) {
+        string arg(argv[i]);
+        //
+        // Verify if is a 'long' option (starts with '--')
+        if (arg.find("--") != 0) {

Although find()==0 works, it's not exactly the most efficient way to check if a string starts with something. But I guess that given the insignificance of the performance of this code, I can let it slide :-)

If you only ask to extract one value, shouldn't it be an error if the user entered more than one?

I.e., should this function verify that there is just one item in the values vector?

+}
+
+static void handle_invalid_argument(const string &name, const string &value, function<void (const string&)> error_handler)
+{
+    error_handler(string("the argument ('") + value + "') for option '--" + name + "' is invalid");
+}
+
+int extract_option_int_value(map<string,vector<string>> &options_values, const string &name, function<void (const string&)> error_handler)
+{
+    auto value_str = extract_option_values(options_values, name)[0];

Maybe use extract_option_value() here, as this already handles the [0] (and, I suggest, also an error if there is more than one?)

+    size_t pos;
+    int value = 0;
+
+    try {
+        value = std::stoi(value_str, &pos);
+        if (pos < value_str.length()) {
+            handle_invalid_argument(name, value_str, error_handler);

Does handle_invalid_argument() trow? If not, this code will end up returning value=0. Is this ok?

Should this be in the "osv" namespace?

+
+using namespace std;

Please don't put "using namespace" inside a header file - this can "infect" its users. You can just write std::string, std::vector, etc., below.

--
You received this message because you are subscribed to the Google Groups "OSv Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/20191003042437.27978-2-jwkozaczuk%40gmail.com.

[Nadav Har'El]

Reviewed-by: Nadav Har'El <n...@scylladb.com>
--
Nadav Har'El

n...@scylladb.com

On Thu, Oct 3, 2019 at 7:24 AM Waldemar Kozaczuk <jwkoz...@gmail.com> wrote:

--
You received this message because you are subscribed to the Google Groups "OSv Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+u...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/20191003042437.27978-3-jwkozaczuk%40gmail.com.

[Nadav Har'El]

Reviewed-by: Nadav Har'El <n...@scylladb.com>

--
Nadav Har'El

n...@scylladb.com

--
You received this message because you are subscribed to the Google Groups "OSv Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+u...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/20191003042437.27978-4-jwkozaczuk%40gmail.com.

[Nadav Har'El]

Reviewed-by: Nadav Har'El <n...@scylladb.com>

--
Nadav Har'El

n...@scylladb.com

On Thu, Oct 3, 2019 at 7:25 AM Waldemar Kozaczuk <jwkoz...@gmail.com> wrote:

--
You received this message because you are subscribed to the Google Groups "OSv Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+u...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/20191003042437.27978-5-jwkozaczuk%40gmail.com.

[Nadav Har'El]

Since I only had tiny comments, I'll commit your patches as-is, and if you want, please send (or commit yourself) followup patches.

Thanks for fixing this! Fedora 30 users everywhere thank you :-)

--
Nadav Har'El

n...@scylladb.com

[Commit Bot]

From: Waldemar Kozaczuk <jwkoz...@gmail.com>
Committer: Nadav Har'El <n...@scylladb.com>
Branch: master

options: implement light and simple alternative to boost program options

This patch provides simple and light alternative to boost program options
library. It adds handful of utility functions that are aimed
to help parse command line options and intended to be used
by kernel loader and the utility apps like cpiod, httpserver
and cloud init.

For more details on what particular functions do
look at the comments in options.hh and options.cc.

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>

Message-Id: <20191003042437.2...@gmail.com>

---
diff --git a/Makefile b/Makefile

--- a/Makefile
+++ b/Makefile
@@ -955,6 +955,7 @@ objects += core/app.o
objects += core/libaio.o
objects += core/osv_execve.o
objects += core/osv_c_wrappers.o
+objects += core/options.o

#include $(src)/libc/build.mk:
libc =
diff --git a/core/options.cc b/core/options.cc

--- a/core/options.cc

--- a/include/osv/options.hh

--- a/modules/tests/Makefile
+++ b/modules/tests/Makefile
@@ -109,7 +109,7 @@ tests := tst-pthread.so misc-ramdisk.so tst-vblk.so
tst-bsd-evh.so \
tst-dns-resolver.so tst-kill.so tst-truncate.so \
misc-panic.so tst-utimes.so tst-utimensat.so tst-futimesat.so \
misc-tcp.so tst-strerror_r.so misc-random.so misc-urandom.so \
- tst-commands.so tst-threadcomplete.so tst-timerfd.so \
+ tst-commands.so tst-options.so tst-threadcomplete.so tst-timerfd.so \
tst-nway-merger.so tst-memmove.so tst-pthread-clock.so misc-procfs.so \
tst-chdir.so tst-chmod.so tst-hello.so misc-concurrent-io.so \
tst-concurrent-init.so tst-ring-spsc-wraparound.so tst-shm.so \
diff --git a/tests/tst-options.cc b/tests/tst-options.cc

--- a/tests/tst-options.cc

[Commit Bot]

From: Waldemar Kozaczuk <jwkoz...@gmail.com>
Committer: Nadav Har'El <n...@scylladb.com>
Branch: master

loader: replace boost program_options with light options utility functions

This patch changes loader.cc to use light options utility
instead of boost program options.

This patch partially addresses #1014 and together with one
of the previous patches sent to update core/commands.cc
to remove boost program options, fully addresses #1014.

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>

Message-Id: <20191003042437.2...@gmail.com>

---
diff --git a/loader.cc b/loader.cc

@@ -252,56 +250,70 @@ void parse_options(int loader_argc, char**

loader_argv)
}
}
}
- opt_mount = !vars.count("nomount");
- opt_pivot = !vars.count("nopivot");
- opt_random = !vars.count("norandom");
- opt_init = !vars.count("noinit");

- if (vars.count("console")) {
- auto v = vars["console"].as<std::vector<std::string>>();
+ opt_mount = !extract_option_flag(options_values, "nomount");
+ opt_pivot = !extract_option_flag(options_values, "nopivot");
+ opt_random = !extract_option_flag(options_values, "norandom");
+ opt_init = !extract_option_flag(options_values, "noinit");
+
+ if (options::option_value_exists(options_values, "console")) {
+ auto v = options::extract_option_values(options_values, "console");
if (v.size() > 1) {
printf("Ignoring '--console' options after the first.");
}

opt_console = v.front();

[Commit Bot]

From: Waldemar Kozaczuk <jwkoz...@gmail.com>
Committer: Nadav Har'El <n...@scylladb.com>
Branch: master

cpiod: reduce cpiod size by replacing boost program options with the custom
light alternative

Fixes #980

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>

Message-Id: <20191003042437.2...@gmail.com>

---
diff --git a/tools/cpiod/cpiod.cc b/tools/cpiod/cpiod.cc

--- a/tools/cpiod/cpiod.cc
+++ b/tools/cpiod/cpiod.cc
@@ -11,17 +11,16 @@
#include <string>
#include <system_error>
#include <boost/asio.hpp>
-#include <boost/program_options.hpp>
#include "cpio.hh"
#include <sys/mount.h>
#include <errno.h>
#include <stdio.h>
#include <sys/stat.h>
+#include <osv/options.hh>

using namespace osv;
using namespace std;
using namespace boost::asio::ip;
-namespace po = boost::program_options;

// Want to use boost::filesystem, but too much effort to integrate
extern "C" { int mkdirp(const char *d, mode_t mode); }

@@ -127,31 +126,55 @@ class cpio_in_expand : public cpio_in {

[Commit Bot]

From: Waldemar Kozaczuk <jwkoz...@gmail.com>
Committer: Nadav Har'El <n...@scylladb.com>
Branch: master

httpserver: replace boost program_options with light options utility
functions

Fixes #1053

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>

Message-Id: <20191003042437.2...@gmail.com>

---
diff --git a/modules/httpserver-api/global_server.cc
b/modules/httpserver-api/global_server.cc

--- a/modules/httpserver-api/global_server.hh
+++ b/modules/httpserver-api/global_server.hh
@@ -10,13 +10,10 @@
#include "routes.hh"
#include "server.hh"
#include <vector>
-#include <boost/program_options/variables_map.hpp>
#include <mutex>
#include <condition_variable>
#include <external/x64/misc.bin/usr/include/yaml-cpp/node/iterator.h>

-namespace po = boost::program_options;
-
namespace httpserver {
/**
* Global server is a singleton class that controls
@@ -46,7 +43,7 @@ public:
*
* @return false if the server already running
*/
- static bool run(po::variables_map& config);
+ static bool run(std::map<std::string,std::vector<std::string>>&
config);

/**
* set an entry in the configuration

@@ -73,15 +70,15 @@ private:

static global_server* instance;
routes _routes;
http::server::server* s;
- po::variables_map config;
+ std::map<std::string,std::vector<std::string>> config;
std::vector<void*> plugins;

/**

* set configuration based on command line.

* @param config a variable map
* @return a reference to the server
*/
- static global_server& set(po::variables_map& config);
+ static global_server&
set(std::map<std::string,std::vector<std::string>>& config);

};

diff --git a/modules/httpserver-api/main.cc b/modules/httpserver-api/main.cc

--- a/modules/httpserver-api/request_handler.hh
+++ b/modules/httpserver-api/request_handler.hh
@@ -15,7 +15,6 @@

#include "routes.hh"

-#include <boost/program_options/variables_map.hpp>
#include <string>

namespace http {
@@ -40,7 +39,7 @@ public:
*
* @param routes the routes object
*/
- explicit request_handler(httpserver::routes* routes, const
boost::program_options::variables_map& _config);
+ explicit request_handler(httpserver::routes* routes,
std::map<std::string,std::vector<std::string>>& _config);

request_handler(const request_handler&) = delete;

@@ -74,7 +73,7 @@ public:
private:
httpserver::routes* routes;
std::vector<std::string> allowed_domains;
- const boost::program_options::variables_map& config;
+ const std::map<std::string,std::vector<std::string>>& config;

};

diff --git a/modules/httpserver-api/routes.hh
b/modules/httpserver-api/routes.hh

--- a/modules/httpserver-api/routes.hh
+++ b/modules/httpserver-api/routes.hh
@@ -14,7 +14,6 @@
#include "common.hh"
#include "reply.hh"

-#include <boost/program_options/variables_map.hpp>
#include <unordered_map>
#include <vector>

diff --git a/modules/httpserver-api/server.cc
b/modules/httpserver-api/server.cc

[Waldek Kozaczuk]

Right, I really meant  "--key value" which would take two arguments which is why I formulated my comment like that. But I agree it is confusing. I will fix it in a followup patch.

+// 3) 'value'
+//
+// If 'allow_separate_values' is false, then only first 2 forms are valid

 I don't understand what 3 means. I *thought* it means you also allow just normal words - e.g., in "ls xyz", the "xyz". But it doesn't seem you allow this at all below.

Again see my explanation above. 

 

and
+// the '--key' arguments are identified as 'flag' options (like '--enabled')
+// and '--key=value' arguments are treated as key, value pairs
+//
+// If 'allow_separate_values' is true, then all 3 forms are valid and
+// the '--key' arguments NOT followed by 'value' are identified as 'flag' options (like '--enabled')
+// and '--key=value' arguments are treated as key, value pairs
+// and '--key value' arguments are treated as key, value pairs as well
+map<string,vector<string>> parse_options_values(int argc, char** argv,
+                                                function<void (const string&)> error_handler,
+                                                bool allow_separate_values)
+{
+    map<string,vector<string>> options_values;
+
+    string key("");
+    for (int i = 0; i < argc; i++) {
+        string arg(argv[i]);
+        //
+        // Verify if is a 'long' option (starts with '--')
+        if (arg.find("--") != 0) {

Although find()==0 works, it's not exactly the most efficient way to check if a string starts with something. But I guess that given the insignificance of the performance of this code, I can let it slide :-)

What would you recommend instead? 

I guess. I can make this tweak. 

+}
+
+static void handle_invalid_argument(const string &name, const string &value, function<void (const string&)> error_handler)
+{
+    error_handler(string("the argument ('") + value + "') for option '--" + name + "' is invalid");
+}
+
+int extract_option_int_value(map<string,vector<string>> &options_values, const string &name, function<void (const string&)> error_handler)
+{
+    auto value_str = extract_option_values(options_values, name)[0];

Maybe use extract_option_value() here, as this already handles the [0] (and, I suggest, also an error if there is more than one?)

Agree. 

+    size_t pos;
+    int value = 0;
+
+    try {
+        value = std::stoi(value_str, &pos);
+        if (pos < value_str.length()) {
+            handle_invalid_argument(name, value_str, error_handler);

Does handle_invalid_argument() trow? If not, this code will end up returning value=0. Is this ok?

It depends on the user. These 'options' utility functions are not really meant to be used outside of the kernel loader, cpiod, etc. I think in all the cases the provided handler prints a message and somehow makes the or kernel terminate. So in all these cases returned 0 does not matter. Is there a better way to design/implement this function?

osv::options? So options nested in osv, right? 

+
+using namespace std;

Please don't put "using namespace" inside a header file - this can "infect" its users. You can just write std::string, std::vector, etc., below.

I will fix it. 

To unsubscribe from this group and stop receiving emails from it, send an email to osv...@googlegroups.com.

[Waldek Kozaczuk]

I have measured the impact of this patch on the size of the kernel (loader-stripped.elf) and here are the results on Ubuntu 19.04 and Fedora 30:

Ubuntu 19.04, GCC 8.3, Boost 1.67

-------------------

After:

fs=rofs   6.2M, 6,420,296 bytes (-0.5M)

fs=zfs:   6.8M, 7,096,136 bytes (-0.6M)

Before:  

fs=rofs   6.7M, 6,952,912 bytes

fs=zfs:   7.4M, 7,739,472 bytes

Fedora 30, GCC 9.2.1, Boost 1.69

----------------

After:

fs=rofs   6.7M, 6,967,288  (-0.7M)

fs=zfs:   7.4M,  7,671,800 (-0.6M)

Before (with cpiod fixed to remove boost program options):  

fs=rofs   7.4M, 7,666,160 bytes

fs=zfs:   8.0M, 8,370,672 bytes

I wonder why the kernel is on average 0.5 larger on Fedora. Is it because of the different version of GCC?

Waldek

To unsubscribe from this group and stop receiving emails from it, send an email to osv...@googlegroups.com.