[Home]

Summary:ASTERISK-01664: Libpri does not support CodeSet 6 (struct q931_ie is invalid)
Reporter:mschaefe (mschaefe)Labels:
Date Opened:2004-05-20 09:34:55Date Closed:2004-11-08 12:26:38.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) codeset6.patch
( 1) libpri_shift.diff
( 2) libpri.shift2.diff
( 3) pri_q931.h.shift.diff
( 4) q931.c.shift.diff
Description:I'll quote from the mailing list post I sent...

I'm trying to get asterisk to talk on a specially configured PRI.  However, I'm getting a SETUP message which contains the following codes at the end of the message: 96 01 01 00.  According to the AT&T PRI manual, 96 means "Locking Switch to CodeSet 6."  It also says that a PRI interface SHALL implement the switch to CodeSet 6 and 7.  I have no idea what this is, but I know that a few things are apparently not right.

1) the Asterisk IE number (pri_q931.h) for "Locking Shift" is 0x90.  While technically correct, the problem is that the lower nibble has the codeset to switch to.  So, really, it should be 0x90 to 0x97 for Locking Shift and 0x98-0x9F for Non-Locking Shift.  According to the docs,
Non-Locking Shift is not implemented.

2) q931_ie is created (Little Endian) with the bitfield ie:7 and f:1. I'm not sure what the purpose is, but Locking Shift to CodeSet 6 is coming across as IE 0x22 and is causing the SETUP to barf on the "UNKNOWN IE"

In the libpri source tree, there is the following code in pri_q931.h

/* Information element format */
typedef struct q931_ie {
#if __BYTE_ORDER == __BIG_ENDIAN
u_int8_t f:1;
u_int8_t ie:7;
#else
u_int8_t ie:7;
u_int8_t f:1;
#endif
u_int8_t len;
u_int8_t data[0];
} q931_ie;

I believe this code is incorrect.  Here's why:

In q931.c, the following code exists:

if (!(ie->ie & 0xf0) && (y < 0))
   mandies[MAX_MAND_IES - 1] = ie->ie;

If the length of ie is 7 bits instead of the normal 8 bits, then that comparison should be 0x70.  The same problem is exhibited when the following might be received as a valid information element:

#define Q931_LOCKING_SHIFT 0x90

In the current code, that ie will never be parsed.

Suggest you change the code to:

typedef struct q931_ie {
u_int8_t ie:8;
u_int8_t len;
u_int8_t data[0];
} q931_ie;

Anywhere you look for ie->f, you can replace it with ie->ie & 0x80, which does not break endianness.

Also the #define above is incorrect.  Q931_LOCKING_SHIFT is 0x90 through 0x97 from what I've seen.  Asterisk barfs on any locking shift due to some poor coding.

Also, according to the AT&T reference manual
96 01 01 00 is:
96 = Locking Shift to CodeSet 6
01 = Originating Line Info Element (in CodeSet 6)
01 = Length of IE
00 = II (POTS) (see http://www.nanpa.com/number_resource_info/ani_ii_assignments.html)

This is obviously handled incorrectly within Asterisk...

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

Also reference http://www.att.com/cpetesting/41459.html for AT&T's PRI specification.  I assume this is 4/5ess protocol and may have to be handled as such.
Comments:By: Mark Spencer (markster) 2004-05-21 00:23:58

I'm going to need access to a machine connected to this sort of PRI in order to implement the locking shift.
By: mschaefe (mschaefe) 2004-05-21 07:33:24

Just tell me how you would implement it and I'll do it.  I have it hacked right now so that it recognizes the CodeSet 6 change, but it does not interpret the following IE's (just one on a call setup) as CodeSet 6 IE's.

Could you tell me if my interpretation of struct q931_ie is valid?  I can submit a patch for that.
By: Mark Spencer (markster) 2004-05-21 11:04:23

You've got two other patches in process that haven't been disclaimed according to bugnotes.  I'm concerned that if you do it, the bug will end up in limbo fixed but unable to be used by anyone else.  If I login and fix it then presumably there is no question.  Being able to support multiple code sets is going to be something significant if done in an extensible manor.
By: mschaefe (mschaefe) 2004-05-21 13:19:10

I'm going to do the coding at home on my own computer own my own time.  I'll submit them to myself at work as a patch.  Then I'll put my work hat on when I get into work and test the patch that I received from the "Asterisk team".  If it works, I'll e-mail myself "good job".  And when I get home I'll submit it to the team with the appropriate disclaimer.

The problem is that that specific instance of Asterisk is running as a switch for at least six ongoing R&D projects (Two inbound PRI's and four lineside T1's + some SIP phones) and I need to schedule the downtime accordingly.  I can't just open up that machine for ongoing development.
By: Mark Spencer (markster) 2004-05-21 13:37:59

In general, the presense of the f-bit indicates that this is a single-octet information element.  therefore you can just test if (ie & 0x70 == 0x10) then you have locking or non-locking shift to the appropriate codeset.  After the shift then the following information element or elements (in the case of a locking shift) will have to be handled on a switch-specific code -- at this point with only 5e and 4e being supported).

