abrtd: eliminate g_pending_jobs[] map and all corresponding TODOs :)

-6k:

   text   data    bss    dec    hexfilename
 194741   2656   2384 199781  30c65abrt.t1/abrt-0.0.8.5/src/Daemon/.libs/abrtd
 188316   2648   2320 193284  2f304abrt.t2/abrt-0.0.8.5/src/Daemon/.libs/abrtd

Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>

9 files changed, 112 insertions, 104 deletions

diff --git a/src/Daemon/CommLayerServer.h b/src/Daemon/CommLayerServer.h
index 9beb945a..d2d2c4ec 100644
--- a/src/Daemon/CommLayerServer.h
+++ b/src/Daemon/CommLayerServer.h
@@ -14,11 +14,12 @@ class CCommLayerServer {
 
         /* just stubs to be called when not implemented in specific comm layer */
         virtual void Crash(const std::string& progname, const std::string& uid) {}
+        virtual void JobDone(const char* pDest, const char* pUUID) = 0;
+        virtual void JobStarted(const char* pDest) {};
+
         virtual void Error(const std::string& arg1) {}
         virtual void Update(const std::string& pMessage, uint64_t pJobID) {};
         virtual void Warning(const std::string& pMessage) {};
-        virtual void JobDone(const char* pDest, uint64_t pJobID) = 0;
-        virtual void JobStarted(const char* pDest, uint64_t pJobID) {};
         virtual void Warning(const std::string& pMessage, uint64_t pJobID) {};
 };
 
diff --git a/src/Daemon/CommLayerServerDBus.cpp b/src/Daemon/CommLayerServerDBus.cpp
index 41f5abab..1be957b8 100644
--- a/src/Daemon/CommLayerServerDBus.cpp
+++ b/src/Daemon/CommLayerServerDBus.cpp
@@ -367,31 +367,32 @@ void CCommLayerServerDBus::Crash(const std::string& progname, const std::string&
     send_flush_and_unref(msg);
 }
 
-void CCommLayerServerDBus::JobDone(const char* pDest, uint64_t job_id)
+void CCommLayerServerDBus::JobStarted(const char* pDest)
 {
-    DBusMessage* msg = new_signal_msg("JobDone");
+    DBusMessage* msg = new_signal_msg("JobStarted");
     /* send unicast dbus signal */
     if (!dbus_message_set_destination(msg, pDest))
         die_out_of_memory();
+    uint64_t nJobID = uint64_t(pthread_self());
     dbus_message_append_args(msg,
             DBUS_TYPE_STRING, &pDest, /* TODO: redundant parameter, remove from API */
-            DBUS_TYPE_UINT64, &job_id,
+            DBUS_TYPE_UINT64, &nJobID, /* TODO: redundant parameter, remove from API */
             DBUS_TYPE_INVALID);
-    VERB2 log("Sending signal JobDone('%s',%llx)", pDest, (unsigned long long)job_id);
+    VERB2 log("Sending signal JobStarted('%s',%llx)", pDest, (unsigned long long)nJobID);
     send_flush_and_unref(msg);
 }
 
-void CCommLayerServerDBus::JobStarted(const char* pDest, uint64_t job_id)
+void CCommLayerServerDBus::JobDone(const char* pDest, const char* pUUID)
 {
-    DBusMessage* msg = new_signal_msg("JobStarted");
+    DBusMessage* msg = new_signal_msg("JobDone");
     /* send unicast dbus signal */
     if (!dbus_message_set_destination(msg, pDest))
         die_out_of_memory();
     dbus_message_append_args(msg,
             DBUS_TYPE_STRING, &pDest, /* TODO: redundant parameter, remove from API */
-            DBUS_TYPE_UINT64, &job_id,
+            DBUS_TYPE_STRING, &pUUID, /* TODO: redundant parameter, remove from API */
             DBUS_TYPE_INVALID);
-    VERB2 log("Sending signal JobStarted('%s',%llx)", pDest, (unsigned long long)job_id);
+    VERB2 log("Sending signal JobDone('%s','%s')", pDest, pUUID);
     send_flush_and_unref(msg);
 }
 
