On 04/18/2013 04:58 PM, Denys Vlasenko wrote:
On 04/18/2013 03:03 PM, Jakub Filak wrote:
> If count item is missing in dump directory at abrtd start, then abrtd
> considers that dump directory as unprocessed.
>
> Closes #562
>
> Signed-off-by: Jakub Filak <jfilak(a)redhat.com>
> ---
> src/daemon/abrtd.c | 125 +++++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 111 insertions(+), 14 deletions(-)
>
> diff --git a/src/daemon/abrtd.c b/src/daemon/abrtd.c
> index 7001079..044eb41 100644
> --- a/src/daemon/abrtd.c
> +++ b/src/daemon/abrtd.c
> @@ -892,6 +892,98 @@ static void start_syslog_logging()
> putenv((char*)"ABRT_SYSLOG=1");
> }
>
> +/* The function expects that FILENAME_COUNT dump dir element is created by
> + * abrtd after all post-create events are successfully done. Thus if
> + * FILENAME_COUNT element doesn't exist abrtd can consider the dump directory
> + * as unprocessed.
> + *
> + * Relying on content of dump directory has one problem. If a hook provides
> + * FILENAME_COUNT abrtd will consider the dump directory as processed.
> + */
> +static void mark_unprocessed_dump_dirs_not_reportable(const char *path)
> +{
> + log("Searching for unprocessed dump directories");
> +
> + DIR *dp = opendir(path);
> + if (!dp)
> + {
> + perror_msg("Can't find unprocessed dump directories in directory
'%s'", path);
I would say "Can't open directory '%s'".
> + return;
> + }
> +
> + struct dirent *dent;
> + while ((dent = readdir(dp)) != NULL)
> + {
> + if (dot_or_dotdot(dent->d_name))
> + continue; /* skip "." and ".." */
> +
> + char *full_name = concat_path_file(path, dent->d_name);
> +
> + struct stat stat_buf;
> + if (stat(full_name, &stat_buf) != 0)
> + {
> + log("Can't access path '%s'", full_name);
this should be perror_msg(), not log().
> + goto next_dd;
> + }
> +
> + if (S_ISDIR(stat_buf.st_mode) == 0)
> + /* This is expected. The dump location contains some aux files */
> + goto next_dd;
> +
> + struct dump_dir *dd = dd_opendir(full_name, /*flags*/0);
> + if (dd)
> + {
> + if (!dd_exist(dd, FILENAME_COUNT))
> + {
> + log("Marking dump dir '%s' as not reportable because of
missing '"FILENAME_COUNT"' item",
> + full_name);
"dump dir" -> "problem dir[ectory]", or remove this part
altogether:
"Marking /var/tmp/cccp-FOO-BAR not reportable..."
already looks understandable without those words.
Can shorten this part: change
"... because of missing '"FILENAME_COUNT"' item"
to
"... (no '"FILENAME_COUNT"' item)"
> +
> + const char *reason = _("The problem data are incomplete. This
"
> + "usually happens when a problem is detected while
"
> + "computer is shutting down or user is logging out.
"
> + "In order to provide valuable problem reports, ABRT
"
> + "will not allow you to submit this problem. If you
"
> + "have time and want to help the developers in their
"
> + "effort to sort out this problem, please contact
"
> + "them directly.");
> +
> + /* Add dummy count 0 (valid dump directories has count >= 1)
> + * Doing it before updating FILENAME_NOT_REPORTABLE item in
> + * order to avoid unexpected growing of FILENAME_NOT_REPORTABLE
> + * item. And if any of the following operation will cause a
> + * failure of abrtd, then abrtd will not go there again after
> + * restart.
> + */
> + VERB1 log("Adding dummy count '0' to
'%s'", full_name);
> + dd_save_text(dd, FILENAME_COUNT, "0");
> +
> + char *old_not_reportable = dd_load_text_ext(dd,
FILENAME_NOT_REPORTABLE,
> + DD_FAIL_QUIETLY_ENOENT |
DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE);
> + if (old_not_reportable)
> + { /* Add the reason to the beginnig of
> + * FILENAME_NOT_REPORTABLE item because this has higher
> + * priority (two new lines because the reason doesn't
> + * have any)
> + */
Hmmm.
Maybe if not_reportable already exists, just skip the directory altogether?
This eliminates the risk of increasing confusion :)
- I agree with Denys here, it's too confusing to show 2 reasons to users...
> + char *message = xasprintf("%s\n\n%s", reason,
old_not_reportable);
> + dd_save_text(dd, FILENAME_NOT_REPORTABLE, message);
> + free(message);
> + free(old_not_reportable);
> + }
> + else
> + dd_save_text(dd, FILENAME_NOT_REPORTABLE, reason);
> + }
> + dd_close(dd);
> + }
> + else
> + log("Found an invalid dump directory '%s'",
full_name);
Wouldn't dd_opendir() already emit a message explaining what's wrong?
> +
> + next_dd:
> + free(full_name);
> + }
> + closedir(dp);
> +}