On Sat, Sep 3, 2016 at 6:32 PM, Benoît Canet <[email protected]> wrote:
> > This patch is just code move. > But this patch takes code which had 3 temporary variables holding various transformations of the argv, and makes all of them into class variables. I didn't understand why... Why can't the prepare_argc_argv() function take parameters (instead of reading class variables) and write only the results we really need (argc and the "one argv" we really need) - and keep the temporary transformations of argv as temporaries, as in the original code? > > 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 < >> [email protected]> 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.
