On FC25 test fails. It is compiler and/or std:: lib dependent.
Diff between v2 nad v3:
- av.reserve(ac+1);
- size_t jj = 0;
- for (auto prm : cmd) {
- av[jj++] = prm.c_str();
+ av.reserve(ac);
+ for (auto it=cmd.begin(); it!=cmd.end(); it++) {
+ av.push_back(it->c_str());
reserve() is not resize(), so I should use push_back, not [] (but even with
that, code works).
I also don't need to reserve ac+1 elements.
Problem was in "(auto prm : cmd)". I guess this creates temporal std::string,
and (temporal) prm.c_str()
is saved into av. On next iteration, (new?) temporal prm.c_str contains
different value, but might be at same address.
So in the failed testcase on FC25, av ended containing 4 pointers, all with
same value.
Using slightly longer iterator version avoids temporal std::string.
Justin
----- Original Message -----
> From: "Justin Cinkelj" <[email protected]>
> To: "Nadav Har'El" <[email protected]>
> Cc: "Osv Dev" <[email protected]>
> Sent: Monday, January 9, 2017 1:14:30 PM
> Subject: Re: [PATCH v2] command line: allow --env in runscript
>
> [xlab@mike-c7 osv]$ sudo ./scripts/run.py -nvd -e '/tests/tst-commands.so'
> ...
> PASS: runscript multiple lines, multiple commands per line, with args
> and quotes
> PASS: runscript with --env
> SUMMARY: 14 tests, 0 failures
> /etc/qemu-ifdown: could not launch network script
> [xlab@mike-c7 osv]$
>
> So debug build on centos7 was OK. And also release build was OK.
> You tested release, on fedora 25? I will retry that.
>
> Justin
>
> On 01/09/2017 10:28 AM, Nadav Har'El wrote:
> > Hi Jusin, thanks. I think this version is indeed cleaner.
> >
> > However, I tried:
> >
> > scripts/build image=tests
> > scripts/run.py -e tests/tst-commands.so
> >
> > and I get a failure:
> >
> > ...
> > PASS: runscript multiple lines
> > PASS: runscript multiple lines, multiple commands per line, with args
> > and quotes
> > FAIL: runscript with --env
> > SUMMARY: 14 tests, 1 failures
> >
> > Could you please look into it?
> >
> > Thanks,
> > Nadav.
> >
> >
> >
> >
> > --
> > Nadav Har'El
> > [email protected] <mailto:[email protected]>
> >
> > On Mon, Jan 9, 2017 at 10:11 AM, Justin Cinkelj
> > <[email protected] <mailto:[email protected]>> wrote:
> >
> > If multiple scripts try to set the same environment variable, then the
> > last one wins.
> >
> > Fixes #819
> >
> > Signed-off-by: Justin Cinkelj <[email protected]
> > <mailto:[email protected]>>
> >
> > # Conflicts:
> > # loader.cc
> > ---
> > core/commands.cc | 64
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > tests/tst-commands.cc | 52 +++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 116 insertions(+)
> >
> > diff --git a/core/commands.cc b/core/commands.cc
> > index b2b3a60..1ba6bf8 100644
> > --- a/core/commands.cc
> > +++ b/core/commands.cc
> > @@ -12,6 +12,8 @@
> >
> > #include <boost/config/warning_disable.hpp>
> > #include <boost/spirit/include/qi.hpp>
> > +#include <boost/program_options.hpp>
> > +#include <osv/power.hh>
> > #include <osv/commands.hh>
> > #include <osv/align.hh>
> > #include <sys/types.h>
> > @@ -90,6 +92,66 @@ 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;
> > + namespace bpos = boost::program_options::command_line_style;
> > + // 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);
> > + 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;
> > +
> > + std::vector<const char*> args = { "dummy-string" };
> > + // due to https://svn.boost.org/trac/boost/ticket/6991
> > <https://svn.boost.org/trac/boost/ticket/6991>, we can't terminate
> > + // command line parsing on the executable name, so we
> > need to look for it
> > + // ourselves
> > + auto ac = cmd.size();
> > + auto av = std::vector<const char*>();
> > + av.reserve(ac+1);
> > + size_t jj = 0;
> > + for (auto prm : cmd) {
> > + av[jj++] = prm.c_str();
> > + }
> > + auto nr_options = std::find_if(av.data(), av.data() + ac,
> > + [](const char* arg) {
> > return arg[0] != '-'; }) - av.data();
> > + std::copy(av.data(), av.data() + nr_options,
> > std::back_inserter(args));
> > +
> > + 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);
> > +
> > + 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()));
> > + }
> > + }
> > +
> > + cmd.erase(cmd.begin(), cmd.begin() + nr_options);
> > + 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 +187,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/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>.
> >
> >
>
>
--
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.
