[Home]

Summary:ASTERISK-17642: [patch] ISDN party subaddress odd_even_indicator inconsitency / undocumented
Reporter:Martin Vit (festr)Labels:
Date Opened:2011-04-04 06:35:29Date Closed:2011-04-07 05:30:28
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Channels/chan_dahdi
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) bug19062.diff.txt
( 1) bug19062.diff2.txt
( 2) patch
Description:I'm playing with party subaddress -> Call(DAHDI/g1/1234:u0a23a4)

this syntax should set party subaddress to user specified and odd_even_indicator to 0. Unfortunatly, odd_even_indicator is overwritten in channels/sig_pri.c in function sig_pri_party_subaddress_from_ast: pri_subaddress->odd_even_indicator = (length & 1);

So you cannot set your own odd_even_indicator specified by prefix 'u' or 'U' as it depends always on string length. I've hacked it by uncommenting "pri_subaddress->odd_even_indicator = (length & 1);"

****** ADDITIONAL INFORMATION ******

I think that calling subbaddress field is undocumented, I cannot find syntax for DAHDI/g1/1234:[uUnN]subaddr so I had to find it in source code.
Comments:By: Martin Vit (festr) 2011-04-04 07:34:44

Uploading patch which sets odd_even indicator according to what user sets in subaddr prefix. This applies only for uU prefix which sets "user specified" subbaddress. Without this modification, odd_even is set according to its length which is maybe right but uU prefix is useless than. Sometimes I need to passthrough odd_even indicator from one E1 to another E1 and I need to do 1:1 mapping which is not possible right now. My suggestion is to add another option which tells to use user-specified and automatic odd-even indicator (l prefix?).
By: Alec Davis (alecdavis) 2011-04-06 04:01:05

festr:
confession time. Agreed in sig_pri_call with :Uu the inital idea (in PRI-64 diff8 patch) was to allow User Specifed Odd/Even flag to be automatically calculated.

By diff11 I'd made the change in sig_pri_call to allow the user to specify Odd/Even, but missed the required change in function sig_pri_party_subaddress_from_ast.

So we shouldn't need another option, and your patch seems correct.

Just need rmudgett to comment, and I'm sure he'll also say oops...


By: Alec Davis (alecdavis) 2011-04-06 04:06:05

richard: your thoughts please.
By: Richard Mudgett (rmudgett) 2011-04-06 10:07:32

The odd_even_indicator probably should just be copied from the source subaddress structure.  Just have to trust the source to be correct. :)  However, the odd/even flag could then become wrong if the conversion had to truncate the ASCII hex string because of the BCD buffer size.  Truncation should never be necessary if the subaddress is to get to the other end correctly.

The existing code is just wrong.  That length is the length of the BCD buffer not the ASCII hex string length (or adjusted ASCII hex string length if truncation was needed).
By: Martin Vit (festr) 2011-04-06 10:15:48

if I receive user type dnid-subaddr I cannot guess even-odd indicator from BCD right? so the lenght calculation is just wrong and uU should be used always?
By: Martin Vit (festr) 2011-04-06 10:15:49

if I receive user type dnid-subaddr I cannot guess even-odd indicator from BCD right? so the lenght calculation is just wrong and uU should be used always?
By: Richard Mudgett (rmudgett) 2011-04-06 11:16:51

The received subaddress is always converted to an ASCII hex string if the type is user specified.  The ASCII hex string can then be easily manipulated by the dialplan.

The odd/even flag can be determined using the ASCII hex string length.  If the ASCII hex string length is odd then the odd/even flag needs to indicate odd.  The original code is just wrong because it is using the filled length of the BCD buffer to determine the odd/even flag.

The sig_pri_party_subaddress_from_ast() function is called by several places to convert from Asterisk to PRI representation.  Dialed subaddress, Caller ID subaddress, and redirecting to/from subaddresses.

There are a few ways to fix this issue.
1) Calculate the odd/even flag correctly from the ASCII hex string and ignore the odd/even indication from the uU setting.
2) Calculate the odd/even flag correctly from the ASCII hex string and then make an exception to always use the odd/even indication from the uU setting for the dialed subaddress.
3) Always trust the source odd/even flag.

Option one is the safest.  The odd/even flag will never be wrong.
Option two is a hybrid between one and three.
Option three leaves it up to the user to either get it right or allow him to play games with the odd/even flag.

Your patch fixes the incorrect code by trusting the setting of the odd_even_indicator flag.
By: Martin Vit (festr) 2011-04-06 11:37:03

way 1) using odd ASCII hex string, isnt it wrong? I though that ASCII string should be always even in case of "user" type (with 0 ending or something like this)

way 2) odd/even flag is releated only in case you put uU which sets type to "user". type (0) does not use odd/even indicator.

way 3) there can be no source in case you are making call (not forwarding it)
By: Richard Mudgett (rmudgett) 2011-04-06 12:32:47

This whole discussion has been dealing with the user specified format of the subaddress.  The NSAP format has its own handling rules and has NOT been discussed in this issue.