@@ -493,17 +494,47 @@ static int handle_CreateReport(DBusMessage* call, DBusMessage* reply)
 
     const char* sender;
     long unix_uid = get_remote_uid(call, &sender);
-    VERB1 log("got %s('%s') call from uid %ld", "CreateReport", pUUID, unix_uid);
-    uint64_t argout1 = CreateReport_t(pUUID, to_string(unix_uid).c_str(), sender);
+    VERB1 log("got %s('%s') call from sender '%s' uid %ld", "CreateReport", pUUID, sender, unix_uid);
+    if (CreateReportThread(pUUID, to_string(unix_uid).c_str(), sender) != 0)
+        return -1; /* can't create thread (err msg is already logged) */
 
     dbus_message_append_args(reply,
-                DBUS_TYPE_UINT64, &argout1,
+                DBUS_TYPE_STRING, &pUUID, /* redundant, eliminate from API */
                 DBUS_TYPE_INVALID);
 
     send_flush_and_unref(reply);
     return 0;
 }
 
+static int handle_GetJobResult(DBusMessage* call, DBusMessage* reply)
+{
+    const char* pUUID;
+    DBusMessageIter in_iter;
+    if (!dbus_message_iter_init(call, &in_iter))
+    {
+        error_msg("dbus call %s: no parameters", "GetJobResult");
+        return -1;
+    }
+    int r = load_val(&in_iter, pUUID);
+    if (r != LAST_FIELD)
+    {
+        if (r == MORE_FIELDS)
+            error_msg("dbus call %s: extra parameters", "GetJobResult");
+        return -1;
+    }
+
+    long unix_uid = get_remote_uid(call);
+    VERB1 log("got %s('%s') call from uid %ld", "GetJobResult", pUUID, unix_uid);
+    map_crash_report_t report = GetJobResult(pUUID, to_string(unix_uid).c_str());
+
+    DBusMessageIter out_iter;
+    dbus_message_iter_init_append(reply, &out_iter);
+    store_val(&out_iter, report);
+
+    send_flush_and_unref(reply);
+    return 0;
+}
+
 static int handle_Report(DBusMessage* call, DBusMessage* reply)
 {
     map_crash_report_t argin1;
@@ -575,35 +606,6 @@ static int handle_DeleteDebugDump(DBusMessage* call, DBusMessage* reply)
     return 0;
 }
 
-static int handle_GetJobResult(DBusMessage* call, DBusMessage* reply)
-{
-    uint64_t job_id;
-    DBusMessageIter in_iter;
-    if (!dbus_message_iter_init(call, &in_iter))
-    {
-        error_msg("dbus call %s: no parameters", "GetJobResult");
-        return -1;
-    }
-    int r = load_val(&in_iter, job_id);
-    if (r != LAST_FIELD)
-    {
-        if (r == MORE_FIELDS)
-            error_msg("dbus call %s: extra parameters", "GetJobResult");
-        return -1;
-    }
-
-    long unix_uid = get_remote_uid(call);
-    VERB1 log("got %s(%llx) call from uid %ld", "GetJobResult", (long long)job_id, unix_uid);
-    map_crash_report_t report = GetJobResult(job_id, to_string(unix_uid));
-
-    DBusMessageIter out_iter;
-    dbus_message_iter_init_append(reply, &out_iter);
-    store_val(&out_iter, report);
-
-    send_flush_and_unref(reply);
-    return 0;
-}
-
 static int handle_GetPluginsInfo(DBusMessage* call, DBusMessage* reply)
 {
     vector_map_string_t plugins_info = g_pPluginManager->GetPluginsInfo();
diff --git a/src/Daemon/CommLayerServerDBus.h b/src/Daemon/CommLayerServerDBus.h
index e205d913..979823d3 100644
--- a/src/Daemon/CommLayerServerDBus.h
+++ b/src/Daemon/CommLayerServerDBus.h
@@ -12,11 +12,11 @@ class CCommLayerServerDBus
 
         /* DBus signal senders */
         virtual void Crash(const std::string& progname, const std::string& uid);
+        virtual void JobStarted(const char* pDest);
+        virtual void JobDone(const char* pDest, const char* pUUID);
+
         virtual void Error(const std::string& arg1);
         virtual void Update(const std::string& pMessage, uint64_t pJobID);
-        //the job id should be enough in jobdone
-        virtual void JobDone(const char* pDest, uint64_t pJobID);
-        virtual void JobStarted(const char* pDest, uint64_t pJobID);
         virtual void Warning(const std::string& pMessage);
         virtual void Warning(const std::string& pMessage, uint64_t pJobID);
 };
