On Mon, Jan 9, 2017 at 10:06 PM, Justin Cinkelj <[email protected]>
wrote:
> 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.
Right. Would "auto& prm : cmd" work? Note the ampersand. It takes the
strings by reference, not making a temporary copy of each.
> 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.
