On Mon, Oct 16, 2017 at 4:30 AM, Justin Cinkelj <justin.cink...@xlab.si> wrote:
> The previous patch (namespace: enable per namespace alarm/signal) > implemented per elf namespace signals for common usecase of alarms. > > But, as Nadav explained, it failed to allow application in a separate > ELF namespace to install its own handler for signals related to a > page fault, division by zero, and similar exceptions. > See also https://groups.google.com/forum/#!topic/osv-dev/AlYM5i4laPM > > The apps/apache-spark is one such example. It installs its own handler > for SIGSEGV. Without this patch, the app started in initial/main ELF > namespace only. But it failed to start in a new ELF namespace. > > This patch fixes this. Instead of always using signal_actions variable, > we ask current application/ELF namespace for its copy of signal_actions, > and use that one when generating signal. > Thanks. Your implementation is even nicer than I suggested (you wierdified a low-level function to get the array, keeping the more interesting functions sane). But I have a few suggestions for further improvement below: > > Signed-off-by: Justin Cinkelj <justin.cink...@xlab.si> > --- > core/app.cc | 9 +++++++++ > include/osv/app.hh | 11 +++++++++++ > libc/signal.cc | 13 ++++++++++--- > 3 files changed, 30 insertions(+), 3 deletions(-) > > diff --git a/core/app.cc b/core/app.cc > index 325ae820..acd9f611 100644 > --- a/core/app.cc > +++ b/core/app.cc > @@ -18,6 +18,7 @@ > #include <boost/range/algorithm/transform.hpp> > #include <osv/wait_record.hh> > #include "libc/pthread.hh" > +#include "libc/signal.hh" > > using namespace boost::range; > > @@ -480,6 +481,14 @@ void application::clone_osv_environ() > } > } > > +struct sigaction* application::get_signal_actions() { > + if (_libsignal == nullptr) { > + // running in initial ELF namespace > + return signal_actions; > + } > + return _libsignal->lookup<struct sigaction>("_ZN3osv14signal_ > actionsE"); > Looking at this implementation made me think: If we already have the "app" structure, and use its "_libsignal" field, why not add another app field, like signal_actions? I.e., when setting _libsignal you could also set _signal_actions - using the same lookup you did here - and then use it in the get_signal_actions method. The benefit would be to avoid the relatively complex and slow lookup code on every signal. This is not critical (I don't think there's a huge number of signals), but may make a difference when there are plenty of SIGSEGV signals (I was worried this is the case in Java, I don't remember if it's true) - what do you think? Another thing is that I'm not happy about the need for the mangled name in this code - the mangling scheme may change in future versions of the compiler or different architectures or different compilers or whatever. One thing we could do is to move the signal_actions out of the osv namespace so it will have a normal non-mangled name. Another idea could be to have an "extern C" function in signal.cc, say, get_signal_actions() (this will be in signal.cc, not app.cc!) which returns osv::signal_actions (which could be made "static", I don't know why it isn't now...) - and then the code which sets app._signal_actions will lookup and call this function instead of looking up a variable directly.. > +} > + > void application::set_environ(const std::string &key, const std::string > &value, > bool new_program) > { > diff --git a/include/osv/app.hh b/include/osv/app.hh > index d3015515..0c3149fb 100644 > --- a/include/osv/app.hh > +++ b/include/osv/app.hh > @@ -19,6 +19,7 @@ > #include <list> > #include <unordered_map> > #include <string> > +#include <api/signal.h> > > extern "C" void __libc_start_main(int(*)(int, char**), int, char**, > void(*)(), > void(*)(), void(*)(), void*); > @@ -183,6 +184,16 @@ public: > std::shared_ptr<elf::object> lib() const { return _lib; } > > elf::program *program(); > + > + /** > + * Returns signal handlers of the current app (elf namespace). > + * > + * This is used to implement per-elf-namespace signal handling, > + * when signal is called from the OSv kernel (for example, when > + * application in a separate elf namespace generates SIGSEGV, and has > + * installed a handler). > + */ > + struct sigaction* get_signal_actions(); > private: > void new_program(); > void clone_osv_signal(); > diff --git a/libc/signal.cc b/libc/signal.cc > index 1992ff88..b65bff2a 100644 > --- a/libc/signal.cc > +++ b/libc/signal.cc > @@ -18,6 +18,7 @@ > #include <osv/clock.hh> > #include <api/setjmp.h> > #include <osv/stubbing.hh> > +#include <osv/app.hh> > > using namespace osv::clock::literals; > > @@ -123,11 +124,17 @@ void generate_signal(siginfo_t &siginfo, > exception_frame* ef) > // needs to be running to generate them. So definitely not > waiting. > abort(); > } > - if (is_sig_dfl(signal_actions[siginfo.si_signo])) { > + > + // Application in elf namespace have different signal handler than > main > + // OSv kernel. Use it if it is installed. > + auto app_signal_actions = application::get_current(). > get()->get_signal_actions(); > Is this ".get()" necessary? Wouldn't just get_current()->get_signal_actions() do the right thing? BTW, I don't remember, does get_current() always work? Don't we need to check if it's zero or something? (I really don't remember and I'm not looking at the code now). > + assert(app_signal_actions); > + > + if (is_sig_dfl(app_signal_actions[siginfo.si_signo])) { > // Our default is to abort the process > abort(); > - } else if(!is_sig_ign(signal_actions[siginfo.si_signo])) { > - arch::build_signal_frame(ef, siginfo, signal_actions[siginfo.si_ > signo]); > + } else if(!is_sig_ign(app_signal_actions[siginfo.si_signo])) { > + arch::build_signal_frame(ef, siginfo, > app_signal_actions[siginfo.si_signo]); > } > } > > -- > 2.13.6 > > -- > 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.
