This patch is just code move. I personally don't like to mix code move and real change so I think I will add a follow up readable patch to this one in order to clean up the mess.
Best regards Benoît On Sat, Sep 3, 2016 at 3:33 PM, Nadav Har'El <[email protected]> wrote: > Thanks. > A general git comment: It's useful to have the header file in the patch > *before* the .cc file, for easier understanding of what's to come. > You can do this for all your projects by putting in your ~/.gitconfig the > lines: > > [diff] > renames = copies > orderfile = /home/nyh/.gitorderfile > > (the "renames" thing is unrelated, but also useful) > And then in ~/.gitorderfile something like: > > *.hh > *.cc > * > > > -- > Nadav Har'El > [email protected] > > On Fri, Sep 2, 2016 at 3:24 PM, Benoit Canet <benoit.canet.contrib@gmail. > com> wrote: > >> The go c-shared object need the libc to pass them >> the argc and argv parameters in order to initialize >> the runtime properly. >> >> This is a preparation patch for this. >> >> Signed-off-by: Benoît Canet <[email protected]> >> --- >> core/app.cc | 65 ++++++++++++++++++++++++++++-- >> ------------------------ >> include/osv/app.hh | 8 +++++++ >> 2 files changed, 42 insertions(+), 31 deletions(-) >> >> diff --git a/core/app.cc b/core/app.cc >> index ad3145f..515096e 100644 >> --- a/core/app.cc >> +++ b/core/app.cc >> @@ -294,20 +294,31 @@ void application::main() >> // _entry_point() doesn't return >> } >> >> -void application::run_main(std::string path, int argc, char** argv) >> +void application::prepare_argc_argv() >> > > Please add a comment on what this function actually prepares - what it > reads > and what results it generates. It's not clear to me why _args for example > is a > class member, and not simply a parameter of prepare_argc_argv? > > It appears it also writes "program_invocation_name" and other stuff, and > nobody can guess that. > > { >> - char *c_path = (char *)(path.c_str()); >> + // C main wants mutable arguments, so we have can't use strings >> directly >> + transform(_args, back_inserter(_mut_args), >> + [](std::string s) { return std::vector<char>(s.data(), >> s.data() + s.size() + 1); }); >> + std::vector<char*> argv; >> + transform(_mut_args.begin(), _mut_args.end(), back_inserter(argv), >> + [](std::vector<char>& s) { return s.data(); }); >> + _argc = argv.size(); >> + argv.push_back(nullptr); >> + >> + char **argv_data = argv.data(); >> + >> + char *c_path = (char *)(_command.c_str()); >> // path is guaranteed to keep existing this function >> program_invocation_name = c_path; >> program_invocation_short_name = basename(c_path); >> >> - auto sz = argc; // for the trailing 0's. >> - for (int i = 0; i < argc; ++i) { >> - sz += strlen(argv[i]); >> + auto sz = _argc; // for the trailing 0's. >> + for (int i = 0; i < _argc; ++i) { >> + sz += strlen(argv_data[i]); >> > > I'm not sure I understand why this change is needed... Would argv[i] not > have worked too instead of that "argv_data[i]"? > > >> } >> >> - std::unique_ptr<char []> argv_buf(new char[sz]); >> - char *ab = argv_buf.get(); >> + _argv_buf.reset(new char[sz]); >> + char *ab = _argv_buf.get(); >> > > I'm completely lost now why we need: args, argv, argv_data, argv_buf, ab, > contig_argv. > I would bet we could cut the number of those by half to make it easier to > review this code :-) > > Note I don't mind the performance hit - it's completely not important. > It's just that I can't > understand this code :-( > > >> // In Linux, the pointer arrays argv[] and envp[] are continguous. >> // Unfortunately, some programs rely on this fact (e.g., libgo's >> // runtime_goenvs_unix()) so it is useful that we do this too. >> @@ -315,44 +326,36 @@ void application::run_main(std::string path, int >> argc, char** argv) >> while (environ[envcount]) { >> envcount++; >> } >> - char *contig_argv[argc + 1 + envcount + 1]; >> + _contig_argv.reset(new char*[_argc + 1 + envcount + 1]); >> + char **contig_argv = _contig_argv.get(); >> >> - for (int i = 0; i < argc; ++i) { >> - size_t asize = strlen(argv[i]); >> - memcpy(ab, argv[i], asize); >> + for (int i = 0; i < _argc; ++i) { >> + size_t asize = strlen(argv_data[i]); >> + memcpy(ab, argv_data[i], asize); >> ab[asize] = '\0'; >> contig_argv[i] = ab; >> ab += asize + 1; >> } >> - contig_argv[argc] = nullptr; >> + contig_argv[_argc] = nullptr; >> >> for (int i = 0; i < envcount; i++) { >> - contig_argv[argc + 1 + i] = environ[i]; >> + contig_argv[_argc + 1 + i] = environ[i]; >> } >> - contig_argv[argc + 1 + envcount] = nullptr; >> - >> - // make sure to have a fresh optind across calls >> - // FIXME: fails if run() is executed in parallel >> - int old_optind = optind; >> - optind = 0; >> - _return_code = _main(argc, contig_argv); >> - optind = old_optind; >> + contig_argv[_argc + 1 + envcount] = nullptr; >> } >> >> void application::run_main() >> { >> trace_app_main(this, _command.c_str()); >> >> - // C main wants mutable arguments, so we have can't use strings >> directly >> - std::vector<std::vector<char>> mut_args; >> - transform(_args, back_inserter(mut_args), >> - [](std::string s) { return std::vector<char>(s.data(), >> s.data() + s.size() + 1); }); >> - std::vector<char*> argv; >> - transform(mut_args.begin(), mut_args.end(), back_inserter(argv), >> - [](std::vector<char>& s) { return s.data(); }); >> - auto argc = argv.size(); >> - argv.push_back(nullptr); >> - run_main(_command, argc, argv.data()); >> + prepare_argc_argv(); >> + >> + // make sure to have a fresh optind across calls >> + // FIXME: fails if run() is executed in parallel >> + int old_optind = optind; >> + optind = 0; >> + _return_code = _main(_argc, _contig_argv.get()); >> + optind = old_optind; >> >> if (_return_code) { >> debug("program %s returned %d\n", _command.c_str(), >> _return_code); >> diff --git a/include/osv/app.hh b/include/osv/app.hh >> index 6fa503a..3ecf1a8 100644 >> --- a/include/osv/app.hh >> +++ b/include/osv/app.hh >> @@ -194,6 +194,7 @@ private: >> void start_and_join(waiter* setup_waiter); >> void main(); >> void run_main(std::string path, int argc, char** argv); >> + void prepare_argc_argv(); >> void run_main(); >> friend void ::__libc_start_main(int(*)(int, char**), int, char**, >> void(*)(), >> void(*)(), void(*)(), void*); >> @@ -214,6 +215,13 @@ private: >> void (*_entry_point)(); >> static app_registry apps; >> >> + // argc and argv variables >> + std::vector<std::vector<char>> _mut_args; >> + std::unique_ptr<char *> _contig_argv; >> + std::unique_ptr<char []> _argv_buf; >> + std::vector<char*> _argv; >> + int _argc; >> > > Do we really need all four (!) of these argv variants to be in the app > object? The code above suggests that the various argv variants are just > temporary and what matters at the end is just the _contig_argv? > > If that is the case, please make only _contig_argv available - and perhaps > just call it just _argv. But would be nice to have a comment about what > this variable contains because the meaning of this "contiguous" thing is > less than obvious. > > + >> // Must be destroyed before _lib, because contained function objects >> may >> // have destructors which are part of the application's code. >> std::list<std::function<void()>> _termination_request_callbacks; >> -- >> 2.7.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 [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.