diff --git a/src/Daemon/CommLayerServerSocket.cpp b/src/Daemon/CommLayerServerSocket.cpp
index ad14666c..a3240a8d 100644
--- a/src/Daemon/CommLayerServerSocket.cpp
+++ b/src/Daemon/CommLayerServerSocket.cpp
@@ -148,7 +148,7 @@ void CCommLayerServerSocket::ProcessMessage(const std::string& pMessage, GIOChan
     {
 //        std::string UUID = pMessage.substr(sizeof(MESSAGE_CREATE_REPORT) - 1);
 //        map_crash_report_t crashReport = CreateReport(UUID, UID);
-//use CreateReport_t instead of CreateReport?
+//use CreateReportThread instead of CreateReport?
 //        std::string message = MESSAGE_CREATE_REPORT + crash_report_to_string(crashReport);
 //        Send(message, pSource);
     }
@@ -220,7 +220,7 @@ vector_crash_infos_t CCommLayerServerSocket::GetCrashInfos(const std::string &pS
     return crashInfos;
 }
 
-//reimplement as CreateReport_t(...)?
+//reimplement as CreateReportThread(...)?
 //map_crash_report_t CCommLayerServerSocket::CreateReport(const std::string &pUUID,const std::string &pSender)
 //{
 //    map_crash_report_t crashReport;
diff --git a/src/Daemon/CommLayerServerSocket.h b/src/Daemon/CommLayerServerSocket.h
index 3fae1871..9d4f21cf 100644
--- a/src/Daemon/CommLayerServerSocket.h
+++ b/src/Daemon/CommLayerServerSocket.h
@@ -31,6 +31,7 @@ class CCommLayerServerSocket : public CCommLayerServer
         virtual bool DeleteDebugDump(const std::string& pUUID, const std::string& pSender);
 
         virtual void Crash(const std::string& arg1);
+        virtual void JobStarted(const char* pDest) {};
+
         virtual void Error(const std::string& arg1);
-        virtual void JobStarted(const char* pDest, uint64_t pJobID) {};
 };
diff --git a/src/Daemon/CrashWatcher.cpp b/src/Daemon/CrashWatcher.cpp
index 1b17af69..1b147ed1 100644
--- a/src/Daemon/CrashWatcher.cpp
+++ b/src/Daemon/CrashWatcher.cpp
@@ -102,48 +102,60 @@ vector_crash_infos_t GetCrashInfos(const std::string &pUID)
     return retval;
 }
 
