<p style="white-space: pre-wrap; word-wrap: break-word;">sorry for the lengthy response :/</p><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524">View Change</a></p><p>29 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/include/osmocom/mgcp/mgcp_trunk.h">File include/osmocom/mgcp/mgcp_trunk.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/include/osmocom/mgcp/mgcp_trunk.h@58">Patch Set #1, Line 58:</a> <code style="font-family:monospace,monospace"> bool</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c">File src/libosmo-mgcp/mgcp_e1.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@57">Patch Set #1, Line 57:</a> <code style="font-family:monospace,monospace">static ubit_t idle_tf_efr[] = { 0, 0, 0, 0, 0, 0, 0, 0,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">const</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@100">Patch Set #1, Line 100:</a> <code style="font-family:monospace,monospace">static ubit_t idle_tf_fr[] = { 0, 0, 0, 0, 0, 0, 0, 0,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">const</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@143">Patch Set #1, Line 143:</a> <code style="font-family:monospace,monospace">static ubit_t idle_tf_spch[] = { 0, 0, 0, 0, 0, 0, 0, 0,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">const</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@239">Patch Set #1, Line 239:</a> <code style="font-family:monospace,monospace">determine_trau_fr_type</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@283">Patch Set #1, Line 283:</a> <code style="font-family:monospace,monospace">LOGPENDP(endp, DE1, LOGL_ERROR, "E1-i460-RX: frame synchronization error, no bits received\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@286">Patch Set #1, Line 286:</a> <code style="font-family:monospace,monospace">%u bits (syncronized)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">the synchroized bits are a TRAU Frame, so maybe call it that way?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@292">Patch Set #1, Line 292:</a> <code style="font-family:monospace,monospace"> LOG</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">that's not an error.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@296">Patch Set #1, Line 296:</a> <code style="font-family:monospace,monospace"> LOG</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">not an error</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@299">Patch Set #1, Line 299:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">case OSMO_I460_RATE_32k:<br> LOGPENDP(endp, DE1, LOGL_ERROR, "E1-i460-RX: cannot decode 32k trau frame, rate not supported!\n");<br> goto skip;<br> break;<br> case OSMO_I460_RATE_64k:<br> LOGPENDP(endp, DE1, LOGL_ERROR, "E1-i460-RX: cannot decode 64k trau frame, rate not supported!\n");<br> goto skip;<br> break;<br> default:<br> LOGPENDP(endp, DE1, LOGL_ERROR, "E1-i460-RX: cannot decode trau frame, invalid rate set!\n");<br> goto skip;<br> break;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@313">Patch Set #1, Line 313:</a> <code style="font-family:monospace,monospace">LOGPENDP(endp, DE1, LOGL_ERROR, "E1-i460-RX: unable to decode trau frame\n</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">also, more like a counter event to me.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@328">Patch Set #1, Line 328:</a> <code style="font-family:monospace,monospace"> LOG</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">rate limit or counter?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@344">Patch Set #1, Line 344:</a> <code style="font-family:monospace,monospace"> "E</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">what other 'type' could it be?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@365">Patch Set #1, Line 365:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">LOGP(DE1, LOGL_ERROR, "E1-TX: (trunk:%u, ts:%u) no data to transmit!\n", trunk->trunk_nr, ts->num);<br> goto skip;<br> }<br> if (rc != E1_TS_BYTES) {<br> LOGP(DE1, LOGL_ERROR,<br> "E1-TX: (trunk:%u, ts:%u) expected to get %u bytes of data, got %u bytes instead!\n",<br> trunk->trunk_nr, <br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@399">Patch Set #1, Line 399:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">LOGP(DE1, LOGL_ERROR, "E1-RX: (trunk:%u) E1 timeslot number (%u) out of range!\n", trunk->trunk_nr,<br> ts->num);<br> return;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">ASSERT. An E1 line can neve have more timeslots, and e1_input should guarantee this.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@403">Patch Set #1, Line 403:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">msg->len != E1_TS_BYTES) {<br> LOGP(DE1, LOGL_ERROR,<br> "E1-RX: (trunk:%u, ts:%u) receiving bad, expected length is %u, actual length is %u!\n",<br> trunk->trunk_nr, ts->num, E1_TS_BYTES, msg->len);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@449">Patch Set #1, Line 449:</a> <code style="font-family:monospace,monospace">LOGP(DE1, LOGL_DEBUG, "(trunk:%u) </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">unrelated / separate patch: Shouldn't we have something like LOGPTRUNK(trunk, ...)?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@514">Patch Set #1, Line 514:</a> <code style="font-family:monospace,monospace">i640</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@518">Patch Set #1, Line 518:</a> <code style="font-family:monospace,monospace">i640</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">same</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@523">Patch Set #1, Line 523:</a> <code style="font-family:monospace,monospace">i640</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">same</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@568">Patch Set #1, Line 568:</a> <code style="font-family:monospace,monospace"> LOG</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@572">Patch Set #1, Line 572:</a> <code style="font-family:monospace,monospace">msg->len</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@592">Patch Set #1, Line 592:</a> <code style="font-family:monospace,monospace">LOGPENDP(endp, DE1, LOGL_ERROR, "E1-i460-TX: subchannel multiplexer missing\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">can this happen? In which situation? Or would it be a programming error? In he latter case, simply assert.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_e1.c@596">Patch Set #1, Line 596:</a> <code style="font-family:monospace,monospace">LOGPENDP(endp, DE1, LOGL_ERROR, "E1-i460-TX: subchannel sync missing\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">same here.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_endp.c">File src/libosmo-mgcp/mgcp_endp.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_endp.c@132">Patch Set #1, Line 132:</a> <code style="font-family:monospace,monospace"> if</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">no {} needed for single line body.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_endp.c@617">Patch Set #1, Line 617:</a> <code style="font-family:monospace,monospace"> endp->callid = talloc_strdup(endp, callid);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">OSMO_ASSERT() that the result exists, or handle NULL checks + reasonable recovery</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_endp.c@630">Patch Set #1, Line 630:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">OSMO_ASSERT(ts != 0xFF);<br> OSMO_ASSERT(ts != 0);<br> OSMO_ASSERT(ss != 0xFF);<br> OSMO_ASSERT(offs != 0xFF);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">can the endp->name be specified by the user / caller (BSC/MSC) here? IF yes, then we shouldn't assert.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_network.c">File src/libosmo-mgcp/mgcp_network.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_network.c@800">Patch Set #1, Line 800:</a> <code style="font-family:monospace,monospace">* Generate an RTP header if it is missing */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">the function seems to unconditionally generate a header, not just if it's missing?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_trunk.c">File src/libosmo-mgcp/mgcp_trunk.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524/1/src/libosmo-mgcp/mgcp_trunk.c@151">Patch Set #1, Line 151:</a> <code style="font-family:monospace,monospace">31</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">don't we have some #define for that? also, why < 31? shouldn't it be < 32?<br>Or do we map TS1 to array index [0]?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524">change 19524</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-mgw/+/19524"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-mgw </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I6b93809b5ac7d01af55888347dd787b0bc997ae1 </div>
<div style="display:none"> Gerrit-Change-Number: 19524 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-CC: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 04 Aug 2020 21:29:21 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>