I run-tested the branch and it seems to be working well. I also reviewed
the code but due to my lack of experience with gtk/dbus/libnotify/NM I
probably missed the major flaws if there are any:) For smaller issues,
see my comments inline.
Side notes:
- How can I trigger unknown problem without creating bogus problem
report for some package? (Can we include a program in will_crash that
will crash with stack random enough that the clustering algorithm
always fails?:))
- On my system, the notification shows after reporter-mailx has
finished (or so it seems), which takes some time (maybe my MTA is
misconfigured?). Can we reorder the events so that the applet shows
immediately?
On Mon, Nov 12, 2012 at 19:14:32 +0100, Jakub Filak wrote:
> diff --git a/src/applet/applet.c b/src/applet/applet.c
> index c34e4c6..68180fe 100644
> --- a/src/applet/applet.c
> +++ b/src/applet/applet.c
> +static GList *g_deffered_crash_queue;
> +static guint g_deffered_timeout = 0;
Deffered is actually spelled deferred.
> * in $XDG_CACHE_HOME/abrt/applet_dirlist. In any case, applet_dirlist
> * is updated with updated list.
> */
> -static GList *new_dir_exists(void)
> +static void new_dir_exists(GList **new_dirs)
> {
Please update the comment of new_dir_exists, it does not reflect what
the function does (i.e. does not return anything). Also update the
comments at the function call sites.
> static void fork_exec_gui(void)
> {
> - pid_t pid = vfork();
> + pid_t pid = fork();
> if (pid < 0)
> - perror_msg("vfork");
> + perror_msg("fork");
> if (pid == 0)
> {
> /* child */
> - signal(SIGCHLD, SIG_DFL); /* undo SIG_IGN in abrt-applet */
> -//TODO: pass s_dirs[] as DIR param(s) to abrt-gui
> + /* double fork to avoid GUI zombies */
> + pid_t grand_child = vfork();
> + if (grand_child != 0)
> + {
> + if (grand_child < 0) perror_msg("vfork");
> + exit(0);
> + }
> +
> + /* grand child */
> + //TODO: pass s_dirs[] as DIR param(s) to abrt-gui
> execl(BIN_DIR"/abrt-gui", "abrt-gui", (char*) NULL);
> /* Did not find abrt-gui in installation directory. Oh well */
> /* Trying to find it in PATH */
> execlp("abrt-gui", "abrt-gui", (char*) NULL);
> perror_msg_and_die("Can't execute '%s'", "abrt-gui");
> }
> +
> + safe_waitpid(pid, /* status */ NULL, /* options */ 0);
> +
If I understand vfork() correctly, doing anything else than exec*/_exit
(e.g. perror_msg_and_die) results in undefined behaviour. We can either
use fork() here or simply _exit on error.
> + notify_notification_add_action(notification, A_KNOWN_OPEN_GUI,
> _("Open"),
...
> + notify_notification_add_action(notification,
> A_KNOWN_OPEN_BROWSER, _("Show"),
I think "Open" and "Show" sound a bit similar, but I can't think of
anythig better:( Can we use two+ words?
> + /* Read streamed data and split lines */
> + for (;;)
> + {
> + char buf[250]; /* usually we get one line, no need to have big buf */
> + errno = 0;
> + gsize r = 0;
> + g_io_channel_read_chars(gio, buf, sizeof(buf) - 1, &r, NULL);
> + if (r <= 0)
> + break;
> + buf[r] = '\0';
> +
> + /* split lines in the current buffer */
> + char *raw = buf;
> + char *newline;
> + while ((newline = strchr(raw, '\n')) != NULL)
> + {
> + *newline = '\0';
> + strbuf_append_str(state->cmd_output, raw);
> + char *msg = state->cmd_output->buf;
>
> - notify_notification_add_action(notification, "REPORT", _("Report"),
> - NOTIFY_ACTION_CALLBACK(action_report),
> - pi, NULL);
> + VERB3 log("%s", msg);
> + pi->known |= (prefixcmp(msg, "THANKYOU") == 0);
>
> - notify_notification_update(notification, _("A Problem has Occurred"),
> buf, NULL);
> - free(buf);
> + strbuf_clear(state->cmd_output);
> + /* jump to next line */
> + raw = newline + 1;
> + }
>
> - GError *err = NULL;
> - notify_notification_show(notification, &err);
> - if (err != NULL)
> + /* beginning of next line. the line continues by next read */
> + strbuf_append_str(state->cmd_output, raw);
> + }
> +
> + if (errno == EAGAIN)
Does g_io_channel_read_chars set the errno? I can't find any mention of
it in the documentation.
Martin