Lets say you receive this user specified BCD subaddress: 0x12 0x34 0x50 with the odd flag set.

This is converted to an odd length ASCII hex string "12345" because that is the number of BCD digits encoded and the odd flag saying that the trailing pad digit is not used.  You can then easily manipulate the digits without worrying about the presence of a pad digit.

You could simply append another digit to the subaddress ASCII hex string so it becomes "123456" which is then converted to BCD: 0x12 0x34 0x56 with the odd flag cleared.

The purpose of the odd/even flag is to indicate if the last nybble in the last byte is used.

way 1) The odd/even flag is automatically calculated based upon the length of the ASCII hex string as shown in the example.  There is no pad character in the string to always make the string an even length.

way 2) Correct.  The odd/even flag is automatically calculated as in way 1.  The Dial() application then overrides the calculated odd/even flag based upon the user specified uU odd/even indication.

way 3) Of course there is a source.  You are the source when you use the uU prefix in the Dial application.
By: Martin Vit (festr) 2011-04-06 12:49:55

now I understand, thank you for explanation. I've checked that I'm receiving even/odd ASCII according to what I set in odd/even flag. I missed that in my tests and as I was working always with even testing ASCII string. So now it should be only fixed lenght calculation?
By: Richard Mudgett (rmudgett) 2011-04-06 13:11:47

Do you mean way 1 verses way 3?

Way 1 is the safest. The odd/even flag cannot be wrong and eliminates dialplan programming error by making the u and U prefix identical.

Way 3 leaves it up to the user to either get it right or allow him to play games with the odd/even flag.  Trust the source. :)

Way 1 should really be done because of its advantages over the other ways even though it is slightly more complicated.  It involves fixing the odd/even calculation in sig_pri_party_subaddress_from_ast() and updating the subaddress parsing code/comments in sig_pri_call().
By: Martin Vit (festr) 2011-04-06 13:27:01

I'm voting for way 1.
By: Alec Davis (alecdavis) 2011-04-06 13:56:40

bug19062.diff.txt
fixes the Odd/Even calculation
Still is automatically calculated, the dialplan cannot influence the flag.

Untested though.
By: Richard Mudgett (rmudgett) 2011-04-06 14:09:24

alecdavis,

The odd/even calculation should take into account truncation.  If the strlen() is greater than 2*sizeof(pri_subaddress->data) then the odd/even flag should be set as even.

Other than that it looks to work.
By: Alec Davis (alecdavis) 2011-04-06 15:56:06

bug19062.diff2.txt
as ~133465 but also check for truncation.

still untested.


By: Alec Davis (alecdavis) 2011-04-06 16:28:30

Changed summary, as it applies to BRI also.

Please test and feedback.
By: Martin Vit (festr) 2011-04-07 03:47:06

I've applied patch bug19062.diff2.txt and it works. Dial/DAHDI/...:uA gives dnid-subaddr-odd=1, Dial...:uAA gives dnid-subaddr-odd)=0. Thank you guys.
By: Alec Davis (alecdavis) 2011-04-07 05:06:09

I also tested, :u with 39 40 and 41 digits, with the correct results.
By: Digium Subversion (svnbot) 2011-04-07 05:19:33

Repository: asterisk
Revision: 313001

U   branches/1.8/channels/sig_pri.c

------------------------------------------------------------------------
r313001 | alecdavis | 2011-04-07 05:19:32 -0500 (Thu, 07 Apr 2011) | 13 lines

Fix ISDN calling subaddr User Specified Odd/Even Flag

Calculation of the Odd/Even flag was wrong.
Implement correct algo, and set odd/even=0 if data would be truncated.
Only allow automatic calculation of the O/E flag, don't let dialplan influence.

(closes issue ASTERISK-17642)
Reported by: festr
Patches:
     bug19062.diff2.txt uploaded by alecdavis (license 585)
Tested by: festr, alecdavis, rmudgett


------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=313001
By: Digium Subversion (svnbot) 2011-04-07 05:30:27

Repository: asterisk
Revision: 313005

_U  trunk/
U   trunk/channels/sig_pri.c

------------------------------------------------------------------------
r313005 | alecdavis | 2011-04-07 05:30:27 -0500 (Thu, 07 Apr 2011) | 19 lines

Merged revisions 313001 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.8

........
 r313001 | alecdavis | 2011-04-07 22:19:31 +1200 (Thu, 07 Apr 2011) | 13 lines
 
 Fix ISDN calling subaddr User Specified Odd/Even Flag
 
 Calculation of the Odd/Even flag was wrong.
 Implement correct algo, and set odd/even=0 if data would be truncated.
 Only allow automatic calculation of the O/E flag, don't let dialplan influence.
 
 (closes issue ASTERISK-17642)
 Reported by: festr
 Patches:
       bug19062.diff2.txt uploaded by alecdavis (license 585)
 Tested by: festr, alecdavis, rmudgett
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=313005