This patch makes osv_execve() wait a little bit longer before returning -
until the thread id it returns is only set, but also this thread has a new
app_runtime set.
I thought of at least a dozen ways to do this, each uglier than the next,
so I ended up with the solution which I considered least ugly, but is still
not pretty... osv_execve() now uses application::run_and_join() directly,
instead of the trivial osv::run() wrapper. The run_and_join() function
received a new parameter - a "waiter" object that it wakes when the
app_runtime is set - and osv_execve() waits on this object.
We use the "waiter" idiom (see wait_record.hh) to ensure that it is ok for
the parent thread (running osv_execve()) to request waiting before or after
the child thread (running run_and_join()) waking it up.
CAVEAT EMPTOR: After osv_execve() returns the returned thread will have the
expected new app_runtime(). However, if the application exits, run_and_join()
restores the previous app_runtime() which becomes visible for a very brief
period until the thread exits. Fixing this will require yet another ugly
workaround, so I wanted to ask first whether it is actually a problem for
our only know use case.
Signed-off-by: Nadav Har'El <n...@scylladb.com>
---
include/osv/app.hh | 7 +++++--
core/app.cc | 11 ++++++++---
core/osv_execve.cc | 23 ++++++++++++-----------
3 files changed, 25 insertions(+), 16 deletions(-)
diff --git a/include/osv/app.hh b/include/osv/app.hh
index dbdf1da..6fa503a 100644
--- a/include/osv/app.hh
+++ b/include/osv/app.hh
@@ -25,6 +25,8 @@
extern "C" void __libc_start_main(int(*)(int, char**), int, char**, void(*)(),
void(*)(), void(*)(), void*);
+class waiter;
+
namespace osv {
class application;
@@ -131,7 +133,8 @@ public:
static shared_app_t run_and_join(const std::string& command,
const std::vector<std::string>& args,
bool new_program = false,
- const std::unordered_map<std::string, std::string> *env = nullptr);
+ const std::unordered_map<std::string, std::string> *env = nullptr,
+ waiter* setup_waiter = nullptr);
/**
* Installs a termination callback which will be called when
@@ -188,7 +191,7 @@ private:
return shared_from_this();
}
void start();
- void start_and_join();
+ void start_and_join(waiter* setup_waiter);
void main();
void run_main(std::string path, int argc, char** argv);
void run_main();
diff --git a/core/app.cc b/core/app.cc
index bf6ff68..56fd18b 100644
--- a/core/app.cc
+++ b/core/app.cc
@@ -16,6 +16,7 @@
#include <errno.h>
#include <algorithm>
#include <boost/range/algorithm/transform.hpp>
+#include <osv/wait_record.hh>
using namespace boost::range;
@@ -138,10 +139,11 @@ void run(const std::vector<std::string>& args) {
shared_app_t application::run_and_join(const std::string& command,
const std::vector<std::string>& args,
bool new_program,
- const std::unordered_map<std::string, std::string> *env)
+ const std::unordered_map<std::string, std::string> *env,
+ waiter* setup_waiter)
{
auto app = std::make_shared<application>(command, args, new_program, env);
- app->start_and_join();
+ app->start_and_join(setup_waiter);
return app;
}
@@ -235,7 +237,7 @@ int application::join()
return _return_code;
}
-void application::start_and_join()
+void application::start_and_join(waiter* setup_waiter)
{
// We start the new application code in the current thread. We temporarily
// change the app_runtime pointer of this thread, while keeping the old
@@ -244,6 +246,9 @@ void application::start_and_join()
auto original_app = sched::thread::current()->app_runtime();
sched::thread::current()->set_app_runtime(runtime());
auto original_name = sched::thread::current()->name();
+ if (setup_waiter) {
+ setup_waiter->wake();
+ }
_thread = pthread_self(); // may be null if the caller is not a pthread.
main();
sched::thread::current()->set_name(original_name);
diff --git a/core/osv_execve.cc b/core/osv_execve.cc
index a7594b3..218cc78 100644
--- a/core/osv_execve.cc
+++ b/core/osv_execve.cc
@@ -2,6 +2,7 @@
#include <osv/debug.hh>
#include <osv/condvar.h>
#include <osv/osv_execve.h>
+#include <osv/wait_record.hh>
#include <thread>
/* Record thread state changes (termination) by storing exit status into a map.
@@ -19,19 +20,16 @@ static int thread_run_app_in_namespace(std::string filename,
const std::unordered_map<std::string,
std::string> envp,
long* thread_id,
int notification_fd,
- sched::thread* parent)
+ waiter* parent_waiter)
{
- int ret;
const bool new_program = true; // run in new ELF namespace
long tid = gettid(); // sched::thread::current()->id();
debugf_execve("thread_run_app_in_namespace... tid=%ld\n", tid);
- if (thread_id) {
- parent->wake_with([&] { *thread_id = tid; });
- }
+ *thread_id = tid;
- // An additional new thread is created by osv::run and caller is blocked
- osv::run(filename, args, &ret, new_program, &envp);
+ auto app = osv::application::run_and_join(filename, args, new_program,
&envp, parent_waiter);
+ auto ret = app->get_return_code();
debugf_execve("thread_run_app_in_namespace ret = %d tid=%ld\n", ret, tid);
WITH_LOCK(exec_mutex) {
@@ -113,14 +111,17 @@ int osv_execve(const char *path, char *const argv[], char
*const envp[],
std::vector<std::string> args = argv_to_array(argv);
std::unordered_map<std::string, std::string> envp_map = envp_to_map(envp);
+ waiter w(sched::thread::current());
+ // If need to set thread_id, wait until the newly created thread sets it
+ // and also sets the new app_runtime on this thread.
+ waiter* wp = thread_id ? &w : nullptr;
std::thread th = std::thread(thread_run_app_in_namespace,
std::move(filename), std::move(args), std::move(envp_map),
- thread_id, notification_fd, sched::thread::current());
+ thread_id, notification_fd, wp);
// detach from thread so that no join needes to be called.
th.detach();
- // If need to set thread_id, wait until the newly created thread sets it
- if (thread_id) {
- sched::thread::wait_until([&] { return *thread_id != 0; });
+ if (wp) {
+ wp->wait();
}
return 0;
--
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 osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
