On Thu, Oct 3, 2019 at 7:24 AM Waldemar Kozaczuk <jwkozac...@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.
> Signed-off-by: Waldemar Kozaczuk <jwkozac...@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
>
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 :-)
> + // 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];
>
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?
+ }
> + }
> + 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 {
>
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.
+
> +// 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
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/osv-dev/20191003042437.27978-2-jwkozaczuk%40gmail.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+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/CANEVyjsGe%3DY3P_T%3DJ_76oNVctRp7YyB3Wx%3Dx0FGavSR3ttyJDg%40mail.gmail.com.
