This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
laforge gerrit-no-reply at lists.osmocom.orglaforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/19524 )
Change subject: mgcp_e1: finish E1 support, add E1 support from libosmoabis
......................................................................
Patch Set 1:
(29 comments)
sorry for the lengthy response :/
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/include/osmocom/mgcp/mgcp_trunk.h
File include/osmocom/mgcp/mgcp_trunk.h:
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/include/osmocom/mgcp/mgcp_trunk.h@58
PS1, Line 58: bool
I would have gone for a single uint32_t bit-mask, rather than 31 bool values (32*4=256 bytes). Not critical, we can als do that later.
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c
File src/libosmo-mgcp/mgcp_e1.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@57
PS1, Line 57: static ubit_t idle_tf_efr[] = { 0, 0, 0, 0, 0, 0, 0, 0,
const
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@100
PS1, Line 100: static ubit_t idle_tf_fr[] = { 0, 0, 0, 0, 0, 0, 0, 0,
const
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@143
PS1, Line 143: static ubit_t idle_tf_spch[] = { 0, 0, 0, 0, 0, 0, 0, 0,
const
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@239
PS1, Line 239: determine_trau_fr_type
I don't like that we call this function potentially for every frame (every 20ms) on every timeslot/call. And then it does all those string compares above. Without looking at it in detail, I would presume there is an easier way to do this, such as storing the TRAU frame type in the endpoint, and only updating it when the codec changes?
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@283
PS1, Line 283: LOGPENDP(endp, DE1, LOGL_ERROR, "E1-i460-RX: frame synchronization error, no bits received\n");
this potentially happens 50 times per second, for each call. Your log file will be filling the disk soon. I think we really need some rate limiting, or simply only have a counter here.
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@286
PS1, Line 286: %u bits (syncronized)
the synchroized bits are a TRAU Frame, so maybe call it that way?
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@292
PS1, Line 292: LOG
that's not an error.
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@296
PS1, Line 296: LOG
not an error
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@299
PS1, Line 299: case OSMO_I460_RATE_32k:
: LOGPENDP(endp, DE1, LOGL_ERROR, "E1-i460-RX: cannot decode 32k trau frame, rate not supported!\n");
: goto skip;
: break;
: case OSMO_I460_RATE_64k:
: LOGPENDP(endp, DE1, LOGL_ERROR, "E1-i460-RX: cannot decode 64k trau frame, rate not supported!\n");
: goto skip;
: break;
: default:
: LOGPENDP(endp, DE1, LOGL_ERROR, "E1-i460-RX: cannot decode trau frame, invalid rate set!\n");
: goto skip;
: break;
: }
I wouldn't go into all that detailed clauses for unrealistic code paths. IF it's not 16k or 8k, ASSERT. TRAU frames by definition only happen in 8k and 16k sub-slots. If the MGW previously adds a TRAU frame synchronizer to a 32k or 64k slot, the bug has happened much earlier. We shouldnt' end up here ,ever. So ASSERT seems appropriate.
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@313
PS1, Line 313: LOGPENDP(endp, DE1, LOGL_ERROR, "E1-i460-RX: unable to decode trau frame\n
also, more like a counter event to me.
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@328
PS1, Line 328: LOG
rate limit or counter?
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@344
PS1, Line 344: "E
what other 'type' could it be?
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@365
PS1, Line 365: LOGP(DE1, LOGL_ERROR, "E1-TX: (trunk:%u, ts:%u) no data to transmit!\n", trunk->trunk_nr, ts->num);
: goto skip;
: }
: if (rc != E1_TS_BYTES) {
: LOGP(DE1, LOGL_ERROR,
: "E1-TX: (trunk:%u, ts:%u) expected to get %u bytes of data, got %u bytes instead!\n",
: trunk->trunk_nr,
I think the I460 mux guarantees you always get data (in the worst case 0xff stuffing), so we can remove all those if clauses and replace them with an ASSERT.
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@399
PS1, Line 399: LOGP(DE1, LOGL_ERROR, "E1-RX: (trunk:%u) E1 timeslot number (%u) out of range!\n", trunk->trunk_nr,
: ts->num);
: return;
ASSERT. An E1 line can neve have more timeslots, and e1_input should guarantee this.
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@403
PS1, Line 403: msg->len != E1_TS_BYTES) {
: LOGP(DE1, LOGL_ERROR,
: "E1-RX: (trunk:%u, ts:%u) receiving bad, expected length is %u, actual length is %u!\n",
: trunk->trunk_nr, ts->num, E1_TS_BYTES, msg->len);
not sure if that's really an error. Maybe only notice? In any case, the I460 demux doesn't care in what chunk size you feed data in. Likewise, the I460 mux can generate arbitrary chunk sizes in the ouptut direction.
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@449
PS1, Line 449: LOGP(DE1, LOGL_DEBUG, "(trunk:%u)
unrelated / separate patch: Shouldn't we have something like LOGPTRUNK(trunk, ...)?
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@514
PS1, Line 514: i640
"I.460" is what it's called in the ITU specs. In C symbol names we cannot use the dot, but in log messages we can and should. And 640->460 in any case.
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@518
PS1, Line 518: i640
same
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@523
PS1, Line 523: i640
same
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@568
PS1, Line 568: LOG
this check is about the header length, so it's about short RTP header, not payloda. Also, I would consider a counter instead of a log message. Assume sombody accidentially or intentionally sends lots of single byte UDP packets... those kind of log messages turn that into a DoS attach as your logging subsystem is eating all of the CPU.
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@572
PS1, Line 572: msg->len
msgb_length(msg) is preferred style than directly accessing the members. You also use msgb_data(msg) in the same line, and not msg->data.
Even better: set msgb->l2h to the RTP payload after the header, then you can use msgb_l2(msg) and msgb_l2len(msg) and don't need the "+rtp header len" here and in the lines below.
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@592
PS1, Line 592: LOGPENDP(endp, DE1, LOGL_ERROR, "E1-i460-TX: subchannel multiplexer missing\n");
can this happen? In which situation? Or would it be a programming error? In he latter case, simply assert.
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@596
PS1, Line 596: LOGPENDP(endp, DE1, LOGL_ERROR, "E1-i460-TX: subchannel sync missing\n");
same here.
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_endp.c
File src/libosmo-mgcp/mgcp_endp.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_endp.c@132
PS1, Line 132: if
no {} needed for single line body.
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_endp.c@617
PS1, Line 617: endp->callid = talloc_strdup(endp, callid);
OSMO_ASSERT() that the result exists, or handle NULL checks + reasonable recovery
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_endp.c@630
PS1, Line 630: OSMO_ASSERT(ts != 0xFF);
: OSMO_ASSERT(ts != 0);
: OSMO_ASSERT(ss != 0xFF);
: OSMO_ASSERT(offs != 0xFF);
can the endp->name be specified by the user / caller (BSC/MSC) here? IF yes, then we shouldn't assert.
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_network.c
File src/libosmo-mgcp/mgcp_network.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_network.c@800
PS1, Line 800: * Generate an RTP header if it is missing */
the function seems to unconditionally generate a header, not just if it's missing?
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_trunk.c
File src/libosmo-mgcp/mgcp_trunk.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_trunk.c@151
PS1, Line 151: 31
don't we have some #define for that? also, why < 31? shouldn't it be < 32?
Or do we map TS1 to array index [0]?
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/19524
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I6b93809b5ac7d01af55888347dd787b0bc997ae1
Gerrit-Change-Number: 19524
Gerrit-PatchSet: 1
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge at osmocom.org>
Gerrit-Comment-Date: Tue, 04 Aug 2020 21:29:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200804/6be90a87/attachment.htm>