DD_CLOSE_ON_OPEN_ERR flag indicate to free all memory allocated by dd_init() which is passed to dd_opendir().
dd_opendir fails when dump_dir 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().
There are two places where condition was inverted.
Signed-off-by: Nikola Pajkovsky npajkovs@redhat.com --- inc/dump_dir.h | 6 ++- lib/plugins/CCpp.cpp | 28 ++------- lib/plugins/Kerneloops.cpp | 6 +-- lib/plugins/Python.cpp | 6 +-- lib/plugins/RunApp.cpp | 6 +-- lib/plugins/SOSreport.cpp | 6 +-- lib/utils/dump_dir.c | 21 ++----- src/daemon/MiddleWare.cpp | 88 +++++++++++--------------- src/daemon/abrt-action-generate-backtrace.c | 13 +--- 9 files changed, 60 insertions(+), 120 deletions(-)
diff --git a/inc/dump_dir.h b/inc/dump_dir.h index 8e65b1f..f02e1a5 100644 --- a/inc/dump_dir.h +++ b/inc/dump_dir.h @@ -26,6 +26,10 @@ extern "C" { #endif
+enum { + DD_CLOSE_ON_OPEN_ERR = 1 << 0, +}; + struct dump_dir { char *dd_dir; DIR *next_dir; @@ -37,7 +41,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 c4a1f08..041c85b 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 0c4b1a5..d3ce2e8 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 9ef830e..5e4cc02 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 0bf5ff0..3ae6638 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 280738d..5c1b658 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 2602ede..f81e7c9 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 */ @@ -108,7 +98,9 @@ int dd_opendir(struct dump_dir *dd, const char *dir) if (!exist_file_dir(dd->dd_dir)) { error_msg("'%s' does not exist", dd->dd_dir); - free(dd->dd_dir); + if (flags & DD_CLOSE_ON_OPEN_ERR) + dd_close(dd); + return 0; }
@@ -485,9 +477,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 ee35369..bfab9f0 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) { @@ -738,11 +734,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, ""); @@ -863,35 +856,32 @@ static mw_result_t SavePackageDescriptionToDebugDump( }
struct dump_dir *dd = dd_init(); - if (dd_opendir(dd, pDebugDumpDir)) + if (!dd_opendir(dd, pDebugDumpDir, DD_CLOSE_ON_OPEN_ERR)) + return MW_ERROR; + + if (rpm_pkg) { - if (rpm_pkg) - { - dd_save_text(dd, FILENAME_PACKAGE, rpm_pkg); - free(rpm_pkg); - } + dd_save_text(dd, FILENAME_PACKAGE, rpm_pkg); + free(rpm_pkg); + }
- if (dsc) - { - dd_save_text(dd, FILENAME_DESCRIPTION, dsc); - free(dsc); - } + if (dsc) + { + dd_save_text(dd, FILENAME_DESCRIPTION, dsc); + free(dsc); + }
- if (component) - { - dd_save_text(dd, FILENAME_COMPONENT, component); - free(component); - } + if (component) + { + dd_save_text(dd, FILENAME_COMPONENT, component); + free(component); + }
- if (!remote) - dd_save_text(dd, FILENAME_HOSTNAME, host); + if (!remote) + dd_save_text(dd, FILENAME_HOSTNAME, host);
- dd_close(dd); - return MW_OK; - } dd_close(dd); - - return MW_ERROR; + return MW_OK; }
bool analyzer_has_InformAllUsers(const char *analyzer_name) @@ -1054,11 +1044,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); @@ -1143,9 +1130,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 8bedf54..46f002a 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. */
On Tue, 2010-09-21 at 11:59 +0200, Nikola Pajkovsky wrote:
DD_CLOSE_ON_OPEN_ERR flag indicate to free all memory allocated by dd_init() which is passed to dd_opendir().
dd_opendir fails when dump_dir 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 (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.)
Can you add a DD_FAIL_QUIETLY flag to make it possible to suppress that message?
@@ -863,35 +856,32 @@ static mw_result_t SavePackageDescriptionToDebugDump( }
struct dump_dir *dd = dd_init();
- if (dd_opendir(dd, pDebugDumpDir))
- if (!dd_opendir(dd, pDebugDumpDir, DD_CLOSE_ON_OPEN_ERR))
return MW_ERROR;- if (rpm_pkg) {
if (rpm_pkg){dd_save_text(dd, FILENAME_PACKAGE, rpm_pkg);free(rpm_pkg);}
dd_save_text(dd, FILENAME_PACKAGE, rpm_pkg);free(rpm_pkg);- }
if (dsc){dd_save_text(dd, FILENAME_DESCRIPTION, dsc);free(dsc);}
- if (dsc)
- {
dd_save_text(dd, FILENAME_DESCRIPTION, dsc);free(dsc);- }
if (component){dd_save_text(dd, FILENAME_COMPONENT, component);free(component);}
- if (component)
- {
dd_save_text(dd, FILENAME_COMPONENT, component);free(component);- }
if (!remote)dd_save_text(dd, FILENAME_HOSTNAME, host);
- if (!remote)
dd_save_text(dd, FILENAME_HOSTNAME, host);
dd_close(dd);return MW_OK;- } dd_close(dd);
- return MW_ERROR;
- return MW_OK;
Can you make the change in this hunk less invasive (that is, without if() statement restructuring)?
My other unrelated patch which I just posted touches the same place. Simpler change will help me to port the patch...
Looks good to me.
On 09/22/2010 01:16 AM, Denys Vlasenko wrote:
On Tue, 2010-09-21 at 11:59 +0200, Nikola Pajkovsky wrote:
DD_CLOSE_ON_OPEN_ERR flag indicate to free all memory allocated by dd_init() which is passed to dd_opendir().
dd_opendir fails when dump_dir 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 (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.)
Can you add a DD_FAIL_QUIETLY flag to make it possible to suppress that message?
yep
@@ -863,35 +856,32 @@ static mw_result_t SavePackageDescriptionToDebugDump( }
struct dump_dir *dd = dd_init();
- if (dd_opendir(dd, pDebugDumpDir))
- if (!dd_opendir(dd, pDebugDumpDir, DD_CLOSE_ON_OPEN_ERR))
return MW_ERROR;- if (rpm_pkg) {
if (rpm_pkg){dd_save_text(dd, FILENAME_PACKAGE, rpm_pkg);free(rpm_pkg);}
dd_save_text(dd, FILENAME_PACKAGE, rpm_pkg);free(rpm_pkg);- }
if (dsc){dd_save_text(dd, FILENAME_DESCRIPTION, dsc);free(dsc);}
- if (dsc)
- {
dd_save_text(dd, FILENAME_DESCRIPTION, dsc);free(dsc);- }
if (component){dd_save_text(dd, FILENAME_COMPONENT, component);free(component);}
- if (component)
- {
dd_save_text(dd, FILENAME_COMPONENT, component);free(component);- }
if (!remote)dd_save_text(dd, FILENAME_HOSTNAME, host);
- if (!remote)
dd_save_text(dd, FILENAME_HOSTNAME, host);
dd_close(dd);return MW_OK;- } dd_close(dd);
- return MW_ERROR;
- return MW_OK;
Can you make the change in this hunk less invasive (that is, without if() statement restructuring)?
My other unrelated patch which I just posted touches the same place. Simpler change will help me to port the patch...
ok no problem
Looks good to me.
I will send another patch.
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 --- inc/dump_dir.h | 7 +++- lib/plugins/CCpp.cpp | 28 +++------------- lib/plugins/Kerneloops.cpp | 6 +--- lib/plugins/Python.cpp | 6 +--- lib/plugins/RunApp.cpp | 6 +--- lib/plugins/SOSreport.cpp | 6 +--- lib/utils/dump_dir.c | 26 ++++++--------- src/daemon/MiddleWare.cpp | 47 ++++++++++---------------- src/daemon/abrt-action-generate-backtrace.c | 13 ++------ 9 files changed, 46 insertions(+), 99 deletions(-)
diff --git a/inc/dump_dir.h b/inc/dump_dir.h index 8e65b1f..ef9ef7a 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 c4a1f08..041c85b 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 0c4b1a5..d3ce2e8 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 9ef830e..5e4cc02 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 0bf5ff0..3ae6638 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 280738d..5c1b658 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 2602ede..d800c69 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 7395910..b3f7ca8 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 8bedf54..46f002a 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. */
On Wed, 2010-09-22 at 12:08 +0200, Nikola Pajkovsky wrote:
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.
Looks good. Thanks!
crash-catcher@lists.fedorahosted.org
-
Denys Vlasenko -
Nikola Pajkovsky