add two flags to dd_opendir()

DD_CLOSE_ON_OPEN_ERR - free dump_dir structure when opening dump_dir
does not exist

DD_FAIL_QUIETLY - suppress message when dump directory does not exist

VERB1 log(_("Unable to open debug dump '%s'"), pDebugDumpDir); is all
removed because there is error_msg("'%s' does not exist", dd->dd_dir);
in dd_opendir() which sometimes we don't want to print(DD_FAIL_QUIETLY)

example: crash dump directory trimming code running concurrently.
Second process may try to delete
a directory which is already gone. it should not complain
that it is missing.

Signed-off-by: Nikola Pajkovsky <npajkovs@redhat.com>
Acked-off-by: Denys Vlasenko <dvlasenk@redhat.com>

9 files changed, 46 insertions, 99 deletions

diff --git a/inc/dump_dir.h b/inc/dump_dir.h
index 8e65b1f2..ef9ef7a7 100644
--- a/inc/dump_dir.h
+++ b/inc/dump_dir.h
@@ -26,6 +26,11 @@
 extern "C" {
 #endif
 
+enum {
+    DD_CLOSE_ON_OPEN_ERR    = (1 << 0),
+    DD_FAIL_QUIETLY         = (1 << 1),
+};
+
 struct dump_dir {
     char *dd_dir;
     DIR *next_dir;
@@ -37,7 +42,7 @@ struct dump_dir {
 struct dump_dir *dd_init(void);
 void dd_close(struct dump_dir *dd);
 
-int dd_opendir(struct dump_dir *dd, const char *dir);
+int dd_opendir(struct dump_dir *dd, const char *dir, int flags);
 int dd_exist(struct dump_dir *dd, const char *path);
 int dd_create(struct dump_dir *dd, const char *dir, uid_t uid);
 DIR *dd_init_next_file(struct dump_dir *dd);
diff --git a/lib/plugins/CCpp.cpp b/lib/plugins/CCpp.cpp
index c4a1f08c..041c85b8 100644
--- a/lib/plugins/CCpp.cpp
+++ b/lib/plugins/CCpp.cpp
@@ -209,12 +209,8 @@ static void GetIndependentBuildIdPC(const char *unstrip_n_output,
 static char* run_unstrip_n(const char *pDebugDumpDir, unsigned timeout_sec)
 {
     struct dump_dir *dd = dd_init();
-    if (!dd_opendir(dd, pDebugDumpDir))
-    {
-        dd_close(dd);
-        VERB1 log(_("Unable to open debug dump '%s'"), pDebugDumpDir);
+    if (!dd_opendir(dd, pDebugDumpDir, DD_CLOSE_ON_OPEN_ERR))
         return NULL;
-    }
 
     char *uid = dd_load_text(dd, CD_UID);
     dd_close(dd);
@@ -403,12 +399,8 @@ static void trim_debuginfo_cache(unsigned max_mb)
 string CAnalyzerCCpp::GetLocalUUID(const char *pDebugDumpDir)
 {
     struct dump_dir *dd = dd_init();
-    if (!dd_opendir(dd, pDebugDumpDir))
-    {
-        dd_close(dd);
-        VERB1 log(_("Unable to open debug dump '%s'"), pDebugDumpDir);
+    if (!dd_opendir(dd, pDebugDumpDir, DD_CLOSE_ON_OPEN_ERR))
         return string("");
-    }
 
     char *executable = dd_load_text(dd, FILENAME_EXECUTABLE);
     char *package = dd_load_text(dd, FILENAME_PACKAGE);
@@ -464,12 +456,8 @@ string CAnalyzerCCpp::GetLocalUUID(const char *pDebugDumpDir)
 string CAnalyzerCCpp::GetGlobalUUID(const char *pDebugDumpDir)
 {
     struct dump_dir *dd = dd_init();
-    if (!dd_opendir(dd, pDebugDumpDir))
-    {
-        dd_close(dd);
-        VERB1 log(_("Unable to open debug dump '%s'"), pDebugDumpDir);
+    if (!dd_opendir(dd, pDebugDumpDir, DD_CLOSE_ON_OPEN_ERR))
         return string("");
-    }
 
     if (dd_exist(dd, FILENAME_GLOBAL_UUID))
     {
@@ -640,12 +628,8 @@ void CAnalyzerCCpp::CreateReport(const char *pDebugDumpDir, int force)
     }
 
     struct dump_dir *dd = dd_init();
-    if (!dd_opendir(dd, pDebugDumpDir))
-    {
-        dd_close(dd);
-        VERB1 log(_("Unable to open debug dump '%s'"), pDebugDumpDir);
+    if (!dd_opendir(dd, pDebugDumpDir, DD_CLOSE_ON_OPEN_ERR))
         return;
-    }
 
     if (!force)
     {
@@ -688,11 +672,9 @@ void CAnalyzerCCpp::CreateReport(const char *pDebugDumpDir, int force)
     gen_backtrace(pDebugDumpDir, m_sDebugInfoDirs.c_str(), m_nGdbTimeoutSec);
 
     dd = dd_init();
-    if (!dd_opendir(dd, pDebugDumpDir))
+    if (!dd_opendir(dd, pDebugDumpDir, DD_CLOSE_ON_OPEN_ERR))
     {
-        dd_close(dd);
         free(build_ids);
-        VERB1 log(_("Unable to open debug dump '%s'"), pDebugDumpDir);
         return;
     }
 
diff --git a/lib/plugins/Kerneloops.cpp b/lib/plugins/Kerneloops.cpp
index 0c4b1a56..d3ce2e8b 100644
--- a/lib/plugins/Kerneloops.cpp
+++ b/lib/plugins/Kerneloops.cpp
@@ -126,12 +126,8 @@ std::string CAnalyzerKerneloops::GetLocalUUID(const char *pDebugDumpDir)
     VERB3 log("Getting local universal unique identification");
 
     struct dump_dir *dd = dd_init();
-    if (!dd_opendir(dd, pDebugDumpDir))
-    {
-        dd_close(dd);
-        VERB1 log(_("Unable to open debug dump '%s'"), pDebugDumpDir);
+    if (!dd_opendir(dd, pDebugDumpDir, DD_CLOSE_ON_OPEN_ERR))
         return std::string("");
-    }
 
     char *oops = dd_load_text(dd, FILENAME_BACKTRACE);
     unsigned hash = hash_oops_str(oops);
diff --git a/lib/plugins/Python.cpp b/lib/plugins/Python.cpp
index 9ef830e3..5e4cc02a 100644
--- a/lib/plugins/Python.cpp
+++ b/lib/plugins/Python.cpp
@@ -26,12 +26,8 @@ using namespace std;
 string CAnalyzerPython::GetLocalUUID(const char *pDebugDumpDir)
 {
     struct dump_dir *dd = dd_init();
-    if (!dd_opendir(dd, pDebugDumpDir))
-    {
-        dd_close(dd);
-        VERB1 log(_("Unable to open debug dump '%s'"), pDebugDumpDir);
+    if (!dd_opendir(dd, pDebugDumpDir, DD_CLOSE_ON_OPEN_ERR))
         return string("");
-    }
 
     char *bt = dd_load_text(dd, FILENAME_BACKTRACE);
     dd_close(dd);
diff --git a/lib/plugins/RunApp.cpp b/lib/plugins/RunApp.cpp
index 0bf5ff0c..3ae6638b 100644
--- a/lib/plugins/RunApp.cpp
+++ b/lib/plugins/RunApp.cpp
@@ -58,12 +58,8 @@ void CActionRunApp::Run(const char *pActionDir, const char *pArgs, int force)
     if (args.size() > FILENAME)
     {
         struct dump_dir *dd = dd_init();
-        if (!dd_opendir(dd, pActionDir))
-        {
-            dd_close(dd);
-            VERB1 log(_("Unable to open debug dump '%s'"), pActionDir);
+        if (!dd_opendir(dd, pActionDir, DD_CLOSE_ON_OPEN_ERR))
             return;
-        }
 
         dd_save_binary(dd, args[FILENAME].c_str(), cmd_out, cmd_out_size);
         dd_close(dd);
diff --git a/lib/plugins/SOSreport.cpp b/lib/plugins/SOSreport.cpp
index 280738d2..5c1b658a 100644
--- a/lib/plugins/SOSreport.cpp
+++ b/lib/plugins/SOSreport.cpp
@@ -51,12 +51,8 @@ void CActionSOSreport::Run(const char *pActionDir, const char *pArgs, int force)
     if (!force)
     {
         struct dump_dir *dd = dd_init();
-        if (!dd_opendir(dd, pActionDir))
-        {
-            dd_close(dd);
-            VERB1 log(_("Unable to open debug dump '%s'"), pActionDir);
+        if (!dd_opendir(dd, pActionDir, DD_CLOSE_ON_OPEN_ERR))
             return;
-        }
 
         bool bt_exists = dd_exist(dd, "sosreport.tar.bz2") || dd_exist(dd, "sosreport.tar.xz");
         if (bt_exists)
diff --git a/lib/utils/dump_dir.c b/lib/utils/dump_dir.c
index 2602eded..d800c692 100644
--- a/lib/utils/dump_dir.c
+++ b/lib/utils/dump_dir.c
@@ -25,16 +25,6 @@
 
 // TODO:
 //
-// dd_opendir needs a bit flags parameter, with bits for
-// "auto-close on error" and "log error message on error".
-// This will simplify a ton of places which do this:
-//    if (!dd_opendir(dd, dirname))
-//    {
-//        dd_close(dd);
-//        VERB1 log(_("Unable to open debug dump '%s'"), dirname);
-//        return ........;
-//    }
-//
 // Perhaps dd_opendir should do some sanity checking like
 // "if there is no "uid" file in the directory, it's not a crash dump",
 // and fail.
@@ -99,7 +89,7 @@ void dd_close(struct dump_dir *dd)
     free(dd);
 }
 
-int dd_opendir(struct dump_dir *dd, const char *dir)
+int dd_opendir(struct dump_dir *dd, const char *dir, int flags)
 {
     if (dd->locked)
         error_msg_and_die("dump_dir is already opened"); /* bug */
@@ -107,8 +97,13 @@ int dd_opendir(struct dump_dir *dd, const char *dir)
     dd->dd_dir = rm_trailing_slashes(dir);
     if (!exist_file_dir(dd->dd_dir))
     {
-        error_msg("'%s' does not exist", dd->dd_dir);
-        free(dd->dd_dir);
+
+        if (!(flags & DD_FAIL_QUIETLY))
+            error_msg("'%s' does not exist", dd->dd_dir);
+
+        if (flags & DD_CLOSE_ON_OPEN_ERR)
+            dd_close(dd);
+
         return 0;
     }
 
@@ -485,9 +480,8 @@ int dd_get_next_file(struct dump_dir *dd, char **short_name, char **full_name)
 void delete_debug_dump_dir(const char *dd_dir)
 {
     struct dump_dir *dd = dd_init();
-    if (dd_opendir(dd, dd_dir))
+    if (dd_opendir(dd, dd_dir, 0))
         dd_delete(dd);
-    else
-        VERB1 log("Unable to open debug dump '%s'", dd_dir);
+
     dd_close(dd);
 }
diff --git a/src/daemon/MiddleWare.cpp b/src/daemon/MiddleWare.cpp
index 73959102..b3f7ca8a 100644
--- a/src/daemon/MiddleWare.cpp
+++ b/src/daemon/MiddleWare.cpp
@@ -189,28 +189,25 @@ static bool DebugDumpToCrashReport(const char *pDebugDumpDir, map_crash_data_t&
     VERB3 log(" DebugDumpToCrashReport('%s')", pDebugDumpDir);
 
     struct dump_dir *dd = dd_init();
-    if (dd_opendir(dd, pDebugDumpDir))
+    if (!dd_opendir(dd, pDebugDumpDir, DD_CLOSE_ON_OPEN_ERR))
+        return false;
+
+    const char *const *v = must_have_files;
+    while (*v)
     {
-        const char *const *v = must_have_files;
-        while (*v)
+        if (!dd_exist(dd, *v))
         {
-            if (!dd_exist(dd, *v))
-            {
-                dd_close(dd);
-                throw CABRTException(EXCEP_ERROR, "DebugDumpToCrashReport(): important file '%s' is missing", *v);
-            }
-
-            v++;
+            dd_close(dd);
+            throw CABRTException(EXCEP_ERROR, "DebugDumpToCrashReport(): important file '%s' is missing", *v);
         }
 
-        load_crash_data_from_debug_dump(dd, pCrashData);
-        dd_close(dd);
-
-        return true;
+        v++;
     }
 
+    load_crash_data_from_debug_dump(dd, pCrashData);
     dd_close(dd);
-    return false;
+
+    return true;
 }
 
 /**
@@ -305,9 +302,8 @@ mw_result_t CreateCrashReport(const char *crash_id,
     try
     {
         struct dump_dir *dd = dd_init();
-        if (!dd_opendir(dd, row->db_dump_dir))
+        if (!dd_opendir(dd, row->db_dump_dir, DD_CLOSE_ON_OPEN_ERR))
         {
-            dd_close(dd);
             db_row_free(row);
             return MW_ERROR;
         }
@@ -462,7 +458,7 @@ report_status_t Report(const map_crash_data_t& client_report,
     if (comment || reproduce || backtrace)
     {
         struct dump_dir *dd = dd_init();
-        if (dd_opendir(dd, pDumpDir.c_str()))
+        if (dd_opendir(dd, pDumpDir.c_str(), 0))
         {
             if (comment)
             {
@@ -732,11 +728,8 @@ static mw_result_t SavePackageDescriptionToDebugDump(
                 VERB2 log("Crash in unpackaged executable '%s', proceeding without packaging information", pExecutable);
 
                 struct dump_dir *dd = dd_init();
-                if (!dd_opendir(dd, pDebugDumpDir))
-                {
-                    dd_close(dd);
+                if (!dd_opendir(dd, pDebugDumpDir, DD_CLOSE_ON_OPEN_ERR))
                     return MW_ERROR;
-                }
 
                 dd_save_text(dd, FILENAME_PACKAGE, "");
                 dd_save_text(dd, FILENAME_COMPONENT, "");
@@ -857,7 +850,7 @@ static mw_result_t SavePackageDescriptionToDebugDump(
     }
 
     struct dump_dir *dd = dd_init();
-    if (dd_opendir(dd, pDebugDumpDir))
+    if (dd_opendir(dd, pDebugDumpDir, 0))
     {
         if (rpm_pkg)
         {
@@ -1048,11 +1041,8 @@ mw_result_t SaveDebugDump(const char *pDebugDumpDir,
     int remote = 0;
 
     struct dump_dir *dd = dd_init();
-    if (!dd_opendir(dd, pDebugDumpDir))
-    {
-        dd_close(dd);
+    if (!dd_opendir(dd, pDebugDumpDir, DD_CLOSE_ON_OPEN_ERR))
         return MW_ERROR;
-    }
 
     char *time = dd_load_text(dd, FILENAME_TIME);
     char *uid = dd_load_text(dd, CD_UID);
@@ -1139,9 +1129,8 @@ mw_result_t FillCrashInfo(const char *crash_id,
         return MW_ERROR;
 
     struct dump_dir *dd = dd_init();
-    if (!dd_opendir(dd, row->db_dump_dir))
+    if (!dd_opendir(dd, row->db_dump_dir, DD_CLOSE_ON_OPEN_ERR))
     {
-        dd_close(dd);
         db_row_free(row);
         return MW_ERROR;
     }
diff --git a/src/daemon/abrt-action-generate-backtrace.c b/src/daemon/abrt-action-generate-backtrace.c
index 8bedf544..46f002a4 100644
--- a/src/daemon/abrt-action-generate-backtrace.c
+++ b/src/daemon/abrt-action-generate-backtrace.c
@@ -303,12 +303,8 @@ int main(int argc, char **argv)
     }
 
     struct dump_dir *dd = dd_init();
-    if (!dd_opendir(dd, dump_dir_name))
-    {
-        dd_close(dd);
-        VERB1 log(_("Unable to open debug dump '%s'"), dump_dir_name);
+    if (!dd_opendir(dd, dump_dir_name, DD_CLOSE_ON_OPEN_ERR))
         return 1;
-    }
 
     char *package = dd_load_text(dd, FILENAME_PACKAGE);
     char *executable = dd_load_text(dd, FILENAME_EXECUTABLE);
@@ -323,12 +319,9 @@ int main(int argc, char **argv)
     }
 
     dd = dd_init();
-    if (!dd_opendir(dd, dump_dir_name))
-    {
-        dd_close(dd);
-        VERB1 log(_("Unable to open debug dump '%s'"), dump_dir_name);
+    if (!dd_opendir(dd, dump_dir_name, DD_CLOSE_ON_OPEN_ERR))
         return 1;
-    }
+
     dd_save_text(dd, FILENAME_BACKTRACE, backtrace_str);
 
     /* Compute and store backtrace hash. */