I pushed the proposed changes, the only thing which has left to fix is
getting the component name (problem_data.c: 129). I need to ask rpm for
it and there are few possible solution:
1. call abrt-action-save-package-data
2. $ rpm -qf executable
3. use librpm
Suggestions?
Thank you,
Jirka
On 05/18/2011 07:20 PM, Denys Vlasenko wrote:
On Wed, 2011-05-18 at 17:05 +0200, Jiri Moskovcak wrote:
> Added few features we found missing:
>
> - flag to tell libreport to wait for the child so application can get
> it's status code and react according to it
> - added example of 3rd part reporting (abrt-gtk.c:188)
> - in abrt-gui go to Help->Report problem with ABRT
> - I will either remove this part or improve it if others find it usefull
> - report() report_dir() generates "global_uuid" using contents of all
> fields, so we at least eliminate the 100% same reports (unlikely to
> happen, but won't hurt to have it and also makes bugzilla reporter happy)
>
> NOTE: if you want to test bugzilla you need to edit conf file and add line:
>
> EVENT=report_Bugzilla analyzer="libreport" abrt-action-bugzilla
>
> TODO:
> - standalone example (maybe not needed?)
> - update python bindings
> - python example using python-meh + python bindings
>
> Please review,
char path[PATH_MAX+1];
...
/* Some callers set SIGCHLD to SIG_IGN.
* However, reporting spawns chils processes.
* Suppressing chil death notification terribly confuses some of them.
s/chils/child/, s/chil/child/
* Just in case, undo it.
* Note that we do it in the child, so the parent is never affected.
*/
signal(SIGCHLD, SIG_DFL); // applet still set it to SIG_IGN
nuke "// applet still set it to SIG_IGN" comment
strncpy(path, BIN_DIR"/bug-reporting-wizard", PATH_MAX);
path[PATH_MAX] = 0;
VERB1 log("Executing: %s", path);
execv(path, args);
maybe simpler, without string copies? -
const char *path; // instead of char path[PATH_MAX+1]
....
path = BIN_DIR"/bug-reporting-wizard";
VERB1 log("Executing: %s", path);
execv(path, args);
/* Did not find abrt-gui in installation directory. Oh well */
^^^^^^^^ replace by word "it" so that it doesn't
go out of sync
/* Trying to find it in PATH */
strncpy(path, "bug-reporting-wizard", PATH_MAX);
execvp(path, args);
perror_msg_and_die("Can't execute %s",
"bug-reporting-wizard");
path = "bug-reporting-wizard";
execvp(path, args);
perror_msg_and_die("Can't execute %s", path);
int status = 0;
redundant assignment.
pid_t p = waitpid(pid,&status, WUNTRACED);
We aren't interested in its inner workings, such as getting stopped/started.
Who knows, maybe it's using SIGSTOP/SIGCONT as some process synchronization
method? We are waiting for death only. s/WUNTRACED/0/
if(p == -1)
I'd say if (p<= 0)
0 is also not a valid return unless we use WNOHANG, which we don't.
Only> 0 (valid pid) return means that status is set.
{
error_msg("can't waitpid");
perror_msg - we want to show errno.
return EXIT_FAILURE;
}
if (WIFEXITED(status))
{
VERB2 log("reporting finished with exitcode: status=%d\n",
WEXITSTATUS(status));
return WEXITSTATUS(status);
}
else if (WIFSIGNALED(status))
{
VERB2 log("reporting killed by signal %d\n",
WTERMSIG(status));
+ return WTERMSIG(status) + 128; /* shell-like exit code on signal */
}
This will never be reported after we drop WUNTRACED:
else if (WIFSTOPPED(status))
{
/* should parent continue when the reporting is stopped??*/
VERB2 log("reporting stopped by signal %d\n",
WSTOPSIG(status));
}
This will never be reported (needs WCONTINUED):
else if (WIFCONTINUED(status))
{
VERB2 log("continued\n");
}
therefore ^^^ "else if (WIFSIGNALED(status))" is always true, and if() can be
removed
char *dir_name = strdup(dd->dd_dirname);
xstrdup.
if (dd_delete(dd) != 0)
{
error_msg("Can't remove tmp dir: %s", dd->dd_dirname);
}
dd_delete(dd) frees dd (regardless of success/failure of deletion).
therefore, use of dd->dd_dirname is "use after free". Use dir_name variable
instead here. Also, perror_msg is better (prints errno).
However, dd_delete(dd) itself emits error messages (even better ones - it tells
exactly which file can't be deleted). You can just skip error check altogether
here. Just do "dd_delete(dd);" and that's it.
if (strlen(text)> 0) ...
Faster: if (text[0]) ...
static void create_hash(char hash_str[SHA1_RESULT_LEN*2 + 1], const char *pInput)
{
unsigned char hash_bytes[SHA1_RESULT_LEN];
sha1_ctx_t sha1ctx;
sha1_begin(&sha1ctx);
sha1_hash(&sha1ctx, pInput, strlen(pInput));
sha1_end(&sha1ctx, hash_bytes);
unsigned len = SHA1_RESULT_LEN;
unsigned char *s = hash_bytes;
char *d = hash_str;
while (len)
{
*d++ = "0123456789abcdef"[*s>> 4];
*d++ = "0123456789abcdef"[*s& 0xf];
s++;
len--;
}
*d = '\0';
//log("hash:%s str:'%s'", hash_str, pInput);
}
static void generate_hash_for_all(gpointer key, gpointer value, gpointer user_data)
{
problem_item *pi = (problem_item *)value;
char *hash_str = (char *)user_data;
create_hash(hash_str, pi->content);
}
int report(problem_data_t *pd)
{
if (FILENAME_DUPHASH doesn't exist) ...
^^^^^^^^^^^^^^ add this
/* create hash from all components, so we at least eliminate the exact same
* reports
*/
char hash_str[SHA1_RESULT_LEN*2 + 1];
g_hash_table_foreach(pd,&generate_hash, hash_str);
add_to_problem_data(pd, FILENAME_DUPHASH, hash_str);
This hash generation is buggy (after s/generate_hash_for_all/generate_hash/
to make it compile) - it calculates hash for every element separately,
overwriting all results except last one.
Start and end of hashing should be moved into report(),
only sha1_hash(&sha1ctx, pInput, strlen(pInput)) should be done for
each element.
To avoid spurious hash differences, sort keys so that elements are
always processed in the same order:
GList *list = g_hash_table_get_keys(problem_data);
list = g_list_sort(list, (GCompareFunc)strcmp);
l = list;
while (l)
{
const char *key = l->data;
l = l->next;
struct problem_item *item = g_hash_table_lookup(problem_data, key);
...
Also, do not hash items which are binary (item->flags& CD_FLAG_BIN).
Their ->content is full file name, with path. Path is always
different and will make hash differ even if files are the same.
/* adds:
* analyzer:libreport
* executable:readlink(/proc/<pid>/exe)
* tries to guess component
*/
add_basics_to_problem_data(pd);
^^^^^^^^^^^^^
Looks like hash generation should be inside this function.
Generating hash is similar to adding other missing fields...
struct dump_dir *dd = create_dump_dir_from_problem_data(pd, "/tmp"/*
/var/tmp ?? */);
if (!dd)
return -1;
dd_create_basic_files(dd, getuid());
char *dir_name = xstrdup(dd->dd_dirname);
dd_close(dd);
VERB2 log("Temp problem dir: '%s'\n", dir_name);
report_dir(dir_name);
You need to reload pd from dir after report_dir() run, so that
caller sees updated data. Caller can't do that: it doesn't know
temp dir name.
If you don't want to do that every time, add a flag to make it optional.
You also need to delete the temp dir.
free(dir_name);
return 0;
}
void add_basics_to_problem_data(problem_data_t *pd)
{
const char *analyzer = get_problem_item_content_or_NULL(pd, FILENAME_ANALYZER);
if (analyzer == NULL)
add_to_problem_data(pd, "analyzer", "libreport");
if (FILENAME_EXECUTABLE is missing...)
^^^^^
pid_t pid = getpid();
if (pid> 0)
{
char buf[PATH_MAX+1];
char *exe = xasprintf("/proc/%u/exe", pid);
ssize_t read = readlink(exe, buf, PATH_MAX);
if (read> 0)
{
buf[read] = 0;
VERB2 log("reporting initiated from: %s\n", buf);
^^^^^^ stray \n.
add_to_problem_data(pd, FILENAME_EXECUTABLE, buf);
}
free(exe);
//#ifdef WITH_RPM
/* FIXME: component should be taken from rpm using
* rpm -qf executable
*/
/* Fedora/RHEL rpm specific piece of code */
const char *component = get_problem_item_content_or_NULL(pd,
FILENAME_ANALYZER);
if(component == NULL) // application didn't specify component
add_to_problem_data(pd, FILENAME_COMPONENT, "abrt");
//#endif
^^^ you don't need to do this, abrt-action-save-package-data does this for you.
(Hmm, need to run post-create to call that...)
}
}
enum {
NOWAIT = 0,
WAIT = (1<< 0), /* wait for report to finish and reload the problem data */
};
^^^^^^ Names are too generic. Lets add prefix to these. [LIB]REPORT_foo ?
/* analyzes AND reports a problem saved on disk
* - takes user through all the steps in reporting wizard
*/
int analyze_and_report_dir(const char* dirname, int flags);
/* analyzes AND reports a problem stored in problem_data_t
* it's first saved to /tmp and then processed as a dump_dir
* - takes user through all the steps in reporting wizard
*/
int analyze_and_report(problem_data_t *pd, int flags);
/* reports a problem saved on disk
* - shows only reporter selector and progress
*/
int report_dir(const char* dirname);
/* to report a problem stored in memory */
int report(problem_data_t *pd);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I propose having just two functions (maybe renamed):
int analyze_and_report_dir(const char* dirname, int flags);
int analyze_and_report(problem_data_t *pd, int flags);
but a few more flags:
enum {
LIBREPORT_POST_CREATE = (1<< n), / run post-create? */
LIBREPORT_ANALYZE = (1<< n), / run analyzers? */
/ ("run reporters" is always on, has no
flag) */
LIBREPORT_WAIT = (1<< n), /* wait for report to finish */
LIBREPORT_RELOAD_DATA = (1<< n), /* reload problem data after run (needs
WAIT) */
};