Re: [PATCH] if better backtrace is avail, then upload one

[Jiri Moskovcak]

Please see the comments inline.

Jirka

On 11/29/2011 03:26 PM, Nikola Pajkovsky wrote:
> Signed-off-by: Nikola Pajkovsky
>
> src/plugins/reporter-bugzilla.c |   17 ++++++++++-
>   src/plugins/rhbz.c              |   62 +++++++++++++++++++++++++++++++++++----
>   src/plugins/rhbz.h              |    1 +
>   3 files changed, 73 insertions(+), 7 deletions(-)
>
> diff --git a/src/plugins/reporter-bugzilla.c b/src/plugins/reporter-bugzilla.c
> index 4046b24..4739c2e 100644
> --- a/src/plugins/reporter-bugzilla.c
> +++ b/src/plugins/reporter-bugzilla.c
> @@ -377,14 +377,16 @@ int main(int argc, char **argv)
>           {
>               const char *package = get_problem_item_content_or_NULL(problem_data, FILENAME_PACKAGE);
>               const char *arch    = get_problem_item_content_or_NULL(problem_data, FILENAME_ARCHITECTURE);
>        const char *rating_str = get_problem_item_content_or_NULL(problem_data, FILENAME_RATING);
>         char *full_dsc = xasprintf("Package: %s\n"
>                                    "Architecture: %s\n"
>                                    "OS Release: %s\n"
>                                   "rating: %s\n"
>                                    "\n"
>                                    "Comment\n"
>                                    "-----\n"
>                                    "%s\n",
>                                   package, arch, release, comment
>                                   package, arch, release, rating_str, comment
>         );
>         log(_("Adding new comment to bug %d"), bz->bi_id);
>         /* unused code, enable it when gui/cli will be ready
>
> @@ -394,6 +396,19 @@ int main(int argc, char **argv)
>               */
>               rhbz_add_comment(client, bz->bi_id, full_dsc, 0);
>               free(full_dsc);
>        unsigned rating = xatou(rating_str);
>        if (bz->bi_best_bt_rating<  rating)
>        {
>            char bug_id_str[sizeof(int)*3 + 2];
>            sprintf(bug_id_str, "%i", bz->bi_id);
>            const char *bt =  get_problem_item_content_or_NULL(problem_data,
>                                                               FILENAME_BACKTRACE);
>            log(_("Attaching better backtrace"));
>            rhbz_attach_blob(client, FILENAME_BACKTRACE, bug_id_str, bt, strlen(bt),
>                             RHBZ_NOMAIL_NOTIFY);
>        }
>     }
> }
>
> diff --git a/src/plugins/rhbz.c b/src/plugins/rhbz.c
> index 13957b8..f767215 100644
> --- a/src/plugins/rhbz.c
> +++ b/src/plugins/rhbz.c
> @@ -42,14 +42,62 @@ void free_bug_info(struct bug_info *bi)
>   list_free_with_free(bi->bi_cc_list);
>
> bi->bi_status = NULL;
> bi->bi_resolution = NULL;
> bi->bi_reporter = NULL;
> bi->bi_product = NULL;
> free(bi);
>
> +}
> bi->bi_cc_list = NULL;
>
> +unsigned find_best_bt_rating_in_comments(xmlrpc_value *result_xml)
> +{
> xmlrpc_value *comments_memb = rhbz_get_member("longdescs", result_xml);
> if (!comments_memb)
>    return 0;
>
> free(bi);
> int comments_memb_size = rhbz_array_size(comments_memb);
> xmlrpc_env env;
> xmlrpc_env_init(&env);

- if you call init() shouldn't there be deinit() somewhere?
> int best_bt_rating = 0;
> for (int i = 0; i<  comments_memb_size; ++i)
> {
>    xmlrpc_value* item = NULL;
>    xmlrpc_array_read_item(&env, comments_memb, i,&item);
>    if (env.fault_occurred)
>        abrt_xmlrpc_die(&env);
>    char *comment_body = rhbz_bug_read_item("body", item, RHBZ_READ_STR);
>    /* attachments are sometimes without comments -- skip them */
>    if (!comment_body)
>        continue;
>    char *start_rating_line = strstr(comment_body, "rating: ");
>    if (!start_rating_line)
>    {
>        error_msg("no rating");

- use VERBX log() here, it's not an error when there is no "rating: " in 
some comment
>        continue;
>    }
>    start_rating_line += strlen("rating: ");
>    char *end_rating_line = strchr(start_rating_line, '\n');
>    if (!end_rating_line)
>        error_msg_and_die("broken comment body");

- I wouldn't die here, what if the next comment body will be ok? and you 
still might want to continue with adding the current reporter to CC
>    char *rating_line = xstrndup(start_rating_line, end_rating_line - start_rating_line);

- at this point I would use char *rating_str instead of rating_line
- and you leak rating_line
>    int old_errno = errno;

- wtf? why you need to store errno?
>    errno = 0;
>    char *e;
>    long rating = strtoul(rating_line,&e, 10);
>    if (errno || rating_line == e || *e != '\0' || rating>  UINT_MAX)
>    {
>        /* error / no digits / illegal trailing chars */
>        errno = old_errno;

-- and restore it ^?
>        continue;
>    }
>    errno = old_errno; /* Ok.  So restore errno. */

- why?^
>    if (rating>  best_bt_rating)
>        best_bt_rating = rating;
>
> }
>
> return best_bt_rating;
> }
>
> void rhbz_login(struct abrt_xmlrpc *ax, const char* login, const char* passwd)
>
> @@ -273,6 +321,8 @@ struct bug_info *rhbz_bug_info(struct abrt_xmlrpc *ax, int bug_id)
>   bz->bi_cc_list = rhbz_bug_cc(xml_bug_response);
>
> bz->bi_best_bt_rating = find_best_bt_rating_in_comments(xml_bug_response);
> xmlrpc_DECREF(xml_bug_response);
>
> return bz;
>
> diff --git a/src/plugins/rhbz.h b/src/plugins/rhbz.h
> index 864f603..9878dd7 100644
> --- a/src/plugins/rhbz.h
> +++ b/src/plugins/rhbz.h
> @@ -48,6 +48,7 @@ enum {
>   struct bug_info {
>       int bi_id;
>       int bi_dup_id;
>
> unsigned bi_best_bt_rating;
>
> char *bi_status;
>  char *bi_resolution;