<p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/24513">View Change</a></p><p>18 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/5/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/5/src/common/abis.c@65">Patch Set #5, Line 65:</a> <code style="font-family:monospace,monospace">W</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">from these names it's not entirely clear to me what the states represent. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/5/src/common/abis.c@66">Patch Set #5, Line 66:</a> <code style="font-family:monospace,monospace"> ABIS_LINK_ST_CONN,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">CONN means CONNECTED here? CONNECTING? Most probably needs to be changed to some of those, it's not […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/5/src/common/abis.c@67">Patch Set #5, Line 67:</a> <code style="font-family:monospace,monospace"> ABIS_LINK_ST_FAIL,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">state is FAILED right? FAIL it's more like an event or action.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/5/src/common/abis.c@87">Patch Set #5, Line 87:</a> <code style="font-family:monospace,monospace">static void abis_link_conn_action(struct osmo_fsm_inst *fi, uint32_t event, void *data)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">we usually don't use "_action" suffix, only the name. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/5/src/common/abis.c@144">Patch Set #5, Line 144:</a> <code style="font-family:monospace,monospace"> /* In some implementations the user may specify a BSC host via commandline switch. If this is the case</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(doesn't look clear to me to take cmdline related specialties here, but OK)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is related to osmo-bts-omldummy, this implementation has a commandline switch to specify the BSC host. Thats the reason why this parameter exists. I wonder if we could get rid of this if we just use the commandline parameter to create a list entry in the bsc_oml_hosts list. What do you think?</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/5/src/common/abis.c@148">Patch Set #5, Line 148:</a> <code style="font-family:monospace,monospace"> } else if ((struct llist_head *)priv->bsc_oml_host != (struct llist_head *)&bts->bsc_oml_hosts.next) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">llist_first(&bts->bsc_oml_hosts)? Or do you actually mean the end of the list? isn't that "previous" […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I detect want to detect the end of the list. Its indeed .prev. I have reorganized this a bit and made some functions for libosmocore to handle that better.</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/5/src/common/abis.c@149">Patch Set #5, Line 149:</a> <code style="font-family:monospace,monospace"> /* Get a BSC host from the list and move the list heade one position forward. */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">type: header</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/5/src/common/abis.c@153">Patch Set #5, Line 153:</a> <code style="font-family:monospace,monospace"> } else {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">maybe moving this early termiantion above (if condition) would help making the function a bit less c […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think I can not do that. I need to check priv->dst_host and the end of the list first before I can know that the code should return/exit</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/5/src/common/abis.c@163">Patch Set #5, Line 163:</a> <code style="font-family:monospace,monospace"> osmo_get_macaddr(bts_dev_info.mac_addr, "eth0");</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">what is this? hardcoded eth0? this looks wrong</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is probably only to get a unique identifier. It was already in the BTS code. Yes it looks wrong, especially since the name "eth0" is not used so much anymore. However this is a different topic, apparently this line never caused problems or nobody really cares about whats in bts_dev_info.mac_addr?</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/5/src/common/abis.c@172">Patch Set #5, Line 172:</a> <code style="font-family:monospace,monospace"> if (line) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">drop {}</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The e1inp_line_get2 macro behaves strange when I leave the braces away. The original code also has the braces. I do not understand why it fails. The macro is defined as:</p><p style="white-space: pre-wrap; word-wrap: break-word;">#define e1inp_line_get2(line, USE) OSMO_ASSERT( osmo_use_count_get_put(&(line)->use_count, USE, 1) == 0 );</p><p style="white-space: pre-wrap; word-wrap: break-word;">I also tried to leave the ; away, but it still does not compile.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The error is:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">abis.c: In function ‘abis_link_connecting’:<br>abis.c:174:2: error: ‘else’ without a previous ‘if’<br> else</pre></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/5/src/common/abis.c@176">Patch Set #5, Line 176:</a> <code style="font-family:monospace,monospace"> priv->line_ctr++;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">not sure what this is about</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">As I understand the line model in libosmo-abis each address (BSC) needs its own line. It is not possible to re-use a line with a different ip-address. It is also not possible to get rid of a line once it is created.</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/5/src/common/abis.c@206">Patch Set #5, Line 206:</a> <code style="font-family:monospace,monospace"> .name = "CONN",</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">CONNECTING</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/5/src/common/abis.c@206">Patch Set #5, Line 206:</a> <code style="font-family:monospace,monospace"> .</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">some strange space-based indenting here, at least it looks like that in gerrit</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/5/src/common/abis.c@207">Patch Set #5, Line 207:</a> <code style="font-family:monospace,monospace"> .in_event_mask = 0 | S(ABIS_LINK_EV_SIGN_LINK_DOWN)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">please drop the "0 |". […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/5/src/common/abis.c@207">Patch Set #5, Line 207:</a> <code style="font-family:monospace,monospace"> .in_event_mask = 0 | S(ABIS_LINK_EV_SIGN_LINK_DOWN)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">this 0| can be dropped</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/5/src/common/abis.c@216">Patch Set #5, Line 216:</a> <code style="font-family:monospace,monospace"> .in_event_mask = 0,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">how does this work? A FSM state "DOWN" which does not permit any input events, but which has multip […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">It does not need any events. It has an onenter function that initiates the connection. If it succeeds it changes in the CONN(ECTED) state or it it may run into an unrecoverable problem, then it changes to the FAIL(ED) state.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/main.c">File src/common/main.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/5/src/common/main.c@399">Patch Set #5, Line 399:</a> <code style="font-family:monospace,monospace"> if (llist_count(&g_bts->bsc_oml_hosts) == 0) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">what about the cmdline oml ip I saw in the fsm which was outisde the llist?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This option only exists for osmo-bts-omldummy (see other comment)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/vty.c">File src/common/vty.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/5/src/common/vty.c@536">Patch Set #5, Line 536:</a> <code style="font-family:monospace,monospace">{</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">iirc you are maintaining a pointer to the currently "connecting" remote ip. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is now checked. Thanks.</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: 6 </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: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-CC: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 29 Jun 2021 14:18:36 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Comment-In-Reply-To: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>