If you'd like to do it, go for it then.
By: mschaefe (mschaefe) 2004-05-21 14:14:50

Yes, but the way you deal with ie's (struct ie ies) is to have a one-to-one mapping between an ie hex value and the associated codes.  Thus, unless you want me to change the ie handling piece, I would have to add #defines for all values from 0x80 to 0x9F as LOCKING or NON_LOCKING_SHIFT.  I can have them all point to the same function and do what you suggest.

Next question, how would you implement the codeset shift?  I would probably do it with a local codeset variable in q931_receive.  The shift is only locked (AFAIK) for that specific message.  Would you implement it as an extra dimension to struct ie ies?  e.g. ies[codeset][ienum].transmit, etc.?  Or would you implement it as a subloop in q931_receive?
By: Mark Spencer (markster) 2004-05-22 15:08:05

Just implement it in q931_receive as a special case I guess, because if you didn't do it that way you'd have to either modify the PRI structure or have an extra parameter that got passed for the codeset.
By: mschaefe (mschaefe) 2004-05-23 21:26:09

I will have to modify the PRI structure to do SDN Marking (Network-Specific Facility IE) because it's a per-PRI option, so it may not be that big of a deal.
By: Mark Spencer (markster) 2004-05-27 22:08:33

So what's the story here?
By: mschaefe (mschaefe) 2004-06-09 21:07:37

I was on vacation.  It looks like you're right.  It seems pretty kludgy to have it as a special case, but I think it's the only IE that affects the actual message processing.  Maybe we can revisit how this is done in a later rearchitecture effort.
By: mschaefe (mschaefe) 2004-06-11 23:57:14

Okay, I solved the problem.  It seems a little cleaner than my original thought, but the proliferation of "LOCKING_SHIFT_X" seems a bit much.  Please take a look.

Comments:
1 - Removed BIG_ENDIAN from q931_ie - need to verify this is correct.
2 - Moved FACILITY IE to CodeSet 6.  In AT&T this is correct, but not sure about non-AT&T PRI's.  Please verify
3 - Lots of rearchitecture to add codesets
By: Paul Cadach (pcadach) 2004-06-12 00:15:19

I disagree with such type of LOCKING SHIFT support. IMHO better is to have different ies arrays for each codeset... Also, FACILITY IE allowed for CodeSet 0 (at least for EuroISDN), so you must have two different handlers for FACILITY IE for different switch types. Usage of many ies arrays and special handling of LOCKING/NON-LOCKING SHIFTs allows to handle such cases more cleanly.

Give me a time - I'll provide a patch with my point-of-view and your handling of FACILITY and ORIGINATING LINE INFO IEs.
By: Paul Cadach (pcadach) 2004-06-12 02:52:02

Something like attached (CAUTION! FULLY NOT TESTED!!!).

Some description of patch:
1) Combine codeset and IE code to single value (codeset << 8 | ie code) - lookups could be done with single compare, but some places requires to extract codeset from full (i.e. including codeset) IE code;
2) dump, transmit and receive function prototypes moved to #define to change argument list more quickly (just single line instead of walking through all q931.c);
3) ie->f field is removed - it is not nessesary to extract high bit of IE code because it is "part" of IE code (IE codes specified with exact value of high bit);
4) as described in TR41459, Locking Shift to codeset 0 isn't possible, Non-Locking shift only;
5) use latest pri_q931.h diff or manually change code of Q931_SENDING_COMPLETE to 0xa1 (it's single-byte IE, forgotten to change).

Good luck!
By: mschaefe (mschaefe) 2004-06-12 07:24:54

I actually looked at the approach of having multiple arrays and it doesn't seem to be any better.  There are a number of reasons:

1)  The q931_receive code becomes significantly more complex because it must know about all the different arrays.
2)  There may be some advantage to the fact that you can have a pointer into the ies array because by TR4159, all ies must come in numeric order.  You can make the structure more efficient and reduce search time for incoming messages.

Also, looking at your patch:
1)   I don't see much difference between adding a codeset and hacking codeset into the ie, except that hacking makes maintenance much more difficult.  Then people have to know that the ie value is really NOT an ie value but ie + codeset.
2)   If you use the ability to restrict ie's to codesets, you wind up with an array almost identical to mine.  Since there aren't a lot of ie's that span codesets, you'll get there sooner rather than later.

