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

Jakub Filak jfilak at redhat.com
Tue Jul 2 09:08:29 UTC 2013


Yes, I agree. See the next patch.

Anyway, new_id could be -1 and we should check it. So, this patch is
probably unnecessary but makes the code more robust (and I though you
like robust code).

On Tue, 2013-07-02 at 10:59 +0200, Denys Vlasenko wrote:
> 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);"

See the patch 2/2


> 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