This one works on top of my previous gui patch and allows user to directly report last crash by clicking on the button in notification bubble.
Please review, J.
On Tue, 2010-03-09 at 11:46 +0100, Jiri Moskovcak wrote:
This one works on top of my previous gui patch and allows user to directly report last crash by clicking on the button in notification bubble.
The bubble gets misplaced. I guess it happens because icon does not appear fast enough, and window manager does not know where to draw the bubble is icon is not visible.
See screenshot.
On 03/10/2010 01:36 PM, Denys Vlasenko wrote:
On Tue, 2010-03-09 at 11:46 +0100, Jiri Moskovcak wrote:
This one works on top of my previous gui patch and allows user to directly report last crash by clicking on the button in notification bubble.
The bubble gets misplaced. I guess it happens because icon does not appear fast enough, and window manager does not know where to draw the bubble is icon is not visible.
See screenshot.
Actually I changed the behaviour so now it's not bind to the icon anymore, the notification placemt is controlled by the notification daemon settings.
J.
Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
On Tue, 2010-03-09 at 11:46 +0100, Jiri Moskovcak wrote:
This one works on top of my previous gui patch and allows user to directly report last crash by clicking on the button in notification bubble.
Please review,
+void CApplet::CrashNotify(const char* crash_id, const char *format, ...) +{ + m_pLastCrashID = crash_id; + va_list args; + va_start(args, format); + char *buf = xvasprintf(format, args); + va_end(args); + + NotifyNotification *notification = new_warn_notification(); + notify_notification_add_action(notification, "REPORT", _("Report"), + NOTIFY_ACTION_CALLBACK(CApplet::action_report), + this, NULL); + notify_notification_add_action(notification, "OPEN_MAIN_WINDOW", "Open ABRT", + NOTIFY_ACTION_CALLBACK(CApplet::action_open_gui), + this, NULL); + + notify_notification_update(notification, _("Warning"), buf, NULL); + + GError *err = NULL; + notify_notification_show(notification, &err); + if (err != NULL) + { + error_msg("%s", err->message); + g_error_free(err); + } +}
buf is leaked. (There are more similar leaks elsewhere).
- g_pCommLayer->Crash(get_crash_data_item_content(crashinfo, FILENAME_PACKAGE).c_str(), uid_str); + g_pCommLayer->Crash(get_crash_data_item_content(crashinfo, FILENAME_PACKAGE).c_str(), + get_crash_data_item_content(crashinfo, CD_UUID).c_str(), uid_str);
Crash id is "UID:UUID", not just "UUID". In the "UID:UUID" form, it identifies crash uniquely.
-- vda
On 03/10/2010 02:09 PM, Denys Vlasenko wrote:
On Tue, 2010-03-09 at 11:46 +0100, Jiri Moskovcak wrote:
This one works on top of my previous gui patch and allows user to directly report last crash by clicking on the button in notification bubble.
Please review,
+void CApplet::CrashNotify(const char* crash_id, const char *format, ...) +{
- m_pLastCrashID = crash_id;
- va_list args;
- va_start(args, format);
- char *buf = xvasprintf(format, args);
- va_end(args);
- NotifyNotification *notification = new_warn_notification();
- notify_notification_add_action(notification, "REPORT", _("Report"),
NOTIFY_ACTION_CALLBACK(CApplet::action_report),
this, NULL);
- notify_notification_add_action(notification, "OPEN_MAIN_WINDOW", "Open ABRT",
NOTIFY_ACTION_CALLBACK(CApplet::action_open_gui),
this, NULL);
- notify_notification_update(notification, _("Warning"), buf, NULL);
- GError *err = NULL;
- notify_notification_show(notification,&err);
- if (err != NULL)
- {
error_msg("%s", err->message);
g_error_free(err);
- }
+}
buf is leaked. (There are more similar leaks elsewhere).
hopefully fixed all instances of leaked buf
g_pCommLayer->Crash(get_crash_data_item_content(crashinfo, FILENAME_PACKAGE).c_str(), uid_str);
g_pCommLayer->Crash(get_crash_data_item_content(crashinfo, FILENAME_PACKAGE).c_str(),
get_crash_data_item_content(crashinfo, CD_UUID).c_str(), uid_str);
Crash id is "UID:UUID", not just "UUID". In the "UID:UUID" form, it identifies crash uniquely.
the second version sends the crashid in the form of uid:uuid from daemon.
J.
-- vda
Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
On Wed, 2010-03-10 at 18:52 +0100, Jiri Moskovcak wrote:
buf is leaked. (There are more similar leaks elsewhere).
hopefully fixed all instances of leaked buf
g_pCommLayer->Crash(get_crash_data_item_content(crashinfo, FILENAME_PACKAGE).c_str(), uid_str);
g_pCommLayer->Crash(get_crash_data_item_content(crashinfo, FILENAME_PACKAGE).c_str(),
get_crash_data_item_content(crashinfo, CD_UUID).c_str(), uid_str);
Crash id is "UID:UUID", not just "UUID". In the "UID:UUID" form, it identifies crash uniquely.
the second version sends the crashid in the form of uid:uuid from daemon.
Tested. Seems to work. (The bubble is still placed in a strange place though)
-void CApplet::CrashNotify(const char *format, ...) +void CApplet::action_report(NotifyNotification *notification, gchar *action, gpointer user_data) { - va_list args; + CApplet *applet = (CApplet *)user_data; + if (applet->m_bDaemonRunning) + { + pid_t pid = vfork(); + if (pid < 0) + perror_msg("vfork"); + if (pid == 0) + { /* child */ + char *buf = NULL; + int n = 0; + n = asprintf(&buf, "--report=%s", applet->m_pLastCrashID);
xasprintf() will be simpler (no need to have "int n" variable)...
+ signal(SIGCHLD, SIG_DFL); /* undo SIG_IGN in abrt-applet */ + execl(BIN_DIR"/abrt-gui", "abrt-gui", (n >= 0 && buf) ? buf : (char*) NULL, (char*) NULL);
...and no need to have "(n >= 0 && buf) ? buf : (char*) NULL" here
+ perror_msg_and_die("Can't exec abrt-gui"); + free(buf);
Unreachable free()
/* Send dbus signal */ + char *crash_id = NULL; if (analyzer_has_InformAllUsers(analyzer)) uid_str = NULL; - g_pCommLayer->Crash(get_crash_data_item_content(crashinfo, FILENAME_PACKAGE).c_str(), uid_str); + else + { + crash_id = xasprintf("%s:%s",uid_str,get_crash_data_item_content(crashinfo, CD_UUID).c_str()); + } + g_pCommLayer->Crash(get_crash_data_item_content(crashinfo, FILENAME_PACKAGE).c_str(), + crash_id != NULL ? crash_id : get_crash_data_item_content(crashinfo, CD_UUID).c_str(), + uid_str); + free(crash_id);
I think you are confused here. uid_str is an uid to inform, whereas "UID:" part from crash_id of the "UID:UUID" format is the UID of the crashed program. uid_str can't be used for it. The code should be:
/* Send dbus signal */ if (analyzer_has_InformAllUsers(analyzer)) uid_str = NULL; crash_id = xasprintf("%s:%s", get_crash_data_item_content(crashinfo, CD_UID).c_str() get_crash_data_item_content(crashinfo, CD_UUID).c_str() ); g_pCommLayer->Crash(get_crash_data_item_content(crashinfo, FILENAME_PACKAGE).c_str(), crash_id, uid_str); free(crash_id);
IOW, "always send the full crash_id".
+ if(notify_is_initted()) + notify_uninit();
Minor style nit: needs space - "if (...)"
-- vda
On 03/11/2010 01:05 PM, Denys Vlasenko wrote:
On Wed, 2010-03-10 at 18:52 +0100, Jiri Moskovcak wrote:
buf is leaked. (There are more similar leaks elsewhere).
hopefully fixed all instances of leaked buf
g_pCommLayer->Crash(get_crash_data_item_content(crashinfo, FILENAME_PACKAGE).c_str(), uid_str);
g_pCommLayer->Crash(get_crash_data_item_content(crashinfo, FILENAME_PACKAGE).c_str(),
get_crash_data_item_content(crashinfo, CD_UUID).c_str(), uid_str);
Crash id is "UID:UUID", not just "UUID". In the "UID:UUID" form, it identifies crash uniquely.
the second version sends the crashid in the form of uid:uuid from daemon.
Tested. Seems to work. (The bubble is still placed in a strange place though)
This is expected behaviour, the bubble placement is defined by configuration of your {GNOME, KDE, XFCE, ...} notification daemon as it's not bind to the icon anymore. (e.g. on my desktop it's shown in the top right corner because I configured my notification daemon that way).
-void CApplet::CrashNotify(const char *format, ...) +void CApplet::action_report(NotifyNotification *notification, gchar *action, gpointer user_data) {
- va_list args;
- CApplet *applet = (CApplet *)user_data;
- if (applet->m_bDaemonRunning)
- {
pid_t pid = vfork();
if (pid< 0)
perror_msg("vfork");
if (pid == 0)
{ /* child */
char *buf = NULL;
int n = 0;
n = asprintf(&buf, "--report=%s", applet->m_pLastCrashID);
xasprintf() will be simpler (no need to have "int n" variable)...
Changed.
signal(SIGCHLD, SIG_DFL); /* undo SIG_IGN in abrt-applet */
execl(BIN_DIR"/abrt-gui", "abrt-gui", (n>= 0&& buf) ? buf : (char*) NULL, (char*) NULL);
...and no need to have "(n>= 0&& buf) ? buf : (char*) NULL" here
perror_msg_and_die("Can't exec abrt-gui");
free(buf);
Unreachable free()
Fixed.
/* Send dbus signal */
char *crash_id = NULL; if (analyzer_has_InformAllUsers(analyzer)) uid_str = NULL;
g_pCommLayer->Crash(get_crash_data_item_content(crashinfo, FILENAME_PACKAGE).c_str(), uid_str);
else
{
crash_id = xasprintf("%s:%s",uid_str,get_crash_data_item_content(crashinfo, CD_UUID).c_str());
}
g_pCommLayer->Crash(get_crash_data_item_content(crashinfo, FILENAME_PACKAGE).c_str(),
crash_id != NULL ? crash_id : get_crash_data_item_content(crashinfo, CD_UUID).c_str(),
uid_str);
free(crash_id);
I think you are confused here. uid_str is an uid to inform, whereas "UID:" part from crash_id of the "UID:UUID" format is the UID of the crashed program. uid_str can't be used for it. The code should be:
/* Send dbus signal */ if (analyzer_has_InformAllUsers(analyzer)) uid_str = NULL; crash_id = xasprintf("%s:%s", get_crash_data_item_content(crashinfo, CD_UID).c_str() get_crash_data_item_content(crashinfo, CD_UUID).c_str() ); g_pCommLayer->Crash(get_crash_data_item_content(crashinfo, FILENAME_PACKAGE).c_str(), crash_id, uid_str); free(crash_id);
IOW, "always send the full crash_id".
Yes, I forgot you removed the '-1' magic so now we always have the right uid.
- if(notify_is_initted())
notify_uninit();
Minor style nit: needs space - "if (...)"
-- vda
Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
On Thu, 2010-03-11 at 13:44 +0100, Jiri Moskovcak wrote:
Tested. Seems to work. (The bubble is still placed in a strange place though)
-void CApplet::CrashNotify(const char *format, ...) +void CApplet::action_report(NotifyNotification *notification, gchar *action, gpointer user_data) {
- va_list args;
- CApplet *applet = (CApplet *)user_data;
- if (applet->m_bDaemonRunning)
- {
pid_t pid = vfork();
if (pid< 0)
perror_msg("vfork");
if (pid == 0)
{ /* child */
char *buf = NULL;
int n = 0;
n = asprintf(&buf, "--report=%s", applet->m_pLastCrashID);
xasprintf() will be simpler (no need to have "int n" variable)...
Changed.
- * - Crash(progname,uid) - a new crash occurred (new /var/cache/abrt/DIR is found) + * - Crash(progname, uid, crash_id) - a new crash occurred (new /var/cache/abrt/DIR is found)
the order is wrong, should be: crash_id, uid
+ char *buf = NULL; + buf = xasprintf("--report=%s", applet->m_pLastCrashID);
This is technically correct, and gcc will optimize out redundant store, but you still may want to combine it into one line or at least remove "= NULL".
+ execlp("abrt-gui", "abrt-gui", buf, (char*) NULL); + free(buf); + perror_msg_and_die("Can't exec abrt-gui");
free() is not needed, exiting frees all. Consider removing if it doesn't go against your coding habits.
Please apply after you deal with these, I think no further review iterations are needed.
crash-catcher@lists.fedorahosted.org