Hi,
This is the review of Nikola's new bz code.
static int32_t abrt_errno;
This should be a member of "struct ctx" and it should be just "int".
static const char* abrt_strerror(bugzilla_codes code)
{
return NULL;
}
Unused function.
const char* reporter = NULL;
xmlrpc_read_string(env, reporter_member, &reporter);
xmlrpc_DECREF(reporter_member);
if (env->fault_occurred)
{
abrt_errno = ABRTE_BUGZILLA_XMLRPC_ERROR;
return NULL;
}
if (*reporter != '\0')
{
VERB3 log("get bug reporter: %s", reporter);
^^^^^^^^^^^^^^^^^^^^ perhaps "got bug reporter", not "get"?
abrt_errno = ABRTE_BUGZILLA_OK;
return reporter;
}
free((void*)reporter);
abrt_errno = ABRTE_BUGZILLA_NO_DATA;
return NULL;
Looks like reporter should be char*, not const char* - the fact that you
need to cast it in free() hints about it. There are many other cases
where it makes sense to s/const char*/char*/.
for (int32_t i = 0; i < array_size; i++)
i should be just "int".
int32_t ctx::get_bug_id(xmlrpc_env* env, xmlrpc_value* result_xml)
{
...
xmlrpc_int bug_id = -1;
xmlrpc_read_int(env, bug, &bug_id);
xmlrpc_DECREF(bug);
...
return bug_id;
}
Apparently return type should be xmlrpc_int.
xmlrpc_value* ctx::get_member(xmlrpc_env* env, const char* member, xmlrpc_value*
result_xml)
{
xmlrpc_value* cc_member = NULL;
xmlrpc_struct_find_value(env, result_xml, member, &cc_member);
if (env->fault_occurred)
{
abrt_errno = ABRTE_BUGZILLA_XMLRPC_ERROR;
return NULL;
}
if (cc_member)
return cc_member;
abrt_errno = ABRTE_BUGZILLA_XMLRPC_MISSING_MEMBER;
return NULL;
}
I think you can just "return cc_member" without checking for NULL.
Caller can just look at the value (and I grepped for it - it actually
does exactly that). Then you can drop ABRTE_BUGZILLA_XMLRPC_MISSING_MEMBER
const - it is only used for this case.
get_bug_resolution() and get_bug_status() can likewise be simplified
to return error indicator as NULL, then ABRTE_BUGZILLA_NO_DATA is not needed too.
xmlrpc_int dup_id_int = -1;
xmlrpc_read_int(env, dup_id, &dup_id_int);
xmlrpc_DECREF(dup_id);
if (env->fault_occurred)
{
abrt_errno = ABRTE_BUGZILLA_XMLRPC_ERROR;
return -1;
}
VERB3 log("get dup_id: %i", dup_id_int);
Formally, cast is needed: (int)dup_id_int. We don't really know for sure xmlrpc_int
is "int", not "long".
int32_t ctx::get_bug_info(xmlrpc_env* env, struct bug_info* bz, uint32_t bug_id)
^^^^^^^ just int?
{
xmlrpc_value* param = xmlrpc_build_value(env, "(s)",
to_string(bug_id).c_str());
if (env->fault_occurred)
{
abrt_errno = ABRTE_BUGZILLA_XMLRPC_ERROR;
return -1;
}
...
You have many functions which return 0/-1 and set abrt_errno to
ABRTE_BUGZILLA_XMLRPC_ERROR.
It looks like abrt_errno and env->fault_occurred nearly mirror each other.
How about this: add a "xmlrpc_env env;" member to ctx, do not set abrt_errno
to ABRTE_BUGZILLA_XMLRPC_ERROR (you may actually drop ABRTE_BUGZILLA_XMLRPC_ERROR const
then)
and check "if (env.fault_occurred)" instead of "if (abrt_errno ==
ABRTE_BUGZILLA_XMLRPC_ERROR)".
With such changes, in the code above you can drop "xmlrpc_env* env" param
and drop "abrt_errno = ABRTE_BUGZILLA_XMLRPC_ERROR" assignment.
The same simplification will be possible in many other places.
xmlrpc_env env;
xmlrpc_env_init(&env);
ctx bz_server(BugzillaXMLRPC.c_str(), NoSSLVerify);
update_client(_("Logging into bugzilla..."));
if ((Login == "") && (Password == ""))
{
VERB3 log("Empty login and password");
throw CABRTException(EXCEP_PLUGIN, _("Empty login and password. Please check
Bugzilla.conf"));
}
Move "ctx bz_server" below "if()". "xmlrpc_env env" will be
simply gone
if you decide to do what I suggest in prev paragraph.
if (result)
xmlrpc_DECREF(result);
IIRC xmlrpc_DECREF(NULL) is safe, you don't need to check for NULL.
--
vda