<p>Patch set 2:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -2</span></p><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/17377">View Change</a></p><p>2 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/17377/2/src/osmo-bts-virtual/l1_if.c">File src/osmo-bts-virtual/l1_if.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-bts/+/17377/2/src/osmo-bts-virtual/l1_if.c@157">Patch Set #2, Line 157:</a> <code style="font-family:monospace,monospace">;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">actually, the entire modification in this file/function is bogus: We are allocating a new tch message (different from treatment of all other gsmtap channel types here).  And then we add a L1SAP header and do nothing with that new msg_tch.  First of all, that means there's a memory leak.  And secondly, this means that the code above L1SAP can never actually see any of the GSMTAP_CHANNEL_VOICE messages we may have received in uplink</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/17377/2/src/osmo-bts-virtual/scheduler_virtbts.c">File src/osmo-bts-virtual/scheduler_virtbts.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-bts/+/17377/2/src/osmo-bts-virtual/scheduler_virtbts.c@427">Patch Set #2, Line 427:</a> <code style="font-family:monospace,monospace">      if (msg_facch)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">this modification breaks the entire FACCH substitution logic.</p><p style="white-space: pre-wrap; word-wrap: break-word;">There may be TCH and FACCH message present at the same time. The FACCH has higher priority and the TCH message is dropped.  The old code did exactly that.  The new code now sends both up, whcih is illegal.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-bts/+/17377">change 17377</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-bts/+/17377"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-bts </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I1cd9a251ce0b87181a0822d7940bbfc9f1428543 </div>
<div style="display:none"> Gerrit-Change-Number: 17377 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-CC: fixeria <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Sat, 07 Mar 2020 16:30:28 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>