--
Nadav Har'El
[email protected] <mailto:[email protected]>
On Thu, Dec 29, 2016 at 6:49 PM, Justin Cinkelj
<[email protected] <mailto:[email protected]>> wrote:
Currently runscript accepts only one option. By reusing
parse_options_parse that can be easily extended with additional
options.
Variant of parse_options_parse accepting vector<string> instead of
argc,argv was added, for easier interfacing.
Fixes #819
Signed-off-by: Justin Cinkelj <[email protected]
<mailto:[email protected]>>
---
core/commands.cc | 39 ++++++++++++++++++++++++++++++++++++++
loader.cc | 22 ++++++++++++++++++++++
tests/tst-commands.cc | 52
+++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 113 insertions(+)
diff --git a/core/commands.cc b/core/commands.cc
index b2b3a60..a5879f4 100644
--- a/core/commands.cc
+++ b/core/commands.cc
@@ -22,6 +22,11 @@ namespace qi = boost::spirit::qi;
namespace ascii = boost::spirit::ascii;
using boost::spirit::ascii::space;
+#include <boost/program_options.hpp>
+void parse_options_parse(std::vector<std::string>& cmd,
+ const boost::program_options::options_description& desc,
+ boost::program_options::variables_map& vars);
Putting such extern declaration in source files is dangerous, because
nothing checks that the declaration here and in loader.cc is actually
the same. This is why you usually put such declarations in header
files, included in both places.
But as I commented earlier, I think it would be even better not to
have this function at all, and just use the boost::program_options
stuff directly. This will add about 5 lines of code, I think, and it
will be clearer (because you will use boost::program_options directly).
+
namespace osv {
typedef std::string::const_iterator sciter;
@@ -90,6 +95,38 @@ parse_command_line_min(const std::string line,
bool &ok)
}
/*
+In each runscript line, first N args starting with - are options.
+Parse options and remove them from result.
+
+Options are applied immediately, just as in loader.cc
parse_options().
+So if two scripts set the same environment variable, then the
last one wins.
+Applying all options before running any command is also safer
than trying to
+apply options for each script at script execution (second script
would modify
+environment setup by the first script, causing a race).
+*/
+static void
runscript_process_options(std::vector<std::vector<std::string> >&
result) {
+ namespace bpo = boost::program_options;
+ bpo::options_description desc("OSv runscript options");
+ desc.add_options()
+ ("env", bpo::value<std::vector<std::string>>(), "set
Unix-like environment variable (putenv())");
+
+ for (size_t ii=0; ii<result.size(); ii++) {
+ auto cmd = result[ii];
+ bpo::variables_map vars;
+ parse_options_parse(cmd, desc, vars);
+
+ if (vars.count("env")) {
+ for (auto t :
vars["env"].as<std::vector<std::string>>()) {
+ debug("Setting in environment: %s\n", t);
+ putenv(strdup(t.c_str()));
+ }
+ }
+
+ result[ii] = cmd;
+ }
+}
+
+/*
If cmd starts with "runcript file", read content of file and
return vector of all programs to be run.
File can contain multiple commands per line.
@@ -125,6 +162,8 @@ std::vector<std::vector<std::string>>
runscript_expand(const std::vector<std::st
ok = false;
return result2;
}
+ // process and remove options from command
+ runscript_process_options(result3);
result2.insert(result2.end(), result3.begin(),
result3.end());
line_num++;
}
diff --git a/loader.cc b/loader.cc
index 47a547f..11a07c8 100644
--- a/loader.cc
+++ b/loader.cc
@@ -177,6 +177,28 @@ std::tuple<int, char**>
parse_options_parse(int ac, char** av,
return std::make_tuple(ac, av);
}
+void parse_options_parse(std::vector<std::string>& cmd,
+ const boost::program_options::options_description& desc,
+ boost::program_options::variables_map& vars)
+{
+ int ac, ii;
+ char** av;
+
+ ac = cmd.size();
+ av = (char**)malloc(sizeof(char*) * (ac+1));
Why ac+1, and then you don't set the last place? Note that malloc does
not default to setting memory to 0 - if that's what you intended...
By the way, you can also
auto av = vector<char*>();
av.reserve(ac+1);
and then use av.data() where you need the char**.
The benefit of using vector<> is that you don't need to worry about
deleting the memory, the compiler does it automatically when you go
out of scope.
+ av[ac] = nullptr;
+ ii = 0;
+ for (auto prm : cmd) {
+ av[ii++] = strdup(prm.c_str());
Why do we need to strdup the individual strings? If we just do
av[ii++] = prm.c_str() wouldn't it also be good enough? The "cmd" (and
all these pointers) will still live when we parse the options below,
right?
+ }
+
+ auto ac_av = parse_options_parse(ac, av, desc, vars);
+ cmd.erase(cmd.begin(), cmd.begin() + (ac - std::get<0>(ac_av)));
+ for (ii=0; ii<ac; ii++) {
+ free(av[ii]);
+ }
Here you forgot to free() av itself... This is the kind of error that
the vector<> I suggested above would have saved you.
+}
+
std::tuple<int, char**> parse_options(int ac, char** av) /**/
{
namespace bpo = boost::program_options;
diff --git a/tests/tst-commands.cc b/tests/tst-commands.cc
index de0c143..7d0a679 100644
--- a/tests/tst-commands.cc
+++ b/tests/tst-commands.cc
@@ -625,6 +625,56 @@ static bool
test_runscript_multiline_multiple_commands_per_line_with_args_quotes
return true;
}
+static bool test_runscript_with_env()
+{
+ std::ofstream of1("/myscript", std::ios::out | std::ios::binary);
+ of1 << "--env=ASDF=ttrt /prog1 pp1a pp1b\n";
+ of1.close();
+
+ std::vector<std::vector<std::string> > result;
+ std::vector<std::string> cmd = { "/prog1" };
+ size_t expected_size[] = {4};
+ bool ok;
+
+ if (NULL != getenv("ASDF")) {
+ return false;
+ }
+
+ result = osv::parse_command_line(
+ std::string("runscript \"/myscript\"; "),
+ ok);
+
+ if (!ok) {
+ return false;
+ }
+
+ if (result.size() != 1) {
+ return false;
+ }
+
+ for (size_t i = 0; i < result.size(); i++) {
+ if (result[i].size() != expected_size[i]) {
+ return false;
+ }
+ if (result[i][0] != cmd[i]) {
+ return false;
+ }
+ }
+
+ if (result[0][1] != std::string("pp1a")) {
+ return false;
+ }
+ if (result[0][2] != std::string("pp1b")) {
+ return false;
+ }
+
+ if (std::string("ttrt") != getenv("ASDF")) {
+ return false;
+ }
+
+ return true;
+}
+
int main(int argc, char *argv[])
{
report(test_parse_simplest(), "simplest command line");
@@ -651,6 +701,8 @@ int main(int argc, char *argv[])
"runscript multiple lines");
report(test_runscript_multiline_multiple_commands_per_line_with_args_quotes(),
"runscript multiple lines, multiple commands per line,
with args and quotes");
+ report(test_runscript_with_env(),
+ "runscript with --env");
printf("SUMMARY: %d tests, %d failures\n", tests, fails);
return 0;
}
--
2.9.3
--
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 [email protected]
<mailto:osv-dev%[email protected]>.
For more options, visit https://groups.google.com/d/optout
<https://groups.google.com/d/optout>.
