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.
