Related to rhbz#980228
Signed-off-by: Jakub Filak jfilak@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.")); + } + log(_("Adding attachments to bug %i"), new_id); char new_id_str[sizeof(int)*3 + 2]; sprintf(new_id_str, "%i", new_id);
Closes rhbz#980228
Signed-off-by: Jakub Filak jfilak@redhat.com --- src/lib/abrt_xmlrpc.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/lib/abrt_xmlrpc.c b/src/lib/abrt_xmlrpc.c index b531d25..75f1f2b 100644 --- a/src/lib/abrt_xmlrpc.c +++ b/src/lib/abrt_xmlrpc.c @@ -206,6 +206,9 @@ xmlrpc_value *abrt_xmlrpc_call_params(xmlrpc_env *env, struct abrt_xmlrpc *ax, c xmlrpc_client_call2(env, ax->ax_client, ax->ax_server_info, method, array, &result);
+ if (env->fault_occurred) + abrt_xmlrpc_die(env); + xmlrpc_DECREF(array); return result; }
On 07/02/2013 09:55 AM, Jakub Filak wrote:
Related to rhbz#980228
Signed-off-by: Jakub Filak jfilak@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().
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@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().
Thank you for the review!
On Tue, 2013-07-02 at 11:21 +0200, Denys Vlasenko wrote:
On 07/02/2013 11:08 AM, Jakub Filak wrote:
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).
Pushed both patches. Thanks!
crash-catcher@lists.fedorahosted.org
-
Denys Vlasenko -
Jakub Filak