<p>Patch set 8:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/24513">View Change</a></p><p>4 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/+/24513/8/src/common/abis.c">File src/common/abis.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/+/24513/8/src/common/abis.c@114">Patch Set #8, Line 114:</a> <code style="font-family:monospace,monospace">      if (g_bts->osmo_link) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Are you sure you can ever get a ABIS_LINK_EV_SIGN_LINK_DOWN event where you have no oml_link nor rsl_link? I doubt so. Hence, oml_rsl_was_connected will always be true here, and the statechg to CONNECTING never happens.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bts/+/24513/8/src/common/abis.c@150">Patch Set #8, Line 150:</a> <code style="font-family:monospace,monospace">   if (!llist_contains(&bts->bsc_oml_hosts, priv->bsc_oml_host))</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think I already explained in previous version that imho this is not the proper way of doing this. You should not keep and use pointer addresses to freed memory. You are only looking for a disaster here. What if a new entry has been allocated in the same memory chunk and added to the tail of the list? This condition would be true, so you'll go on and take next in list, which would be none, and be done. And in the process you could have jumped/discarded several addresses in the middle.<br>Rather than that, better send an event to the FSM from the VTY code before freeing the entry, and then drop the pointer (set it to NULL) in the FSM when receiving the event.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bts/+/24513/8/src/common/abis.c@170">Patch Set #8, Line 170:</a> <code style="font-family:monospace,monospace">    osmo_get_macaddr(bts_dev_info.mac_addr, "eth0");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I see  the eth0 comes from old version abis_open() below. This is probably hardcoded to sysmobts but it's not going to work  fine on other systems? Anyway not related to this patch.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bts/+/24513/8/src/common/abis.c@223">Patch Set #8, Line 223:</a> <code style="font-family:monospace,monospace">             .out_state_mask = S(ABIS_LINK_ST_CONNECTING) | S(ABIS_LINK_ST_CONNECTED) | S(ABIS_LINK_ST_FAILED),</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">In general I find much more readable to have one state/event per line. It also makes new patches easier to review/apply/revert.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-bts/+/24513">change 24513</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/+/24513"/><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: I205f68a3a7f35fee4c38a7cfba2b014237df2727 </div>
<div style="display:none"> Gerrit-Change-Number: 24513 </div>
<div style="display:none"> Gerrit-PatchSet: 8 </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-Reviewer: daniel <dwillmann@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 07 Jul 2021 09:00:35 +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>