[prev in list] [next in list] [prev in thread] [next in thread]
List: asterisk-dev
Subject: Re: [asterisk-dev] strange libss7 coding style!
From: "tian" <tian00di00 () live ! com>
Date: 2009-07-24 5:59:15
Message-ID: BLU133-DS5610999C62C91EB3DBDD5BC190 () phx ! gbl
[Download RAW message or body]
UbJGR;7b MIME 8qJ=5D6`7=SJ<~!#
[Attachment #2 (multipart/alternative)]
UbJGR;7b MIME 8qJ=5D6`7=SJ<~!#
Thank you Donny, I will try one of them.
From: Donny Kavanagh
Sent: Friday, July 24, 2009 11:31 AM
To: Asterisk Developers Mailing List
Subject: Re: [asterisk-dev] strange libss7 coding style!
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@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@digium.com>
Sent: Friday, July 24, 2009 8:32 AM
To: "Asterisk Developers Mailing List" <asterisk-dev@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
--------------------------------------------------------------------------------
_______________________________________________
--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
[Attachment #5 (text/html)]
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN">
<HTML><HEAD>
<META http-equiv=Content-Type content=text/html;charset=iso-8859-1>
<META content="MSHTML 6.00.2900.5803" name=GENERATOR></HEAD>
<BODY id=MailContainerBody
style="PADDING-RIGHT: 10px; PADDING-LEFT: 10px; PADDING-TOP: 15px" leftMargin=0
topMargin=0 CanvasTabStop="true" name="Compose message area">
<DIV><FONT face=宋体 size=2>Thank you Donny, I will try one of \
them.</FONT></DIV> <DIV style="FONT: 10pt Tahoma">
<DIV><BR></DIV>
<DIV style="BACKGROUND: #f5f5f5">
<DIV style="font-color: black"><B>From:</B> <A title=donnyk@gmail.com
href="mailto:donnyk@gmail.com">Donny Kavanagh</A> </DIV>
<DIV><B>Sent:</B> Friday, July 24, 2009 11:31 AM</DIV>
<DIV><B>To:</B> <A title=asterisk-dev@lists.digium.com
href="mailto:asterisk-dev@lists.digium.com">Asterisk Developers Mailing List</A>
</DIV>
<DIV><B>Subject:</B> Re: [asterisk-dev] strange libss7 coding
style!</DIV></DIV></DIV>
<DIV><BR></DIV>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 :)<BR><BR>Donny<BR><BR>
<DIV class=gmail_quote>On Thu, Jul 23, 2009 at 11:07 PM, tian <SPAN
dir=ltr><<A
href="mailto:tian00di00@live.com">tian00di00@live.com</A>></SPAN> wrote:<BR>
<BLOCKQUOTE class=gmail_quote
style="PADDING-LEFT: 1ex; MARGIN: 0pt 0pt 0pt 0.8ex; BORDER-LEFT: rgb(204,204,204) \
1px solid">I am NOT a programmer, I am even running Windows instead of Linux. I
have<BR>some knowledge of C and I am reading through libss7 source code to
learn<BR>about the internals of SS7 signaling. I don't know how to submit a
patch but<BR>I have found a couple of errors in the libss7 source code, how
can a person<BR>like me (some sort of a code reviewer but not a technician) to
send<BR>feedbacks? Is there any guideline? Thanks!<BR><BR>p.s. I believe the
following libss7 code contain bugs:<BR><BR>Version SVN<BR>SVN Trunk<BR>SVN
Revision 269<BR><BR>Source file: mtp3.c<BR><BR>Lines: 470-476<BR><BR>if
(LINKSET_UP_DELAY > -1) { // #define LINKSET_UP_DELAY 500<BR>
if (ss7->linkset_up_timer > -1)<BR>
ss7_schedule_del(ss7, &ss7->linkset_up_timer);<BR>
ss7->linkset_up_timer = ss7_schedule_event(ss7,
LINKSET_UP_DELAY,<BR>&linkset_up_expired, ss7);<BR>
ss7_message(ss7, "LINKSET UP DELAYING RESETTING\n");<BR>} else // ****
this clause will NEVER be excecuted!<BR>
ss7_linkset_up_event(ss7);<BR><BR><BR>Description of the bug: since
LINKSET_UP_DELAY is #define'd to be 500, the<BR>if condition is always true so
the else clause will never be executed!<BR><BR><BR>Version SVN<BR>SVN
Trunk<BR>SVN Revision 269<BR><BR>Source file: mtp3.c<BR><BR>Lines:
452-464<BR><BR>/* try force uninhibit */<BR>if (i == ss7->numlinks)
{<BR> for (i = 0; i < ss7->numlinks; i++) {<BR>
if (ss7->links[i]->inhibit & INHIBITED_REMOTELY)
{<BR> if
(!(ss7->links[i]->got_sent_netmsg & SENT_LFU))<BR>
break; // **** This break
should be deleted! ****<BR> AUTORL(rl,
ss7->links[i]);<BR>
net_mng_send(ss7->links[i], NET_MNG_LFU, rl, 0);<BR>
ss7_message(ss7, "Forced uninhibiting remotely
inhibited link<BR>(no more signalling links are in service) SLC: %i ADJPC:
%i\n",<BR>ss7->links[i]->slc, ss7->links[i]->dpc);<BR>
break;<BR>
}<BR> }<BR>}<BR><BR>Bug description: the first 'break'
keyword is misused and should be deleted!<BR>Apparently the logic of the
program is to check whether a LFU has been sent<BR>to the far end, and if it
is not the case, send a LFU, but the 'break'<BR>breaks the logic.<BR><BR><BR>I
have some other bugs, if positive replies are received, I am glad to<BR>report
them (In the hope that installing a Linux box is not required in
the<BR>process of bug
reporting).<BR><BR>Regard,<BR><BR>Tian<BR><BR><BR>--------------------------------------------------<BR>From: \
"Russell Bryant" <<A
href="mailto:russell@digium.com">russell@digium.com</A>><BR>Sent: Friday,
July 24, 2009 8:32 AM<BR>To: "Asterisk Developers Mailing List" <<A
href="mailto:asterisk-dev@lists.digium.com">asterisk-dev@lists.digium.com</A>><BR>Subject: \
Re: [asterisk-dev] strange libss7 coding style!<BR>
<DIV>
<DIV></DIV>
<DIV class=h5><BR>> tian wrote:<BR>>> Hi, guys!<BR>><BR>>
Hi!<BR>><BR>> Please create a new message instead of reply to another
one when<BR>> starting a new thread.<BR>><BR>>> suppose that A, B,
C and D are some boolean expressions, the libss7 code<BR>>> used
something like this:<BR>>><BR>>> if ( (A && B) ? 1 :
(C || D) )<BR>>> // statements when condition is
true<BR>>><BR>>><BR>>> I think the following would be
equivalent and more clean<BR>>><BR>>> if ( (A && B)
|| (C || D) )<BR>>> // statements when condition is
true<BR>>><BR>>><BR>>> I think this is strange coding style
and am wondering what benefit can we<BR>>> get from this? Do
you?<BR>><BR>> Agreed. Patches that simplify logic like this are
welcome.<BR>><BR>> --<BR>> Russell Bryant<BR>> Digium, Inc. |
Engineering Manager, Open Source Software<BR>> 445 Jan Davis Drive NW -
Huntsville, AL 35806 - USA<BR>> Check us out at: <A
href="http://www.digium.com" target=_blank>www.digium.com</A> & <A
href="http://www.asterisk.org"
target=_blank>www.asterisk.org</A><BR>><BR>>
_______________________________________________<BR>> --Bandwidth and
Colocation Provided by <A href="http://www.api-digital.com--"
target=_blank>http://www.api-digital.com--</A><BR>><BR>> asterisk-dev
mailing list<BR>> To UNSUBSCRIBE or update options visit:<BR>> <A
href="http://lists.digium.com/mailman/listinfo/asterisk-dev"
target=_blank>http://lists.digium.com/mailman/listinfo/asterisk-dev</A><BR>><BR><BR>_______________________________________________<BR>--Bandwidth \
and Colocation Provided by <A href="http://www.api-digital.com--"
target=_blank>http://www.api-digital.com--</A><BR><BR>asterisk-dev mailing
list<BR>To UNSUBSCRIBE or update options visit:<BR> <A
href="http://lists.digium.com/mailman/listinfo/asterisk-dev"
target=_blank>http://lists.digium.com/mailman/listinfo/asterisk-dev</A><BR></DIV></DIV></BLOCKQUOTE></DIV><BR>
<P>
<HR>
<P></P>_______________________________________________<BR>--Bandwidth and
Colocation Provided by http://www.api-digital.com--<BR><BR>asterisk-dev mailing
list<BR>To UNSUBSCRIBE or update options visit:<BR>
http://lists.digium.com/mailman/listinfo/asterisk-dev</BODY></HTML>
_______________________________________________
--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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic