[asterisk-dev] strange libss7 coding style!

[Donny Kavanagh]

You could run one of the numerous free Virtual Machine packages, Vmware
Player, Sun Virtual Box, and run a Linux machine contained within your
windows machine :)

Donny

On Thu, Jul 23, 2009 at 11:07 PM, tian <tian00di00 at live.com> wrote:

> 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
> >
>
> _______________________________________________
> --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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.digium.com/pipermail/asterisk-dev/attachments/20090723/9220070c/attachment.htm