[asterisk-dev] strange libss7 coding style!

[tian]

I am NOT a programmer, I am even running Windows instead of Linux. I have 
some knowledge of C and I am reading through libss7 source code to learn 
about the internals of SS7 signaling. I don't know how to submit a patch but 
I have found a couple of errors in the libss7 source code, how can a person 
like me (some sort of a code reviewer but not a technician) to send 
feedbacks? Is there any guideline? Thanks!

p.s. I believe the following libss7 code contain bugs:

Version SVN
SVN Trunk
SVN Revision 269

Source file: mtp3.c

Lines: 470-476

if (LINKSET_UP_DELAY > -1) { // #define LINKSET_UP_DELAY 500
    if (ss7->linkset_up_timer > -1)
        ss7_schedule_del(ss7, &ss7->linkset_up_timer);
    ss7->linkset_up_timer = ss7_schedule_event(ss7, LINKSET_UP_DELAY, 
&linkset_up_expired, ss7);
    ss7_message(ss7, "LINKSET UP DELAYING RESETTING\n");
} else // **** this clause will NEVER be excecuted!
    ss7_linkset_up_event(ss7);


Description of the bug: since LINKSET_UP_DELAY is #define'd to be 500, the 
if condition is always true so the else clause will never be executed!


Version SVN
SVN Trunk
SVN Revision 269

Source file: mtp3.c

Lines: 452-464

/* try force uninhibit */
if (i == ss7->numlinks) {
    for (i = 0; i < ss7->numlinks; i++) {
        if (ss7->links[i]->inhibit & INHIBITED_REMOTELY) {
            if (!(ss7->links[i]->got_sent_netmsg & SENT_LFU))
                break;  // **** This break should be deleted! ****
            AUTORL(rl, ss7->links[i]);
            net_mng_send(ss7->links[i], NET_MNG_LFU, rl, 0);
            ss7_message(ss7, "Forced uninhibiting remotely inhibited link 
(no more signalling links are in service) SLC: %i ADJPC: %i\n", 
ss7->links[i]->slc, ss7->links[i]->dpc);
            break;
        }
    }
}

Bug description: the first 'break' keyword is misused and should be deleted! 
Apparently the logic of the program is to check whether a LFU has been sent 
to the far end, and if it is not the case, send a LFU, but the 'break' 
breaks the logic.


I have some other bugs, if positive replies are received, I am glad to 
report them (In the hope that installing a Linux box is not required in the 
process of bug reporting).

Regard,

Tian


--------------------------------------------------
From: "Russell Bryant" <russell at digium.com>
Sent: Friday, July 24, 2009 8:32 AM
To: "Asterisk Developers Mailing List" <asterisk-dev at lists.digium.com>
Subject: Re: [asterisk-dev] strange libss7 coding style!

> tian wrote:
>> Hi, guys!
>
> Hi!
>
> Please create a new message instead of reply to another one when
> starting a new thread.
>
>> suppose that A, B, C and D are some boolean expressions, the libss7 code
>> used something like this:
>>
>> if (  (A && B) ? 1 : (C || D)  )
>>     // statements when condition is true
>>
>>
>> I think the following would be equivalent and more clean
>>
>> if (   (A && B) || (C || D)   )
>>     // statements when condition is true
>>
>>
>> I think this is strange coding style and am wondering what benefit can we
>> get from this? Do you?
>
> Agreed.  Patches that simplify logic like this are welcome.
>
> -- 
> Russell Bryant
> Digium, Inc. | Engineering Manager, Open Source Software
> 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
> Check us out at: www.digium.com & www.asterisk.org
>
> _______________________________________________
> --Bandwidth and Colocation Provided by http://www.api-digital.com--
>
> asterisk-dev mailing list
> To UNSUBSCRIBE or update options visit:
>   http://lists.digium.com/mailman/listinfo/asterisk-dev
>