GCR (osmo_store16be/osmo_store32be)
Hi All, I was working a few months back on some LCLS implementation with bts-loop and I got it working, including that the pbx can use SIP Re-INVITES to get in and out of the audio loop during calls. However, I only ever tested it with one call, that is with two phones. When I tested it on alive site, immediately there where problems so I reverted and left it. Looking at it again now, there's a very obvious problem which is that osmo-msc generates the same GCR for (almost) every call. I had noticed this before but for some reason I thought I had seen it generating different GCR for a second simultaneous call, but no. Anyway, the above is extraneous info to the question: Could somebody take a look at this: https://gitea.osmocom.org/cellular-infrastructure/osmo-msc/src/branch/master... where we have: osmo_store32be(trans->callref, lcls->gcr.cr); osmo_store16be(use_lac ? trans->msc_a->via_cell.lai.lac : trans->msc_a->via_cell.cell_identity, lcls->gcr.cr + 3); Now, If I change the order, such that would seem logical: osmo_store16be(use_lac ? trans->msc_a->via_cell.lai.lac : trans->msc_a->via_cell.cell_identity, lcls->gcr.cr + 3); osmo_store32be(trans->callref, lcls->gcr.cr); Then I get a different GCR, reflecting the trans->callref for each call. But am I then maybe overwriting the LAC/CI ? Would seem to make sense, but I'm just not sure if that is all there is to it, as I don't really grok osmo_store_xxxx for(i = 0; i < n; q[i] = (x >> ((n - 1 - i) * 8)) & 0xFF, i++); Is it just a simple order error, and is everything OK with lcls->gcr.cr + 3 as the pointer *p passed to osmo_store32be_ext() ? given that: struct osmo_lcls *lcls; where: struct osmo_lcls { struct osmo_gcr_parsed gcr; }; struct osmo_gcr_parsed { uint8_t cr[5]; }; I guess I'm still not really 100% on the char/uint8_t thing and advancing pointers. Thanks! k.
Hi Keith, On 12.01.2023 10:05, Keith wrote:
Could somebody take a look at this:
https://gitea.osmocom.org/cellular-infrastructure/osmo-msc/src/branch/master...
where we have:
osmo_store32be(trans->callref, lcls->gcr.cr); osmo_store16be(use_lac ? trans->msc_a->via_cell.lai.lac : trans->msc_a->via_cell.cell_identity, lcls->gcr.cr + 3);
I don't have any deep knowledge of the related code, but I see weird things happening in the code you're pointing to above: * the lcls->gcr.cr[] is only 5 octets large, * the code stores 4 + 2 = 6 octets in that buffer. | 0 | 1 | 2 | 3 | 4 | ^^^^^^^^^^^^xxx CallRef at &lcls->gcr.cr[0] ^^^^^^^ LAC/CID at &lcls->gcr.cr[3] As can be seen, LAC/CID overwrites last octet of the CallRef. This looks like a bug to me, because we're basically loosing an essential part of the CallRef (e.g. 0x80000023 becomes 0x800000??).
Now, If I change the order, such that would seem logical:
osmo_store16be(use_lac ? trans->msc_a->via_cell.lai.lac : trans->msc_a->via_cell.cell_identity, lcls->gcr.cr + 3); osmo_store32be(trans->callref, lcls->gcr.cr);
Then I get a different GCR, reflecting the trans->callref for each call.
If you change the order, the CallRef overwrites half of the LAC/CID: | 0 | 1 | 2 | 3 | 4 | xxx~~~~ LAC/CID at &lcls->gcr.cr[3] ~~~~~~~~~~~~~~~ CallRef at &lcls->gcr.cr[0]
But am I then maybe overwriting the LAC/CI ?
Exactly. I guess the limit of 5 octets is defined by the specs? If so, then it's better to sacrifice the most significant octet (usually 0x80 for CC) of the CallRef, rather than the least significant one. == Option a): | 0 | 1 | 2 | 3 | 4 | ^^^^^^^^^^^ CallRef at &lcls->gcr.cr[0] ^^^^^^^ LAC/CID at &lcls->gcr.cr[3] lcls->gcr.cr[2] = (trans->callref >> 0) & 0xff; lcls->gcr.cr[1] = (trans->callref >> 8) & 0xff; lcls->gcr.cr[0] = (trans->callref >> 16) & 0xff; osmo_store16be(/* LAC/CID */, &lcls->gcr.cr[3]); == Option b): | 0 | 1 | 2 | 3 | 4 | ^^^^^^^^^^^ CallRef at &lcls->gcr.cr[2] ^^^^^^^ LAC/CID at &lcls->gcr.cr[0] /* NOTE: LAC/CID overwrites MSB of CallRef at &lcls->gcr.cr[1] */ osmo_store32be(/* CallRef */, &lcls->gcr.cr[1]); osmo_store16be(/* LAC/CID */, &lcls->gcr.cr[0]); Personally I would prefer option b), because hexadecimal representation of the resulting lcls->gcr.cr[] would look similar to the CallRef in hex, so it would be easier to match them in logging. Best regards, Vadim. -- - Vadim Yanitskiy <vyanitskiy at sysmocom.de> http://www.sysmocom.de/ ======================================================================= * sysmocom - systems for mobile communications GmbH * Alt-Moabit 93 * 10559 Berlin, Germany * Sitz / Registered office: Berlin, HRB 134158 B * Geschaeftsfuehrer / Managing Director: Harald Welte
On 11/01/2023 22:26, Vadim Yanitskiy wrote: Hi Vadim, just to say, I'm preparing a patch for this... see below:
I guess the limit of 5 octets is defined by the specs? If so, then it's better to sacrifice the most significant octet (usually 0x80 for CC) of the CallRef, rather than the least significant one.
== Option a):
| 0 | 1 | 2 | 3 | 4 | ^^^^^^^^^^^ CallRef at &lcls->gcr.cr[0] ^^^^^^^ LAC/CID at &lcls->gcr.cr[3]
lcls->gcr.cr[2] = (trans->callref >> 0) & 0xff; lcls->gcr.cr[1] = (trans->callref >> 8) & 0xff; lcls->gcr.cr[0] = (trans->callref >> 16) & 0xff; osmo_store16be(/* LAC/CID */, &lcls->gcr.cr[3]);
== Option b):
| 0 | 1 | 2 | 3 | 4 | ^^^^^^^^^^^ CallRef at &lcls->gcr.cr[2] ^^^^^^^ LAC/CID at &lcls->gcr.cr[0]
/* NOTE: LAC/CID overwrites MSB of CallRef at &lcls->gcr.cr[1] */ osmo_store32be(/* CallRef */, &lcls->gcr.cr[1]); osmo_store16be(/* LAC/CID */, &lcls->gcr.cr[0]);
Personally I would prefer option b), because hexadecimal representation of the resulting lcls->gcr.cr[] would look similar to the CallRef in hex, so it would be easier to match them in logging.
The spec actually forces us to option A, unfortunately. I don't really know how much it matters for us, or for anything else, that we might break it. Anyway, see you in Code Review? Many thanks, Keith.
participants (2)
-
Keith -
Vadim Yanitskiy