Hi,
The attached patch allows user to select which reporter he wants to use to report a crash using CLI. The daemon skips reporters which are not in the list of reporters provided via Report() dbus call.
src/CLI/dbus.cpp - added GetSettings dbus call - send reporters list when calling dbus Report() src/CLI/report.cpp - new functions split() - new function get_enabled_reporters(crash_report) - new function ask_yesno(question) - get_reporter_plugin_settings now gets the list of reporters - report using all reporters only in the --report-always mode - ask if user wants to report using a particular reporter; if he agrees, then check the reporter configuration (and ask for login/password if it's missing) src/Daemon/CommLayerServerDBus.cpp - pass reporter list from Report() dbus call to Report() implementation in MiddleWare.cpp src/Daemon/MiddleWare.cpp - skip reporters which are not in the list in Report() - removed old commented ~/.abrt code which is now in CLI
Best regards, Karel
Compiles and works ok and I don't see any major problems at first sight, just a few minor ones:
- remove the trailing spaces before you push it into git - not sure about removing the (now commented out) code for writing the settings in $HOME, as the plan is to split the daemon into root/non-root parts (but we can always find it in git history :)
Reviewed by: jmoskovc@redhat.com
On 03/15/2010 05:25 PM, Karel Klic wrote:
Hi,
The attached patch allows user to select which reporter he wants to use to report a crash using CLI. The daemon skips reporters which are not in the list of reporters provided via Report() dbus call.
src/CLI/dbus.cpp
- added GetSettings dbus call
- send reporters list when calling dbus Report()
src/CLI/report.cpp
- new functions split()
- new function get_enabled_reporters(crash_report)
- new function ask_yesno(question)
- get_reporter_plugin_settings now gets the list of reporters
- report using all reporters only in the --report-always mode
- ask if user wants to report using a particular reporter;
if he agrees, then check the reporter configuration (and ask for login/password if it's missing) src/Daemon/CommLayerServerDBus.cpp
- pass reporter list from Report() dbus call to Report()
implementation in MiddleWare.cpp src/Daemon/MiddleWare.cpp
- skip reporters which are not in the list in Report()
- removed old commented ~/.abrt code which is now in CLI
Best regards, Karel
Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
On Mon, 2010-03-15 at 17:25 +0100, Karel Klic wrote:
Hi,
The attached patch allows user to select which reporter he wants to use to report a crash using CLI. The daemon skips reporters which are not in the list of reporters provided via Report() dbus call.
src/CLI/dbus.cpp
- added GetSettings dbus call
- send reporters list when calling dbus Report()
src/CLI/report.cpp
- new functions split()
- new function get_enabled_reporters(crash_report)
- new function ask_yesno(question)
- get_reporter_plugin_settings now gets the list of reporters
- report using all reporters only in the --report-always mode
- ask if user wants to report using a particular reporter; if he agrees, then check the reporter configuration (and ask for login/password if it's missing)
src/Daemon/CommLayerServerDBus.cpp
- pass reporter list from Report() dbus call to Report() implementation in MiddleWare.cpp
src/Daemon/MiddleWare.cpp
- skip reporters which are not in the list in Report()
- removed old commented ~/.abrt code which is now in CLI
+static vector_string_t split(const std::string &s, char delim) +{ + std::stringstream ss(s); + std::string item; + std::vectorstd::string elems; + while (std::getline(ss, item, delim)) + elems.push_back(item); + return elems; +}
Use of C++ streams usually adds on the order of 10k of code. We need to talk later whether we want to go entirely C. For now, personally I try to minimize usage of C++ constructs. Here, looks like you can as easily perform splitting using, say, loop with strchr('\n') inside.
+ /* Get the analyzer from the crash. */ + map_crash_data_t::const_iterator analyzerit = crash_data.find(FILENAME_ANALYZER); + if (analyzerit == crash_data.end()) + return result; /* No analyzer found in the crash data. */ + std::string analyzer = analyzerit->second[CD_CONTENT];
there is a helper for this which also would make sure [CD_CONTENT] index is not out of range (i.e. it won't SEGV if it is not there, unlike code above):
const char *analyser = get_crash_data_item_content_or_NULL(crash_data, FILENAME_ANALYZER); if (!analyzer) return result;
Look into CrashTypes.h, there are a few more of them.
I run tested the patch, it seems to work. -- vda
On 03/16/2010 03:05 PM, Denys Vlasenko wrote:
+static vector_string_t split(const std::string&s, char delim) +{
- std::stringstream ss(s);
- std::string item;
- std::vectorstd::string elems;
- while (std::getline(ss, item, delim))
- elems.push_back(item);
- return elems;
+}
Use of C++ streams usually adds on the order of 10k of code. We need to talk later whether we want to go entirely C. For now, personally I try to minimize usage of C++ constructs. Here, looks like you can as easily perform splitting using, say, loop with strchr('\n') inside.
Agreed. I rewrote this routine to use strtok_r.
- /* Get the analyzer from the crash. */
- map_crash_data_t::const_iterator analyzerit = crash_data.find(FILENAME_ANALYZER);
- if (analyzerit == crash_data.end())
- return result; /* No analyzer found in the crash data. */
- std::string analyzer = analyzerit->second[CD_CONTENT];
there is a helper for this which also would make sure [CD_CONTENT] index is not out of range (i.e. it won't SEGV if it is not there, unlike code above):
const char *analyser = get_crash_data_item_content_or_NULL(crash_data, FILENAME_ANALYZER); if (!analyzer) return result;
The code has been modified to take advantage of this function.
Commited to git.
On 03/17/2010 11:53 AM, Karel Klic wrote:
On 03/16/2010 03:05 PM, Denys Vlasenko wrote:
+static vector_string_t split(const std::string&s, char delim) +{
- std::stringstream ss(s);
- std::string item;
- std::vectorstd::string elems;
- while (std::getline(ss, item, delim))
- elems.push_back(item);
- return elems;
+}
Use of C++ streams usually adds on the order of 10k of code. We need to talk later whether we want to go entirely C. For now, personally I try to minimize usage of C++ constructs. Here, looks like you can as easily perform splitting using, say, loop with strchr('\n') inside.
Agreed. I rewrote this routine to use strtok_r.
- /* Get the analyzer from the crash. */
- map_crash_data_t::const_iterator analyzerit =
crash_data.find(FILENAME_ANALYZER);
- if (analyzerit == crash_data.end())
- return result; /* No analyzer found in the crash data. */
- std::string analyzer = analyzerit->second[CD_CONTENT];
there is a helper for this which also would make sure [CD_CONTENT] index is not out of range (i.e. it won't SEGV if it is not there, unlike code above):
const char *analyser = get_crash_data_item_content_or_NULL(crash_data, FILENAME_ANALYZER); if (!analyzer) return result;
The code has been modified to take advantage of this function.
Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
On Wed, 2010-03-17 at 11:53 +0100, Karel Klic wrote:
On 03/16/2010 03:05 PM, Denys Vlasenko wrote:
+static vector_string_t split(const std::string&s, char delim) +{
- std::stringstream ss(s);
- std::string item;
- std::vectorstd::string elems;
- while (std::getline(ss, item, delim))
- elems.push_back(item);
- return elems;
+}
Use of C++ streams usually adds on the order of 10k of code. We need to talk later whether we want to go entirely C. For now, personally I try to minimize usage of C++ constructs. Here, looks like you can as easily perform splitting using, say, loop with strchr('\n') inside.
Agreed. I rewrote this routine to use strtok_r.
Eh, since I am late in commenting, I can't ask *you* to do it a bit differently, I guess I have to propose the my own patch on top.
Here is it.
Sometime ago we talked about const string parameters and decided to generally use const char*, not const string&. The patch below uses that convention. It also fixes a check for enabled plugins and does a fem other small tweaks.
Please review...
I tested your patch, and it seems to work well. Thank you for improving that code.
Reviewed by: Karel Klic kklic@redhat.com
Dne 18.3.2010 14:12, Denys Vlasenko napsal(a):
On Wed, 2010-03-17 at 11:53 +0100, Karel Klic wrote:
On 03/16/2010 03:05 PM, Denys Vlasenko wrote:
+static vector_string_t split(const std::string&s, char delim) +{
- std::stringstream ss(s);
- std::string item;
- std::vectorstd::string elems;
- while (std::getline(ss, item, delim))
- elems.push_back(item);
- return elems;
+}
Use of C++ streams usually adds on the order of 10k of code. We need to talk later whether we want to go entirely C. For now, personally I try to minimize usage of C++ constructs. Here, looks like you can as easily perform splitting using, say, loop with strchr('\n') inside.
Agreed. I rewrote this routine to use strtok_r.
Eh, since I am late in commenting, I can't ask *you* to do it a bit differently, I guess I have to propose the my own patch on top.
Here is it.
Sometime ago we talked about const string parameters and decided to generally use const char*, not const string&. The patch below uses that convention. It also fixes a check for enabled plugins and does a fem other small tweaks.
Please review...
crash-catcher@lists.fedorahosted.org