laforge 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 <pma...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: laforge <lafo...@osmocom.org> Gerrit-Comment-Date: Tue, 04 Aug 2020 21:29:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