+/*
+ * "GetJobResult" is a bit of a misnomer.
+ * It actually _creates_ a_ report_ and returns the result.
+ * It is called in two cases:
+ * (1) by CreateReport dbus call -> CreateReportThread(), in the thread
+ * (2) by GetJobResult dbus call
+ * In the second case, it finishes quickly, because previous
+ * CreateReport dbus call already did all the processing, and we just retrieve
+ * the result from dump directory, which is fast.
+ */
+map_crash_report_t GetJobResult(const char* pUUID, const char* pUID)
+{
+    map_crash_info_t crashReport;
+
+    mw_result_t res = CreateCrashReport(pUUID, pUID, crashReport);
+    switch (res)
+    {
+        case MW_OK:
+            break;
+        case MW_IN_DB_ERROR:
+            warn_client(std::string("Did not find crash with UUID ") + pUUID + " in database");
+            break;
+        case MW_PLUGIN_ERROR:
+            warn_client("Particular analyzer plugin isn't loaded or there is an error within plugin(s)");
+            break;
+        case MW_CORRUPTED:
+        case MW_FILE_ERROR:
+        default:
+            warn_client(std::string("Corrupted crash with UUID ") + pUUID + ", deleting");
+            std::string debugDumpDir = DeleteCrashInfo(pUUID, pUID);
+            DeleteDebugDumpDir(debugDumpDir);
+            break;
+    }
+    return crashReport;
+}
+
 typedef struct thread_data_t {
     pthread_t thread_id;
     char* UUID;
     char* UID;
     char* dest;
 } thread_data_t;
-static void *create_report(void *arg)
+static void* create_report(void* arg)
 {
     thread_data_t *thread_data = (thread_data_t *) arg;
-    map_crash_info_t crashReport;
 
-    g_pCommLayer->JobStarted(thread_data->dest, uint64_t(thread_data->thread_id));
+    g_pCommLayer->JobStarted(thread_data->dest);
 
-    log("Creating report...");
     try
     {
-        mw_result_t res;
-        res = CreateCrashReport(thread_data->UUID, thread_data->UID, crashReport);
-        switch (res)
-        {
-            case MW_OK:
-                break;
-            case MW_IN_DB_ERROR:
-                warn_client(std::string("Did not find crash with UUID ") + thread_data->UUID + " in database");
-                break;
-            case MW_PLUGIN_ERROR:
-                warn_client(std::string("Particular analyzer plugin isn't loaded or there is an error within plugin(s)"));
-                break;
-            case MW_CORRUPTED:
-            case MW_FILE_ERROR:
-            default:
-                warn_client(std::string("Corrupted crash with UUID ") + thread_data->UUID + ", deleting");
-                std::string debugDumpDir = DeleteCrashInfo(thread_data->UUID, thread_data->UID);
-                DeleteDebugDumpDir(debugDumpDir);
-                break;
-        }
-        /* only one thread can write */
-        pthread_mutex_lock(&g_pJobsMutex);
-        g_pending_jobs[std::string(thread_data->UID)][uint64_t(thread_data->thread_id)] = crashReport;
-        pthread_mutex_unlock(&g_pJobsMutex);
-
-        g_pCommLayer->JobDone(thread_data->dest, uint64_t(thread_data->thread_id));
+	/* "GetJobResult" is a bit of a misnomer */
+        log("Creating report...");
+        map_crash_info_t crashReport = GetJobResult(thread_data->UUID, thread_data->UID);
+        g_pCommLayer->JobDone(thread_data->dest, thread_data->UUID);
     }
     catch (CABRTException& e)
     {
@@ -167,13 +179,18 @@ static void *create_report(void *arg)
     /* Bogus value. pthreads require us to return void* */
     return NULL;
 }
-uint64_t CreateReport_t(const char* pUUID, const char* pUID, const char* pSender)
+int CreateReportThread(const char* pUUID, const char* pUID, const char* pSender)
 {
     thread_data_t *thread_data = (thread_data_t *)xzalloc(sizeof(thread_data_t));
     thread_data->UUID = xstrdup(pUUID);
     thread_data->UID = xstrdup(pUID);
     thread_data->dest = xstrdup(pSender);
-    if (pthread_create(&thread_data->thread_id, NULL, create_report, (void *)thread_data) != 0)
+//TODO: do we need this?
+//pthread_attr_t attr;
+//pthread_attr_init(&attr);
+//pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
+    int r = pthread_create(&thread_data->thread_id, NULL, create_report, thread_data);
+    if (r != 0)
     {
         free(thread_data->UUID);
         free(thread_data->UID);
@@ -182,10 +199,14 @@ uint64_t CreateReport_t(const char* pUUID, const char* pUID, const char* pSender
         /* The only reason this may happen is system-wide resource starvation,
          * or ulimit is exceeded (someone floods us with CreateReport() dbus calls?)
          */
-        error_msg("cannot create thread");
-        return 0;
+        error_msg("Can't create thread");
+    }
+    else
+    {
+        VERB3 log("Thread %llx created", (unsigned long long)thread_data->thread_id);
     }
-    return uint64_t(thread_data->thread_id);
+//pthread_attr_destroy(&attr);
+    return r;
 }
 
 bool DeleteDebugDump(const std::string& pUUID, const std::string& pUID)
@@ -208,12 +229,3 @@ bool DeleteDebugDump(const std::string& pUUID, const std::string& pUID)
     }
     return true;
 }
