On Sat, Sep 3, 2016 at 10:46 PM, Nadav Har'El <[email protected]> wrote:

>
> On Fri, Sep 2, 2016 at 5:35 PM, Benoit Canet <benoit.canet.contrib@gmail.
> com> wrote:
>
>> This will allow go program to get the vdso library address.
>>
>> Signed-off-by: Benoît Canet <[email protected]>
>> ---
>>  core/app.cc        | 29 ++++++++++++++++++++++++++---
>>  include/osv/app.hh |  8 +++++++-
>>  2 files changed, 33 insertions(+), 4 deletions(-)
>>
>> diff --git a/core/app.cc b/core/app.cc
>> index 2c3257b..f2665fc 100644
>> --- a/core/app.cc
>> +++ b/core/app.cc
>> @@ -173,7 +173,7 @@ application::application(const std::string& command,
>>          }
>>
>>          merge_in_environ(new_program, env);
>> -        prepare_argc_argv();
>> +        prepare_argc_argv(current_program);
>>          char **argv = _contig_argv.get();
>>          std::vector<std::string> extra_path;
>>          _lib = current_program->get_library(_command, extra_path,
>> _argc, argv);
>> @@ -297,7 +297,7 @@ void application::main()
>>      // _entry_point() doesn't return
>>  }
>>
>> -void application::prepare_argc_argv()
>> +void application::prepare_argc_argv(elf::program *current_program)
>>
>
> I don't remember the details, but we also have a _program member in
> application, and also a get_program() function, so do we really need to
> pass current_program() again?
>
>
current program is the right one wether we are starting a new_program or
not.
It factorise the comparison to just pass it here.


>
>
>>  {
>>      // C main wants mutable arguments, so we have can't use strings
>> directly
>>      transform(_args, back_inserter(_mut_args),
>> @@ -329,7 +329,7 @@ void application::prepare_argc_argv()
>>      while (environ[envcount]) {
>>          envcount++;
>>      }
>> -    _contig_argv.reset(new char*[_argc + 1 + envcount + 1]);
>> +    _contig_argv.reset(new char*[_argc + 1 + envcount + 1 + 1]);
>>      char **contig_argv = _contig_argv.get();
>>
>>      for (int i = 0; i < _argc; ++i) {
>> @@ -345,6 +345,29 @@ void application::prepare_argc_argv()
>>          contig_argv[_argc + 1 + i] = environ[i];
>>      }
>>      contig_argv[_argc + 1 + envcount] = nullptr;
>> +
>> +    if (_command == std::string("/zpool.so")       ||
>> +        _command == std::string("/libzfs.so")      ||
>> +        _command == std::string("/libuutil.so")    ||
>> +        _command == std::string("/zfs.so")         ||
>> +        _command == std::string("/tools/mkfs.so")  ||
>> +        _command == std::string("/tools/cpiod.so")) {
>> +        return;
>>
>
> What's special about these particular commands?
>

They are used to inject vdso.so in the image before it's loadable.
But your next comment will solve this.


>
>
>> +    }
>> +
>> +    _libvdso = current_program->get_library("libvdso.so");
>>
> +    if (!_libvdso) {
>> +        abort("could not load libvdso.so\n");
>> +        return;
>> +    }
>> +
>> +    _auxv[0].a_type = AT_SYSINFO_EHDR;
>> +    _auxv[0].a_un.a_val = reinterpret_cast<uint64_t>(_libvdso->base());
>>
>
> I'm not very familiar with VDSO details, but must it be a separate shared
> library?
>
> Couldn't we give it the kernel's core instead? I.e., use _core instead of
> _libvdso. _core->base() exists too - it is normaly ELF_IMAGE_START,  which
> is set in the Makefile but defaults to 0x200000, but you wouldn't need to
> care about these details, and just use _core->base(). Wouldn't that be
> valid too?
>

As long a base is not a nullptr it's doable (go check for nullptr)


>
>
+
>> +    _auxv[1].a_type = AT_NULL;
>> +    _auxv[1].a_un.a_val = 0;
>> +
>> +    contig_argv[_argc + 1 + envcount + 1] = reinterpret_cast<char
>> *>(_auxv);
>>  }
>>
>>  void application::run_main()
>> diff --git a/include/osv/app.hh b/include/osv/app.hh
>> index 3ecf1a8..231fb6e 100644
>> --- a/include/osv/app.hh
>> +++ b/include/osv/app.hh
>> @@ -22,6 +22,10 @@
>>  #include <unordered_map>
>>  #include <string>
>>
>> +#include "musl/include/elf.h"
>>
>
> Why not just #include <elf.h>? We have include/api/elf.h (linking to this
> musl file) so that would work.
>

Yes it's cleaner.

Thanks for reviewing.


>
>
+#undef AT_UID // prevent collisions
>> +#undef AT_GID
>>
> +
>>  extern "C" void __libc_start_main(int(*)(int, char**), int, char**,
>> void(*)(),
>>      void(*)(), void(*)(), void*);
>>
>> @@ -194,7 +198,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 prepare_argc_argv(elf::program *current_program);
>>      void run_main();
>>      friend void ::__libc_start_main(int(*)(int, char**), int, char**,
>> void(*)(),
>>          void(*)(), void(*)(), void*);
>> @@ -211,6 +215,8 @@ private:
>>      mutex _termination_mutex;
>>      std::shared_ptr<elf::object> _lib;
>>      std::shared_ptr<elf::object> _libenviron;
>> +    std::shared_ptr<elf::object> _libvdso;
>> +    Elf32_auxv_t _auxv[2];
>>      main_func_t* _main;
>>      void (*_entry_point)();
>>      static app_registry apps;
>> --
>> 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.

Reply via email to