On Thu, Aug 24, 2017 at 7:56 PM, Justin Cinkelj <justin.cink...@xlab.si> wrote:
> Parse loader options with loader_parse_cmdline instead of strtok_r.
> The cmdline passed in prepare_commands to osv::parse_command_line
> is not reassebled from already parsed argv anymore, as app_cmdline
> contains unmodifed commandline string (only loader options are removed).
>
> Minor bug: commandline with only whitespaces results in VM to nicely
> shutdown instead of reporting:
> "This image has an empty command line. Nothing to run."
> E.g.
> ./scripts/run.py -e ' --env=AA=aa ' # OK
> ./scripts/run.py -e ' --env=AA=aa ' # bug
> Will be fixed in a follow up commit.
Is this bug fixed in patch 6 (where you change the condition for
printing that message), or not?
>
> Cosmetic change: many argc/argv were renamed to loader_argc/loader_argv.
>
> Fixes #892
>
> Signed-off-by: Justin Cinkelj <justin.cink...@xlab.si>
> ---
> arch/x64/boot.S | 4 ++--
> core/commands.cc | 31 ++++++++++++-------------------
> include/osv/commands.hh | 5 +++--
> loader.cc | 49
> ++++++++++++++++---------------------------------
> 4 files changed, 33 insertions(+), 56 deletions(-)
>
> diff --git a/arch/x64/boot.S b/arch/x64/boot.S
> index 1782a4c..c4b97e2 100644
> --- a/arch/x64/boot.S
> +++ b/arch/x64/boot.S
> @@ -102,8 +102,8 @@ start64:
> mov %rbx, osv_multiboot_info
> lea init_stack_top, %rsp
> call premain
> - mov __argc, %edi
> - mov __argv, %rsi
> + mov __loader_argc, %edi
> + mov __loader_argv, %rsi
> call main
> .cfi_endproc
>
> diff --git a/core/commands.cc b/core/commands.cc
> index 22024fa..8607ab6 100644
> --- a/core/commands.cc
> +++ b/core/commands.cc
> @@ -311,7 +311,6 @@ parse_command_line(const std::string line, bool &ok)
> // time. So let's go for a more traditional memory management to avoid
> testing
> // early / not early, etc
> char *osv_cmdline = nullptr;
> -static std::vector<char*> args;
>
> std::string getcmdline()
> {
> @@ -433,31 +432,25 @@ void loader_parse_cmdline(char* str, int *pargc,
> char*** pargv, char** app_cmdli
>
> int parse_cmdline(const char *p)
> {
> - char* save;
> -
> - if (args.size()) {
> - // From the strtok manpage, we see that: "The first call to strtok()
> - // sets this pointer to point to the first byte of the string." It
> - // follows from this that the first argument contains the address we
> - // should use to free the memory allocated for the string
> - free(args[0]);
> + if (__loader_argv) {
> + if (__loader_argv[0]) {
> + // __loader_argv[0] was allocated by us
> + free(__loader_argv[0]);
> + }
> + // __loader_argv was allocated by loader_parse_cmdline
> + free(__loader_argv);
> }
>
> - args.resize(0);
> if (osv_cmdline) {
> free(osv_cmdline);
> }
> osv_cmdline = strdup(p);
>
> - char* cmdline = strdup(p);
> -
> - while ((p = strtok_r(cmdline, " \t\n", &save)) != nullptr) {
> - args.push_back(const_cast<char *>(p));
> - cmdline = nullptr;
> - }
> - args.push_back(nullptr);
> - __argv = args.data();
> - __argc = args.size() - 1;
> + char* cmdline = strdup(p); // will be freed on next call, as
> __loader_argv[0].
> + loader_parse_cmdline(cmdline, &__loader_argc, &__loader_argv,
> &__app_cmdline);
> + // Ensure that even with space at start of string p, __loader_argv[0] ==
> cmdline is still true.
> + assert(__loader_argv);
> + assert(cmdline == __loader_argv[0]);
>
> return 0;
> }
> diff --git a/include/osv/commands.hh b/include/osv/commands.hh
> index 0693d28..e25ab51 100644
> --- a/include/osv/commands.hh
> +++ b/include/osv/commands.hh
> @@ -13,8 +13,9 @@
> #include <vector>
> #include <system_error>
>
> -extern int __argc;
> -extern char** __argv;
> +extern int __loader_argc;
> +extern char** __loader_argv;
> +extern char* __app_cmdline;
> static constexpr size_t max_cmdline = 1024;
>
> namespace osv {
> diff --git a/loader.cc b/loader.cc
> index 0f99ba5..504e230 100644
> --- a/loader.cc
> +++ b/loader.cc
> @@ -113,11 +113,11 @@ void premain()
> boot_time.event(".init functions");
> }
>
> -int main(int ac, char **av)
> +int main(int loader_argc, char **loader_argv)
> {
> smp_initial_find_current_cpu()->init_on_cpu();
> - void main_cont(int ac, char** av);
> - sched::init([=] { main_cont(ac, av); });
> + void main_cont(int loader_argc, char** loader_argv);
> + sched::init([=] { main_cont(loader_argc, loader_argv); });
> }
>
> static bool opt_leak = false;
> @@ -144,20 +144,13 @@ int maxnic;
> static int sampler_frequency;
> static bool opt_enable_sampler = false;
>
> -std::tuple<int, char**> parse_options(int ac, char** av)
> +void parse_options(int loader_argc, char** loader_argv)
> {
> namespace bpo = boost::program_options;
> namespace bpos = boost::program_options::command_line_style;
>
> std::vector<const char*> args = { "osv" };
> -
> - // due to 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 nr_options = std::find_if(av, av + ac,
> - [](const char* arg) { return arg[0] !=
> '-'; }) - av;
> - std::copy(av, av + nr_options, std::back_inserter(args));
> + std::copy(loader_argv, loader_argv + loader_argc,
> std::back_inserter(args));
>
> bpo::options_description desc("OSv options");
> desc.add_options()
> @@ -297,29 +290,19 @@ std::tuple<int, char**> parse_options(int ac, char** av)
> }
>
> boot_delay = std::chrono::duration_cast<std::chrono::nanoseconds>(1_s *
> vars["delay"].as<float>());
> -
> - av += nr_options;
> - ac -= nr_options;
> - return std::make_tuple(ac, av);
> }
>
> // return the std::string and the commands_args poiting to them as a move
> -std::vector<std::vector<std::string> > prepare_commands(int ac, char** av)
> +std::vector<std::vector<std::string> > prepare_commands(char* app_cmdline)
> {
> - if (ac == 0) {
> + if (strlen(app_cmdline) == 0) {
> puts("This image has an empty command line. Nothing to run.");
> osv::poweroff();
> }
> std::vector<std::vector<std::string> > commands;
> - std::string line = std::string("");
> bool ok;
>
> - // concatenate everything
> - for (auto i = 0; i < ac; i++) {
> - line += std::string(av[i]) + " ";
> - }
> -
> - commands = osv::parse_command_line(line, ok);
> + commands = osv::parse_command_line(app_cmdline, ok);
>
> if (!ok) {
> puts("Failed to parse command line.");
> @@ -338,7 +321,7 @@ static std::string read_file(std::string fn)
>
> void* do_main_thread(void *_main_args)
> {
> - auto main_args = static_cast<std::tuple<int,char**> *>(_main_args);
> + auto app_cmdline = static_cast<char*>(_main_args);
>
> if (!arch_setup_console(opt_console)) {
> abort("Unknown console:%s\n", opt_console.c_str());
> @@ -448,7 +431,7 @@ void* do_main_thread(void *_main_args)
> }
> }
>
> - auto commands = prepare_commands(std::get<0>(*main_args),
> std::get<1>(*main_args));
> + auto commands = prepare_commands(app_cmdline);
>
> // Run command lines in /init/* before the manual command line
> if (opt_init) {
> @@ -516,7 +499,7 @@ void* do_main_thread(void *_main_args)
> return nullptr;
> }
>
> -void main_cont(int ac, char** av)
> +void main_cont(int loader_argc, char** loader_argv)
> {
> osv::firmware_probe();
>
> @@ -526,7 +509,7 @@ void main_cont(int ac, char** av)
>
> std::vector<std::vector<std::string> > cmds;
>
> - std::tie(ac, av) = parse_options(ac, av);
> + parse_options(loader_argc, loader_argv);
>
> setenv("OSV_VERSION", osv::version().c_str(), 1);
>
> @@ -581,7 +564,6 @@ void main_cont(int ac, char** av)
>
> pthread_t pthread;
> // run the payload in a pthread, so pthread_self() etc. work
> - std::tuple<int,char**> main_args = std::make_tuple(ac,av);
> // start do_main_thread unpinned (== pinned to all cpus)
> cpu_set_t cpuset;
> CPU_ZERO(&cpuset);
> @@ -591,7 +573,7 @@ void main_cont(int ac, char** av)
> pthread_attr_t attr;
> pthread_attr_init(&attr);
> pthread_attr_setaffinity_np(&attr, sizeof(cpuset), &cpuset);
> - pthread_create(&pthread, &attr, do_main_thread, (void *) &main_args);
> + pthread_create(&pthread, &attr, do_main_thread, (void *) __app_cmdline);
> void* retval;
> pthread_join(pthread, &retval);
>
> @@ -610,5 +592,6 @@ void main_cont(int ac, char** av)
> }
> }
>
> -int __argc;
> -char** __argv;
> +int __loader_argc = 0;
> +char** __loader_argv = nullptr;
> +char* __app_cmdline = nullptr;
> --
> 2.9.4
>
> --
> 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.
> 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 osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