-
-map_crash_report_t GetJobResult(uint64_t pJobID, const std::string& pSender)
-{
-    /* FIXME: once we return the result, we should remove it from map to free memory
-       - use some TTL to clean the memory even if client won't get it
-       - if we don't find it in the cache we should try to ask MW to get it again??
-    */
-    return g_pending_jobs[pSender][pJobID];
-}
diff --git a/src/Daemon/CrashWatcher.h b/src/Daemon/CrashWatcher.h
index f35e1001..bd98d826 100644
--- a/src/Daemon/CrashWatcher.h
+++ b/src/Daemon/CrashWatcher.h
@@ -49,8 +49,8 @@ class CCrashWatcher
 };
 
 vector_crash_infos_t GetCrashInfos(const std::string &pUID);
-uint64_t CreateReport_t(const char* pUUID, const char* pUID, const char* pSender);
+int CreateReportThread(const char* pUUID, const char* pUID, const char* pSender);
+map_crash_report_t GetJobResult(const char* pUUID, const char* pUID);
 bool DeleteDebugDump(const std::string& pUUID, const std::string& pUID);
-map_crash_report_t GetJobResult(uint64_t pJobID, const std::string& pSender);
 
 #endif /*CRASHWATCHER_H_*/
diff --git a/src/Daemon/Daemon.cpp b/src/Daemon/Daemon.cpp
index 149b2bda..f680471f 100644
--- a/src/Daemon/Daemon.cpp
+++ b/src/Daemon/Daemon.cpp
@@ -53,7 +53,7 @@
  *      v[N]["executable"/"uid"/"kernel"/"backtrace"][N] = "contents"
  * - CreateReport(UUID): starts creating a report for /var/cache/abrt/DIR with this UUID
  *      Returns job id (uint64)
- * - GetJobResult(job_id): returns map_crash_report_t (map_vector_string_t)
+ * - GetJobResult(UUID): returns map_crash_report_t (map_vector_string_t)
  * - Report(map_crash_report_t (map_vector_string_t)):
  *      "Please report this crash": calls Report() of all registered reporter plugins
  *      Returns report_status_t (map_vector_string_t) - the status of each call
@@ -98,12 +98,7 @@ static GMainLoop* g_pMainloop;
 
 int g_verbose;
 CCommLayerServer* g_pCommLayer;
-/*
- * Map to cache the results from CreateReport_t
- * <UID, <job_id, result>>
- */
-std::map<const std::string, std::map<uint64_t, map_crash_report_t> > g_pending_jobs;
-/* mutex to protect g_pending_jobs */
+
 pthread_mutex_t g_pJobsMutex;
 
 
diff --git a/src/Daemon/Daemon.h b/src/Daemon/Daemon.h
index 8fcce717..e03d983a 100644
--- a/src/Daemon/Daemon.h
+++ b/src/Daemon/Daemon.h
@@ -49,9 +49,6 @@ extern CPluginManager* g_pPluginManager;
  */
 extern set_string_t g_setBlackList;
 
-/* Map <UID, <job_id, result>> to cache the results from CreateReport_t() */
-extern std::map<const std::string, std::map<uint64_t, map_crash_report_t> > g_pending_jobs;
-/* Mutex to protect g_pending_jobs */
 extern pthread_mutex_t g_pJobsMutex;
 
 #endif