[LIBREPORT PATCH 1/2] rhbz: test rhbz_new_bug() return value for errors

Denys Vlasenko dvlasenk at redhat.com
Tue Jul 2 08:59:30 UTC 2013


On 07/02/2013 09:55 AM, Jakub Filak wrote:
> Related to rhbz#980228
> 
> Signed-off-by: Jakub Filak <jfilak at redhat.com>
> ---
>  src/plugins/reporter-bugzilla.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/plugins/reporter-bugzilla.c b/src/plugins/reporter-bugzilla.c
> index d0e04b6..626b41f 100644
> --- a/src/plugins/reporter-bugzilla.c
> +++ b/src/plugins/reporter-bugzilla.c
> @@ -1240,6 +1240,11 @@ int main(int argc, char **argv)
>              free(bzcomment);
>              free(summary);
>  
> +            if (new_id == -1)
> +            {
> +                error_msg_and_die(_("Failed to create a new bug."));
> +            }
> +

I looked at the reasons rhbz_new_bug() may fail,
and it only returns -1 here:


    xmlrpc_value* result = abrt_xmlrpc_call_params(&env, ax, "Bug.create", params);
    xmlrpc_DECREF(params);
    xmlrpc_env_clean(&env);

^^^^^^^^^^^^^ missing check for error, ^^^^^^^^^^^^^^^^^^^^
something a-la "if (env->fault_occurred) abrt_xmlrpc_die(env);"
We are losing any potentially useful error message in there.
If we add such code here, then we dill die on the spot
and never return -1, making error check in the called unnecessary.

    free(status_whiteboard);
    free(summary);

    if (!result)
        return -1;

    int *r = rhbz_bug_read_item("id", result, RHBZ_MANDATORY_MEMB | RHBZ_READ_INT);
    xmlrpc_DECREF(result);
    int new_bug_id = *r;
    free(r);
^^^^^^^^^^^^^^^^^^^ this never returns error indicator -1
(RHBZ_MANDATORY_MEMB makes it die if member "id" is not there).
Well, unless the server in fact returns "id" == -1,
which is unlikely, and moreover, it wouldn't be an error,
but a real (weird) bug id.

    log(_("New bug id: %i"), new_bug_id);
    return new_bug_id;
}



So, it short, I think error check is needed, but maybe
it is better to put it not into reporter-bugzilla's main(),
but inside rhbz_new_bug().


More information about the Crash-catcher mailing list