13 Mar
2013
13 Mar
'13
5:47 p.m.
hi,
(somehow i already sent this patch to the list. but it seems to be gone.)
this patch should fix the underrun problem. it will only send radio link
failure to bsc one and then stop processing timeout counter.
regards,
andreas
13 Mar
13 Mar
10:30 p.m.
New subject: lchan->s and integer overflow
On Wed, Mar 13, 2013 at 05:47:26PM +0100, jolly wrote:
Dear jolly,
this patch should fix the underrun problem. it will
only send radio link
failure to bsc one and then stop processing timeout counter.
is the semantic of btsb->link_time_out == 0 == disabled something from the
specification or something the implementor has decided)? Either way is fine,
I would just have the commit message reflect that.
thank you for the fix
kind regards
holger
PS: So before and after the patch it appears possible that the connection
failure is sent more than once? (e.g. if the BSC doesn't release the channel
within 3 SACCH frames?). Is that intended? In both cases it is fine, it is
just that the commit message should reflect the decision you took.
9:06 a.m.
New subject: lchan->s and integer overflow
On Thu, Mar 14, 2013 at 07:47:25AM +0100, jolly wrote:
Holger Hans Peter Freyther wrote:
Hi,
this must be frustrating for you, but it really is for me as well. I
take absolutely no joy in pointing out these issues. I would prefer to
work on my code but it is something that will bite us in a commercial
setup so I a have to be the PITA here.
/* 9.4.14 Connection Failure Criterion */
if (TLVP_PRESENT(&tp, NM_ATT_CONN_FAIL_CRIT) &&
- (TLVP_LEN(&tp, NM_ATT_CONN_FAIL_CRIT) >= 2) &&
- *TLVP_VAL(&tp, NM_ATT_CONN_FAIL_CRIT) == 0x01) {
+ (TLVP_LEN(&tp, NM_ATT_CONN_FAIL_CRIT) >= 2)) {
const uint8_t *val = TLVP_VAL(&tp, NM_ATT_CONN_FAIL_CRIT);
- btsb->radio_link_timeout = val[1];
+ if (val[0] == 0x01 && val[1] >= 4)
+ btsb->radio_link_timeout = val[1];
}
Here you point out that the range of 4 to UINT8_MAX is coming from the
specification. Which is very good. The way it is done is a violation of
the principle of least surprise though. If a BSC sends the value '2' it
doesn't make sense that '32' is used. The configurator/implementor
certainly wanted to have a low value.
IMHO the right thing to do is to NACK the set bts attributes with
a descriptive error message. The same goes for the conn fail crit
being present but not of the type we support.
holger
PS:
the lchan->s handling is fine. The switch/case has grown to a state
I would move the link failure handling to a new helper function though,
this way one could even write a unit test for the handling...
switch (data_ind->sapi) {
case GsmL1_Sapi_Sacch:
- /* process radio link timeout coniter S */
+ /* process radio link timeout counter S */
11:45 a.m.
New subject: lchan->s and integer overflow
Holger Hans Peter Freyther wrote:
Here you point out that the range of 4 to UINT8_MAX is
coming from the
specification. Which is very good. The way it is done is a violation of
the principle of least surprise though. If a BSC sends the value '2' it
doesn't make sense that '32' is used. The configurator/implementor
certainly wanted to have a low value.
IMHO the right thing to do is to NACK the set bts attributes with
a descriptive error message. The same goes for the conn fail crit
being present but not of the type we support.
hi holger,
the range is actually 4..64, so i send a NACK now, if the given
attribute is not is that range. also the counting of S value is moved to
a seperate function. check out the attached patch.
regards,
andreas
15 Mar
15 Mar
9:17 p.m.
New subject: lchan->s and integer overflow
On Thu, Mar 14, 2013 at 11:45:42AM +0100, jolly wrote:
the range is actually 4..64, so i send a NACK now, if
the given
attribute is not is that range. also the counting of S value is moved to
a seperate function. check out the attached patch.
thank you. this looks good. feel free to push it.
holger
4070
days inactive
4072
days old
6 comments
3 participants
participants (3)
-
Andreas Eversberg -
Holger Hans Peter Freyther -
jolly