On 08/04/2010 12:32 PM, Michal Toman wrote:
Please review
When I tried to generate a Python report using abrt-gui with this patch applied, abrtd started to loop repeatedly saying:
abrtd: Lock file '/var/spool/abrt/pyhook-1280938727-28545.lock' is already locked by us
I think that the reason for this might be that the try { CDebugDump dd; ... } catch {} block was also used to limit the scope of existence of CDebugDump instances, so dd's destructor was called when leaving the try catch block (the destructor unlocks the directory). AFAICT you can call dd.Close() after you are finished with particular CDebugDump instance to fix this (esp. in src/Daemon/MiddleWare.cpp).
On 08/04/2010 08:01 PM, Karel Klic wrote:
On 08/04/2010 12:32 PM, Michal Toman wrote:
Please review
When I tried to generate a Python report using abrt-gui with this patch applied, abrtd started to loop repeatedly saying:
abrtd: Lock file '/var/spool/abrt/pyhook-1280938727-28545.lock' is already locked by us
I think that the reason for this might be that the try { CDebugDump dd; ... } catch {} block was also used to limit the scope of existence of CDebugDump instances, so dd's destructor was called when leaving the try catch block (the destructor unlocks the directory). AFAICT you can call dd.Close() after you are finished with particular CDebugDump instance to fix this (esp. in src/Daemon/MiddleWare.cpp).
It seems like we're missing a lot of dd.Close() calls:
[20:07:00 jmoskovc@localhost abrt]$ grep -r "dd.Close(" * | wc 8 26 411 [20:07:29 jmoskovc@localhost abrt]$ grep -r "dd.Open(" * | wc 19 38 1070
Jirka
Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
Added dd.Close() to functions containing dd.Create() or dd.Open()
On 08/04/2010 08:01 PM, Karel Klic wrote:
On 08/04/2010 12:32 PM, Michal Toman wrote:
Please review
When I tried to generate a Python report using abrt-gui with this patch applied, abrtd started to loop repeatedly saying:
abrtd: Lock file '/var/spool/abrt/pyhook-1280938727-28545.lock' is already locked by us
I think that the reason for this might be that the try { CDebugDump dd; ... } catch {} block was also used to limit the scope of existence of CDebugDump instances, so dd's destructor was called when leaving the try catch block (the destructor unlocks the directory). AFAICT you can call dd.Close() after you are finished with particular CDebugDump instance to fix this (esp. in src/Daemon/MiddleWare.cpp). _______________________________________________ Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
Here is the review.
I tried to generate and report a Python crash, a kerneloops, and a C crash and everything seems to work.
========= lib/Plugins/CCpp.cpp The curly braces embracing CDebugDump usage can be removed on three places where the dd.Close() was added:
@@ -191,6 +191,7 @@ static void GetBacktrace(const char *pDebugDumpDir, @@ -327,6 +328,7 @@ static string run_unstrip_n(const char *pDebugDumpDir) @@ -511,6 +513,7 @@ string CAnalyzerCCpp::GetLocalUUID(const char *pDebugDumpDir)
========= lib/Plugins/KerneloopsScanner.cpp The function save_oops_to_debug_dump used to throw an exception when something went wrong with the dump directory creation. Currently the failure is not reported to upper level. I think the function should return boolean value indicating success, and its callers should check for it and log some error message so we know where the error came from.
src/Hooks/dumpoops.cpp imports save_oops_to_debug_dump from the plugin dynamic library, so the function interface must be changed there as well.
========= src/Daemon/MiddleWare.cpp In function DebugDumpToCrashReport the dd.Open() return value must be checked, as the Open() no longer throws an exception.
========= src/Hooks/abrt-hook-ccpp.cpp The indentation of comments became ugly on several places:
- /* We need coredumps to be readable by all, because - * when abrt daemon processes coredump, - * process producing backtrace is run under the same UID - * as the crashed process. - * Thus 644, not 600 */ + /* We need coredumps to be readable by all, because + * when abrt daemon processes coredump, + * process producing backtrace is run under the same UID + * as the crashed process. + * Thus 644, not 600 */
ABRT codebase contains a space between 'if' and '(':
- if (src_fd_binary > 0) + if(dd.Create(path, uid))
Dne 5.8.2010 12:52, Michal Toman napsal(a):
Added dd.Close() to functions containing dd.Create() or dd.Open()
On 08/04/2010 08:01 PM, Karel Klic wrote:
On 08/04/2010 12:32 PM, Michal Toman wrote:
Please review
When I tried to generate a Python report using abrt-gui with this patch applied, abrtd started to loop repeatedly saying:
abrtd: Lock file '/var/spool/abrt/pyhook-1280938727-28545.lock' is already locked by us
I think that the reason for this might be that the try { CDebugDump dd; ... } catch {} block was also used to limit the scope of existence of CDebugDump instances, so dd's destructor was called when leaving the try catch block (the destructor unlocks the directory). AFAICT you can call dd.Close() after you are finished with particular CDebugDump instance to fix this (esp. in src/Daemon/MiddleWare.cpp). _______________________________________________ Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
Fixed
On 08/06/2010 12:43 PM, Karel Klic wrote:
Here is the review.
I tried to generate and report a Python crash, a kerneloops, and a C crash and everything seems to work.
========= lib/Plugins/CCpp.cpp The curly braces embracing CDebugDump usage can be removed on three places where the dd.Close() was added:
@@ -191,6 +191,7 @@ static void GetBacktrace(const char *pDebugDumpDir, @@ -327,6 +328,7 @@ static string run_unstrip_n(const char *pDebugDumpDir) @@ -511,6 +513,7 @@ string CAnalyzerCCpp::GetLocalUUID(const char *pDebugDumpDir)
========= lib/Plugins/KerneloopsScanner.cpp The function save_oops_to_debug_dump used to throw an exception when something went wrong with the dump directory creation. Currently the failure is not reported to upper level. I think the function should return boolean value indicating success, and its callers should check for it and log some error message so we know where the error came from.
src/Hooks/dumpoops.cpp imports save_oops_to_debug_dump from the plugin dynamic library, so the function interface must be changed there as well.
========= src/Daemon/MiddleWare.cpp In function DebugDumpToCrashReport the dd.Open() return value must be checked, as the Open() no longer throws an exception.
========= src/Hooks/abrt-hook-ccpp.cpp The indentation of comments became ugly on several places:
/* We need coredumps to be readable by all, because* when abrt daemon processes coredump,* process producing backtrace is run under the same UID* as the crashed process.* Thus 644, not 600 */
/* We need coredumps to be readable by all, because* when abrt daemon processes coredump,* process producing backtrace is run under the same UID* as the crashed process.* Thus 644, not 600 */ABRT codebase contains a space between 'if' and '(':
if (src_fd_binary> 0)
if(dd.Create(path, uid))Dne 5.8.2010 12:52, Michal Toman napsal(a):
Added dd.Close() to functions containing dd.Create() or dd.Open()
On 08/04/2010 08:01 PM, Karel Klic wrote:
On 08/04/2010 12:32 PM, Michal Toman wrote:
Please review
When I tried to generate a Python report using abrt-gui with this patch applied, abrtd started to loop repeatedly saying:
abrtd: Lock file '/var/spool/abrt/pyhook-1280938727-28545.lock' is already locked by us
I think that the reason for this might be that the try { CDebugDump dd; ... } catch {} block was also used to limit the scope of existence of CDebugDump instances, so dd's destructor was called when leaving the try catch block (the destructor unlocks the directory). AFAICT you can call dd.Close() after you are finished with particular CDebugDump instance to fix this (esp. in src/Daemon/MiddleWare.cpp). _______________________________________________ Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
Thank you.
Here is probably the last issue: I think we should check the returned value of CDebugDump::Open() every time it's called.
$ git grep dd.Open
Not checked: lib/plugins/CCpp.cpp: dd.Open(pDebugDumpDir); lib/plugins/CCpp.cpp: dd.Open(pDebugDumpDir); lib/plugins/CCpp.cpp: dd.Open(pDebugDumpDir); lib/plugins/CCpp.cpp: dd.Open(pDebugDumpDir); lib/plugins/CCpp.cpp: dd.Open(pDebugDumpDir); lib/plugins/CCpp.cpp: dd.Open(pDebugDumpDir); lib/plugins/Kerneloops.cpp: dd.Open(pDebugDumpDir); lib/plugins/Python.cpp: dd.Open(pDebugDumpDir); lib/plugins/RunApp.cpp: dd.Open(pActionDir); lib/plugins/SOSreport.cpp: dd.Open(pActionDir); lib/plugins/SOSreport.cpp: dd.Open(pActionDir); lib/utils/DebugDump.cpp: dd.Open(pDebugDumpDir);
Checked: src/daemon/MiddleWare.cpp: if (dd.Open(pDebugDumpDir)) src/daemon/MiddleWare.cpp: if (dd.Open(row.m_sDebugDumpDir.c_str())) src/daemon/MiddleWare.cpp: if (dd.Open(pDumpDir.c_str())) src/daemon/MiddleWare.cpp: if (dd.Open(pDebugDumpDir)) src/daemon/MiddleWare.cpp: if (dd.Open(pDebugDumpDir)) src/daemon/MiddleWare.cpp: if (dd.Open(pDebugDumpDir)) src/daemon/MiddleWare.cpp: if (dd.Open(row.m_sDebugDumpDir.c_str()))
Dne 9.8.2010 11:29, Michal Toman napsal(a):
Fixed
On 08/06/2010 12:43 PM, Karel Klic wrote:
Here is the review.
I tried to generate and report a Python crash, a kerneloops, and a C crash and everything seems to work.
========= lib/Plugins/CCpp.cpp The curly braces embracing CDebugDump usage can be removed on three places where the dd.Close() was added:
@@ -191,6 +191,7 @@ static void GetBacktrace(const char *pDebugDumpDir, @@ -327,6 +328,7 @@ static string run_unstrip_n(const char *pDebugDumpDir) @@ -511,6 +513,7 @@ string CAnalyzerCCpp::GetLocalUUID(const char *pDebugDumpDir)
========= lib/Plugins/KerneloopsScanner.cpp The function save_oops_to_debug_dump used to throw an exception when something went wrong with the dump directory creation. Currently the failure is not reported to upper level. I think the function should return boolean value indicating success, and its callers should check for it and log some error message so we know where the error came from.
src/Hooks/dumpoops.cpp imports save_oops_to_debug_dump from the plugin dynamic library, so the function interface must be changed there as well.
========= src/Daemon/MiddleWare.cpp In function DebugDumpToCrashReport the dd.Open() return value must be checked, as the Open() no longer throws an exception.
========= src/Hooks/abrt-hook-ccpp.cpp The indentation of comments became ugly on several places:
- /* We need coredumps to be readable by all, because
- when abrt daemon processes coredump,
- process producing backtrace is run under the same UID
- as the crashed process.
- Thus 644, not 600 */
- /* We need coredumps to be readable by all, because
- when abrt daemon processes coredump,
- process producing backtrace is run under the same UID
- as the crashed process.
- Thus 644, not 600 */
ABRT codebase contains a space between 'if' and '(':
- if (src_fd_binary> 0)
- if(dd.Create(path, uid))
Dne 5.8.2010 12:52, Michal Toman napsal(a):
Added dd.Close() to functions containing dd.Create() or dd.Open()
On 08/04/2010 08:01 PM, Karel Klic wrote:
On 08/04/2010 12:32 PM, Michal Toman wrote:
Please review
When I tried to generate a Python report using abrt-gui with this patch applied, abrtd started to loop repeatedly saying:
abrtd: Lock file '/var/spool/abrt/pyhook-1280938727-28545.lock' is already locked by us
I think that the reason for this might be that the try { CDebugDump dd; ... } catch {} block was also used to limit the scope of existence of CDebugDump instances, so dd's destructor was called when leaving the try catch block (the destructor unlocks the directory). AFAICT you can call dd.Close() after you are finished with particular CDebugDump instance to fix this (esp. in src/Daemon/MiddleWare.cpp). _______________________________________________ Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
On 08/10/2010 01:18 PM, Karel Klic wrote:
Thank you.
Here is probably the last issue: I think we should check the returned value of CDebugDump::Open() every time it's called.
$ git grep dd.Open
Not checked: lib/plugins/CCpp.cpp: dd.Open(pDebugDumpDir); lib/plugins/CCpp.cpp: dd.Open(pDebugDumpDir); lib/plugins/CCpp.cpp: dd.Open(pDebugDumpDir); lib/plugins/CCpp.cpp: dd.Open(pDebugDumpDir); lib/plugins/CCpp.cpp: dd.Open(pDebugDumpDir); lib/plugins/CCpp.cpp: dd.Open(pDebugDumpDir); lib/plugins/Kerneloops.cpp: dd.Open(pDebugDumpDir); lib/plugins/Python.cpp: dd.Open(pDebugDumpDir); lib/plugins/RunApp.cpp: dd.Open(pActionDir); lib/plugins/SOSreport.cpp: dd.Open(pActionDir); lib/plugins/SOSreport.cpp: dd.Open(pActionDir); lib/utils/DebugDump.cpp: dd.Open(pDebugDumpDir);
Checked: src/daemon/MiddleWare.cpp: if (dd.Open(pDebugDumpDir)) src/daemon/MiddleWare.cpp: if (dd.Open(row.m_sDebugDumpDir.c_str())) src/daemon/MiddleWare.cpp: if (dd.Open(pDumpDir.c_str())) src/daemon/MiddleWare.cpp: if (dd.Open(pDebugDumpDir)) src/daemon/MiddleWare.cpp: if (dd.Open(pDebugDumpDir)) src/daemon/MiddleWare.cpp: if (dd.Open(pDebugDumpDir)) src/daemon/MiddleWare.cpp: if (dd.Open(row.m_sDebugDumpDir.c_str()))
Dne 9.8.2010 11:29, Michal Toman napsal(a):
Fixed
On 08/06/2010 12:43 PM, Karel Klic wrote:
Here is the review.
I tried to generate and report a Python crash, a kerneloops, and a C crash and everything seems to work.
========= lib/Plugins/CCpp.cpp The curly braces embracing CDebugDump usage can be removed on three places where the dd.Close() was added:
@@ -191,6 +191,7 @@ static void GetBacktrace(const char *pDebugDumpDir, @@ -327,6 +328,7 @@ static string run_unstrip_n(const char *pDebugDumpDir) @@ -511,6 +513,7 @@ string CAnalyzerCCpp::GetLocalUUID(const char *pDebugDumpDir)
========= lib/Plugins/KerneloopsScanner.cpp The function save_oops_to_debug_dump used to throw an exception when something went wrong with the dump directory creation. Currently the failure is not reported to upper level. I think the function should return boolean value indicating success, and its callers should check for it and log some error message so we know where the error came from.
src/Hooks/dumpoops.cpp imports save_oops_to_debug_dump from the plugin dynamic library, so the function interface must be changed there as well.
========= src/Daemon/MiddleWare.cpp In function DebugDumpToCrashReport the dd.Open() return value must be checked, as the Open() no longer throws an exception.
========= src/Hooks/abrt-hook-ccpp.cpp The indentation of comments became ugly on several places:
- /* We need coredumps to be readable by all, because
- when abrt daemon processes coredump,
- process producing backtrace is run under the same UID
- as the crashed process.
- Thus 644, not 600 */
- /* We need coredumps to be readable by all, because
- when abrt daemon processes coredump,
- process producing backtrace is run under the same UID
- as the crashed process.
- Thus 644, not 600 */
ABRT codebase contains a space between 'if' and '(':
- if (src_fd_binary> 0)
- if(dd.Create(path, uid))
Dne 5.8.2010 12:52, Michal Toman napsal(a):
Added dd.Close() to functions containing dd.Create() or dd.Open()
On 08/04/2010 08:01 PM, Karel Klic wrote:
On 08/04/2010 12:32 PM, Michal Toman wrote:
Please review
When I tried to generate a Python report using abrt-gui with this patch applied, abrtd started to loop repeatedly saying:
abrtd: Lock file '/var/spool/abrt/pyhook-1280938727-28545.lock' is already locked by us
I think that the reason for this might be that the try { CDebugDump dd; ... } catch {} block was also used to limit the scope of existence of CDebugDump instances, so dd's destructor was called when leaving the try catch block (the destructor unlocks the directory). AFAICT you can call dd.Close() after you are finished with particular CDebugDump instance to fix this (esp. in src/Daemon/MiddleWare.cpp). _______________________________________________ Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
The last patch looks fine to me.
On 08/12/2010 01:37 PM, Michal Toman wrote:
On 08/10/2010 01:18 PM, Karel Klic wrote:
Thank you.
Here is probably the last issue: I think we should check the returned value of CDebugDump::Open() every time it's called.
$ git grep dd.Open
Not checked: lib/plugins/CCpp.cpp: dd.Open(pDebugDumpDir); lib/plugins/CCpp.cpp: dd.Open(pDebugDumpDir); lib/plugins/CCpp.cpp: dd.Open(pDebugDumpDir); lib/plugins/CCpp.cpp: dd.Open(pDebugDumpDir); lib/plugins/CCpp.cpp: dd.Open(pDebugDumpDir); lib/plugins/CCpp.cpp: dd.Open(pDebugDumpDir); lib/plugins/Kerneloops.cpp: dd.Open(pDebugDumpDir); lib/plugins/Python.cpp: dd.Open(pDebugDumpDir); lib/plugins/RunApp.cpp: dd.Open(pActionDir); lib/plugins/SOSreport.cpp: dd.Open(pActionDir); lib/plugins/SOSreport.cpp: dd.Open(pActionDir); lib/utils/DebugDump.cpp: dd.Open(pDebugDumpDir);
Checked: src/daemon/MiddleWare.cpp: if (dd.Open(pDebugDumpDir)) src/daemon/MiddleWare.cpp: if (dd.Open(row.m_sDebugDumpDir.c_str())) src/daemon/MiddleWare.cpp: if (dd.Open(pDumpDir.c_str())) src/daemon/MiddleWare.cpp: if (dd.Open(pDebugDumpDir)) src/daemon/MiddleWare.cpp: if (dd.Open(pDebugDumpDir)) src/daemon/MiddleWare.cpp: if (dd.Open(pDebugDumpDir)) src/daemon/MiddleWare.cpp: if (dd.Open(row.m_sDebugDumpDir.c_str()))
Dne 9.8.2010 11:29, Michal Toman napsal(a):
Fixed
On 08/06/2010 12:43 PM, Karel Klic wrote:
Here is the review.
I tried to generate and report a Python crash, a kerneloops, and a C crash and everything seems to work.
========= lib/Plugins/CCpp.cpp The curly braces embracing CDebugDump usage can be removed on three places where the dd.Close() was added:
@@ -191,6 +191,7 @@ static void GetBacktrace(const char *pDebugDumpDir, @@ -327,6 +328,7 @@ static string run_unstrip_n(const char *pDebugDumpDir) @@ -511,6 +513,7 @@ string CAnalyzerCCpp::GetLocalUUID(const char *pDebugDumpDir)
========= lib/Plugins/KerneloopsScanner.cpp The function save_oops_to_debug_dump used to throw an exception when something went wrong with the dump directory creation. Currently the failure is not reported to upper level. I think the function should return boolean value indicating success, and its callers should check for it and log some error message so we know where the error came from.
src/Hooks/dumpoops.cpp imports save_oops_to_debug_dump from the plugin dynamic library, so the function interface must be changed there as well.
========= src/Daemon/MiddleWare.cpp In function DebugDumpToCrashReport the dd.Open() return value must be checked, as the Open() no longer throws an exception.
========= src/Hooks/abrt-hook-ccpp.cpp The indentation of comments became ugly on several places:
- /* We need coredumps to be readable by all, because
- when abrt daemon processes coredump,
- process producing backtrace is run under the same UID
- as the crashed process.
- Thus 644, not 600 */
- /* We need coredumps to be readable by all, because
- when abrt daemon processes coredump,
- process producing backtrace is run under the same UID
- as the crashed process.
- Thus 644, not 600 */
ABRT codebase contains a space between 'if' and '(':
- if (src_fd_binary> 0)
- if(dd.Create(path, uid))
Dne 5.8.2010 12:52, Michal Toman napsal(a):
Added dd.Close() to functions containing dd.Create() or dd.Open()
On 08/04/2010 08:01 PM, Karel Klic wrote:
On 08/04/2010 12:32 PM, Michal Toman wrote: > Please review
When I tried to generate a Python report using abrt-gui with this patch applied, abrtd started to loop repeatedly saying:
abrtd: Lock file '/var/spool/abrt/pyhook-1280938727-28545.lock' is already locked by us
I think that the reason for this might be that the try { CDebugDump dd; ... } catch {} block was also used to limit the scope of existence of CDebugDump instances, so dd's destructor was called when leaving the try catch block (the destructor unlocks the directory). AFAICT you can call dd.Close() after you are finished with particular CDebugDump instance to fix this (esp. in src/Daemon/MiddleWare.cpp).
crash-catcher@lists.fedorahosted.org
-
Jiri Moskovcak -
Karel Klic -
Michal Toman