-- Nadav Har'El [email protected] On Thu, Dec 29, 2016 at 6:49 PM, Justin Cinkelj <[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]> > --- > 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]. > For more options, visit https://groups.google.com/d/optout. > -- 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]. For more options, visit https://groups.google.com/d/optout.
