Re: [Crash-catcher] [PATCH] new bugzilla reporter

[Denys Vlasenko]

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

[Crash-catcher] [PATCH] new bugzilla reporter

[Nikola Pajkovsky]

Dne 8.2.2010 19:37, Denys Vlasenko napsal(a):
>  Hi,
>
>  This is the review of Nikola's new bz code.
>
>
>     
> > static int32_t abrt_errno;
> >       
My idea is rid of all try and catch mechanism and replace it with our 
errno mechanism.
>  This should be a member of "struct ctx" and it should be
> just "int"
>      
What do you have against int32_t?

> > static const char* abrt_strerror(bugzilla_codes code)
> > {
> >     return NULL;
> > }
> >      
>  Unused function.
>      
Will be in future

>       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"?
>      
fixed s/get.../got.../ in all VERB3 log
>           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*/.
>      
    in /usr/include/xmlrpc-c/base.h
     230 void
     231 xmlrpc_read_string(xmlrpc_env *               const envP,
     232                                  const xmlrpc_value *  const 
valueP,
     233                                  const char **                
const stringValueP);

>       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.
>      
Right, it should be xmlrpc_int32 to be same as int32_t
fixed

    in /usr/include/xmlrpc-c/base.h
      41 typedef int32_t xmlrpc_int32;
      42     /* An integer of the type defined by XML-RPC <i4>; i.e. 32 
bit */

>  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.
>
>      
This will stay for now. I'd like to know if all xml members are ok.
>  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.
>      
right

>       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".
>      
we knew it ;)
>  int32_t ctx::get_bug_info(xmlrpc_env* env, struct bug_info* bz,
> uint32_t bug_id)
>  ^^^^^^^ just int?
>      
why are you hating so much int32_t? :D
>  {
>       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.
>      
Great idea. Will implement as soon as possible. I will be glad if I rid 
of ctx namespace in future. I hope you are ok with this.
>       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.
>      
fixed

ok, thx

-- 
Nikola

[Crash-catcher] [PATCH] new bugzilla reporter

[Denys Vlasenko]

On Tue, 2010-02-09 at 13:46 +0100, Nikola Pajkovsky wrote:
>  Dne 8.2.2010 19:37, Denys Vlasenko napsal(a):
>  > Hi,
>  >
>  > This is the review of Nikola's new bz code.
>  >
>  >
>  >    
>  >> static int32_t abrt_errno;
>  >>      
>  My idea is rid of all try and catch mechanism and replace it with our 
>  errno mechanism.
>  > This should be a member of "struct ctx" and it should be just
> "int"
>  >    
>  What do you have against int32_t? 
Because int32_t is a type with explicit bit width.
It is supposed to be used only when you _have to_ have an exactly
32-bit wide variable.

OTOH int type is an "integer of natural size for this architecture".

20 years from now, on most machines "int" will probably be 64-bit
and people would wonder "why abrt_errno is int32_t? there must be
some special reason for that...". But there is not.

--
vda

[Crash-catcher] [PATCH] new bugzilla reporter

[Gavin Romig-Koch]

On 02/23/2010 07:59 AM, Denys Vlasenko wrote:
>  Because int32_t is a type with explicit bit width.
>  It is supposed to be used only when you _have to_ have an exactly
>  32-bit wide variable.
>
>  OTOH int type is an "integer of natural size for this architecture".
>
>  20 years from now, on most machines "int" will probably be 64-bit
>  and people would wonder "why abrt_errno is int32_t? there must be
>  some special reason for that...". But there is not.
>
>      
I would encourage you to at least create a typedef for this, with a 
comment explaining the range of values, and the operations the type 
should support.   Since this is C++ you could also get stronger type 
checking by using a class containing only the functions you want this 
type to support.   GCC's optimization of single member classes is pretty 
good.

How ever you abstract this, if you abstract it at all, you should look 
at the new ISO C/C++ types like uint_fast16_t, or even uint_fast8_t.   
See 
http://www.gnu.org/software/libc/manual/html_node/Integers.html#Integers.

                                                                                          
           -gavin...