At this point, I prefer to wait and see what Mark says before continuing.
By: Paul Cadach (pcadach) 2004-06-12 09:11:30

mschaefe, your patch violates support for non-locking shift. My patch also violates order of codesets and ie codes... :(((

Combining codeset with IE code allows to minimize memory usage for ies table, and simplifies search of requested IE code within table. Also, binary search is possible and is SIMPLE, while binary search through multiple values is complex a little (requires many condition checks).

As for difficulity of maintenance - you could split combined IE code into codeset and "clean" IE's code, so it's not so big problem.

Ok, lets listen for Mark.
By: Paul Cadach (pcadach) 2004-06-12 11:38:21

Patch set is updated:
1) fixed mistake in pri_q931.h;
2) some sanity checks for correctness of shifts and IE order;
3) non-locking shifted IEs (as pointed by TR41459) are ignored for switchtypes PRI_SWITCH_LUCENT5E and PRI_SWITCH_ATT4ESS (I'm not sure this list is correct, just for example);
4) Q931_DISPLAY is moved after Q931_PROGRESS_INDICATOR due to misorder (by IE code);
5) slightly tested with testlibpri.
By: Paul Cadach (pcadach) 2004-06-14 03:08:55

libpri_shift.diff is cumulative patch (as suggested by Mark), nothing new except for small fixes (0xf0 mask was used where 0xf8 is required) and ability to show warning messages only when debugging is enabled.

Patch is applied to my working system - all works without problem (and without shifts :( ).

mschaefe, could you test this patch on your equipment?
By: mschaefe (mschaefe) 2004-06-14 09:04:40

My patch does not violate support for non-locking shifts.  Since the ie number is included with a "", the message will be received.  Yes, I forgot to change the comment to "NON-locking shift to codeset x", but all non-locking shifts will be handled as appropriate - they are ignored.  Only locking shifts will change the codeset.

As far as proper handling of codeset changes, I am assuming the happy path.  I've noticed that there is not a lot of error handling in libpri, so I wasn't about to start coding in all the required error handling for shifts.  Simply ignoring the shift to a lower codeset is not appropriate since it's a protocol violation.
By: mschaefe (mschaefe) 2004-06-14 16:34:06

I really don't care how this gets solved.  Obviously, it's easier for me if my solution is the one that gets accepted, but I think there are some great ideas that PCadach has, for example, making the transmit, send and receive functions macros - makes it much easier to modify the parameters.

In combining the codeset and IE number, my past experience says that it is better to be explicit about what things are as much as possible rather than save a clock cycle here or there.  Of course, this is coming from a C++ programmer and I know that C programmers have a joy in making things as quick and efficient as possible without being limited by maintainability.  In that respect, I believe that it is a much more efficient and compact method of solving the problem than I proposed.

I'm just trying to make this as easy as possible.  I've had to maintain this delta, and a delta to support NSF for three months now and I'd like to get them in the source code so I can go back to "cvs update".

BTW: What exactly makes this a "feature" rather than a bug?  I contend that 4/5ess switch support is broken without codeset support.
By: Mark Spencer (markster) 2004-06-14 17:17:04

Okay I've merged pcadach's patch.  Looks good from this side too.  We'll update on our switch too.
By: mschaefe (mschaefe) 2004-06-16 10:17:56

Patch doesn't work.  I get
Jun 16 10:59:39 WARNING[-1189385296]: chan_zap.c:6588 zt_pri_error: PRI: !! < Unknown IE 150 (len = 1)
< IE: Originating Line Information (len = 3)
Jun 16 10:59:39 WARNING[-1189385296]: chan_zap.c:6588 zt_pri_error: PRI: XXX Missing mandatory IE 1537/Originating Line Information XXX

Obviously, this is not detecting the shift to codeset 6.  I downloaded the source straight from cvs and built it from scratch.
By: mschaefe (mschaefe) 2004-06-16 10:34:22

There are two bugs.  The first bug is that dumpie also needs the codeset logic.  The second bug is a stupid catchall at the end of q931_receive:

if (!(ie->ie & 0xf0) && (y < 0))                                         mandies[MAX_MAND_IES - 1] = Q931_FULL_IE(cur_codeset, ie->ie);

This needs to be updated to only care about codeset 0, I assume.
By: Paul Cadach (pcadach) 2004-06-16 13:34:44

Fixed and verified. Description of "Originating Line Information" IE and ability of transmission is added. See attached patch.
By: Mark Spencer (markster) 2004-06-16 14:42:14

Fixed in CVS (again)
By: Paul Cadach (pcadach) 2004-11-08 12:26:38.000-0600

Reminder sent to mschaefe

Could you confirm your connection uses NI2 PRI signalling? I need it to make distinct to use Originating Line information and Generic digits for passing/receiving ANI II digits over different switch types.