From 14156a33ac0233f4233546080d6b975618b331fa Mon Sep 17 00:00:00 2001 From: Michael Lotz Date: Wed, 26 Aug 2015 21:39:15 +0200 Subject: [PATCH] launch_daemon: Delegate launch data replies to Job. Previously the LaunchDaemon would send out its own team id when a given job was not yet launched, leading to invalid BMessengers once the port owner changed to the actually launched team. The launch of the target team and the launch data replies were also not synchronized, which could lead to the launched team getting a reply pointing to the launch_daemon when requesting data for itself. This is the case for the BRoster init of the registrar. The fix in hrev49561 therefore didn't always work, because the registrar would sometimes get the launch_daemon team id instead of the id of itself. It would later try talking to the launch_daemon, which obviously never replied, leading to #12237. The LaunchDaemon now delegates the launch data reply to the Job instead. The Job either replies directly, in case it has already been launched, or queues the reply for when the launch completes. This causes launch data requesters to block until the launch attempt is completed, but won't block the LaunchDaemon message loop. This commit introduces the seperate fLaunchStatus to properly handle the ambiguity of fTeam being < 0, which is the case for both, when no launch was attempted and when the launch failed. This new field now determines what IsLaunched() returns and how launch data replies are handled. The new launch status is additionally protected by the launch status lock, which will later probably be made broader in scope to protect against race conditions once service monitoring is implemented. --- src/servers/launch/Job.cpp | 85 ++++++++++++++++++++++++++--- src/servers/launch/Job.h | 13 +++++ src/servers/launch/LaunchDaemon.cpp | 34 ++++++------ 3 files changed, 107 insertions(+), 25 deletions(-) diff --git a/src/servers/launch/Job.cpp b/src/servers/launch/Job.cpp index 7432921aaf..5cb9c985b2 100644 --- a/src/servers/launch/Job.cpp +++ b/src/servers/launch/Job.cpp @@ -27,8 +27,11 @@ Job::Job(const char* name) fCreateDefaultPort(false), fInitStatus(B_NO_INIT), fTeam(-1), - fTarget(NULL) + fLaunchStatus(B_NO_INIT), + fTarget(NULL), + fPendingLaunchDataReplies(0, false) { + mutex_init(&fLaunchStatusLock, "launch status lock"); } @@ -40,8 +43,12 @@ Job::Job(const Job& other) fCreateDefaultPort(other.CreateDefaultPort()), fInitStatus(B_NO_INIT), fTeam(-1), - fTarget(other.Target()) + fLaunchStatus(B_NO_INIT), + fTarget(other.Target()), + fPendingLaunchDataReplies(0, false) { + mutex_init(&fLaunchStatusLock, "launch status lock"); + fCondition = other.fCondition; // TODO: copy events //fEvent = other.fEvent; @@ -343,16 +350,21 @@ Job::Launch() // Launch by signature BString signature("application/"); signature << Name(); - return BRoster::Private().Launch(signature.String(), NULL, NULL, - 0, NULL, &environment[0], &fTeam); + + status_t status = BRoster::Private().Launch(signature.String(), NULL, + NULL, 0, NULL, &environment[0], &fTeam); + _SetLaunchStatus(status); + return status; } // Build argument vector entry_ref ref; status_t status = get_ref_for_path(fArguments.StringAt(0).String(), &ref); - if (status != B_OK) + if (status != B_OK) { + _SetLaunchStatus(status); return status; + } std::vector args; @@ -365,15 +377,28 @@ Job::Launch() } // Launch via entry_ref - return BRoster::Private().Launch(NULL, &ref, NULL, count, &args[0], + status = BRoster::Private().Launch(NULL, &ref, NULL, count, &args[0], &environment[0], &fTeam); + _SetLaunchStatus(status); + return status; } bool Job::IsLaunched() const { - return fTeam >= 0; + return fLaunchStatus != B_NO_INIT; +} + + +status_t +Job::HandleGetLaunchData(BMessage* message) +{ + MutexLocker launchLocker(fLaunchStatusLock); + if (IsLaunched()) + return _SendLaunchDataReply(message); + + return fPendingLaunchDataReplies.AddItem(message) ? B_OK : B_NO_MEMORY; } @@ -434,3 +459,49 @@ Job::_AddStringList(std::vector& array, const BStringList& list) array.push_back(list.StringAt(index).String()); } } + + +void +Job::_SetLaunchStatus(status_t launchStatus) +{ + MutexLocker launchLocker(fLaunchStatusLock); + fLaunchStatus = launchStatus != B_NO_INIT ? launchStatus : B_ERROR; + launchLocker.Unlock(); + + _SendPendingLaunchDataReplies(); +} + + +status_t +Job::_SendLaunchDataReply(BMessage* message) +{ + BMessage reply(fTeam < 0 ? fTeam : (uint32)B_OK); + if (reply.what == B_OK) { + reply.AddInt32("team", fTeam); + + PortMap::const_iterator iterator = fPortMap.begin(); + for (; iterator != fPortMap.end(); iterator++) { + BString name; + if (iterator->second.HasString("name")) + name << iterator->second.GetString("name") << "_"; + name << "port"; + + reply.AddInt32(name.String(), + iterator->second.GetInt32("port", -1)); + } + } + + message->SendReply(&reply); + delete message; + return B_OK; +} + + +void +Job::_SendPendingLaunchDataReplies() +{ + for (int32 i = 0; i < fPendingLaunchDataReplies.CountItems(); i++) + _SendLaunchDataReply(fPendingLaunchDataReplies.ItemAt(i)); + + fPendingLaunchDataReplies.MakeEmpty(); +} diff --git a/src/servers/launch/Job.h b/src/servers/launch/Job.h index e2ee0b4d0f..63afeae97a 100644 --- a/src/servers/launch/Job.h +++ b/src/servers/launch/Job.h @@ -15,6 +15,8 @@ #include #include +#include + using namespace BSupportKit; class BMessage; @@ -68,6 +70,8 @@ public: status_t Launch(); bool IsLaunched() const; + status_t HandleGetLaunchData(BMessage* message); + protected: virtual status_t Execute(); @@ -78,6 +82,11 @@ private: void _AddStringList(std::vector& array, const BStringList& list); + void _SetLaunchStatus(status_t launchStatus); + + status_t _SendLaunchDataReply(BMessage* message); + void _SendPendingLaunchDataReplies(); + private: BStringList fArguments; BStringList fRequirements; @@ -87,8 +96,12 @@ private: PortMap fPortMap; status_t fInitStatus; team_id fTeam; + status_t fLaunchStatus; + mutex fLaunchStatusLock; ::Target* fTarget; ::Condition* fCondition; + BObjectList + fPendingLaunchDataReplies; }; diff --git a/src/servers/launch/LaunchDaemon.cpp b/src/servers/launch/LaunchDaemon.cpp index b64d40aead..f9733f916c 100644 --- a/src/servers/launch/LaunchDaemon.cpp +++ b/src/servers/launch/LaunchDaemon.cpp @@ -483,31 +483,29 @@ LaunchDaemon::_HandleGetLaunchData(BMessage* message) launchJob = false; } } - } + } else + launchJob = false; + bool ownsMessage = false; if (reply.what == B_OK) { - // If the job has not been launched yet, we'll pass on our - // team here. The rationale behind this is that this team - // will temporarily own the synchronous reply ports. - reply.AddInt32("team", job->Team() < 0 - ? current_team() : job->Team()); - - PortMap::const_iterator iterator = job->Ports().begin(); - for (; iterator != job->Ports().end(); iterator++) { - BString name; - if (iterator->second.HasString("name")) - name << iterator->second.GetString("name") << "_"; - name << "port"; - - reply.AddInt32(name.String(), - iterator->second.GetInt32("port", -1)); - } - // Launch the job if it hasn't been launched already if (launchJob) _LaunchJob(job); + + DetachCurrentMessage(); + status_t result = job->HandleGetLaunchData(message); + if (result == B_OK) { + // Replying is delegated to the job. + return; + } + + ownsMessage = true; + reply.what = result; } + message->SendReply(&reply); + if (ownsMessage) + delete message; }