<p><a href="https://gerrit.osmocom.org/c/osmo-smlc/+/20470">View Change</a></p><p>13 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/include/osmocom/smlc/sccp_lb_inst.h">File include/osmocom/smlc/sccp_lb_inst.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-smlc/+/20470/6/include/osmocom/smlc/sccp_lb_inst.h@52">Patch Set #6, Line 52:</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;">Compatibility with legacy osmo-hnbgw that was unable to properly handle RESET messages.  Set to 'false' to<br>        * require proper RESET procedures, set to 'true' to implicitly put a lb_peer in RAN_PEER_ST_READY upon the<br>    * first CO message. Default is false = be strict. */<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">we are not talking to osmo-hnbgw. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">copy paste artifact, this should be dropped here indeed</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/cell_locations.c">File src/osmo-smlc/cell_locations.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-smlc/+/20470/6/src/osmo-smlc/cell_locations.c@38">Patch Set #6, Line 38:</a> <code style="font-family:monospace,monospace">uint32_t</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Do we really need to enforce 'uint32_t' here? The maximum would be 63 * 550 == 34650, so 'uint16_t'  […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">there is also Access Delay being the TA expanded to 8 bit range, 255 * 550 fits in 18 bits</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/cell_locations.c@194">Patch Set #6, Line 194:</a> <code style="font-family:monospace,monospace">DEFUN</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Would be nice to attach 'when-it-applies' attributes to all configuration commands.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">i have not looked at how that works, could you help me with that, in a subsequent patch?<br>The cell locations config takes effect immediately.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/cell_locations.c@248">Patch Set #6, Line 248:</a> <code style="font-family:monospace,monospace">unidirectional</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">You probably mean omnidirectional here.  "unidirectional" would be the opposite.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">omni!</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/lb_conn.c">File src/osmo-smlc/lb_conn.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-smlc/+/20470/6/src/osmo-smlc/lb_conn.c@49">Patch Set #6, Line 49:</a> <code style="font-family:monospace,monospace">        if (total == 0</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">No need for a line break.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">i find that it better illustrates the two cases for 'total'</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/lb_conn.c@78">Patch Set #6, Line 78:</a> <code style="font-family:monospace,monospace">use_count</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Hmm, so this doesn't compile?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">not sure what i was thinking, totally fine</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/lb_peer.c">File src/osmo-smlc/lb_peer.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-smlc/+/20470/6/src/osmo-smlc/lb_peer.c@172">Patch Set #6, Line 172:</a> <code style="font-family:monospace,monospace">struct msgb *msg = ctx->msg</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Who is responsible for free()ing this msgb? The sender of a event?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">thanks for reminding me, i was leaking msgb in two places. this time i verified it by running ttcn tests and looking at the msgb count</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/lb_peer.c@463">Patch Set #6, Line 463:</a> <code style="font-family:monospace,monospace">                                 " Dropping message.\n");</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">is there some kind of STATUS we should return to the peer?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">i have no idea, really, could you tell me what should happen, or should i try to find out?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/smlc_data.c">File src/osmo-smlc/smlc_data.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-smlc/+/20470/6/src/osmo-smlc/smlc_data.c@27">Patch Set #6, Line 27:</a> <code style="font-family:monospace,monospace">smlc_ctr_description</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I find it hard to read, let's use line wraps please.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">you marked "smlc_ctr_description", but you mean the strings below, right?<br>(the single line nature comes from using a '%s/...' rule)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/smlc_loc_req.c">File src/osmo-smlc/smlc_loc_req.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-smlc/+/20470/6/src/osmo-smlc/smlc_loc_req.c@152">Patch Set #6, Line 152:</a> <code style="font-family:monospace,monospace">smlc_loc_req = (struct smlc_loc_req){</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This could be twice shorter: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">because i don't know whether the future adds more elements to the struct, and this zero initializes all members that i don't set. Let's not micro optimise and rather opt for maintainability and non-randomness.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/smlc_main.c">File src/osmo-smlc/smlc_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-smlc/+/20470/6/src/osmo-smlc/smlc_main.c@179">Patch Set #6, Line 179:</a> <code style="font-family:monospace,monospace">loglevel = LOGL_NOTICE</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">AFAIR, it's safe to omit this filed: libosmocore would apply LOGL_NOTICE automatically.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">i just copy pasted it from somewhere</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/tests/cell_locations.vty">File tests/cell_locations.vty:</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-smlc/+/20470/6/tests/cell_locations.vty@83">Patch Set #6, Line 83:</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;">logging level llapd notice<br> logging level linp notice<br> logging level lmux notice<br> logging level lmi notice<br> logging level lmib notice<br> logging level lsms notice<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This shall not be a part of the transcript test: adding new logging categories to libosmocore would  […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">whoa this is an accident, thanks</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/tests/osmo-smlc.cfg">File tests/osmo-smlc.cfg:</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-smlc/+/20470/6/tests/osmo-smlc.cfg">Patch Set #6:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Empty config? Why not just do: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">because it's annoying to care about build-time files, and maybe some day we want some actual config in here</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-smlc/+/20470">change 20470</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-smlc/+/20470"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-smlc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I917ba8fc51a1f1150be77ae01e12a7b16a853052 </div>
<div style="display:none"> Gerrit-Change-Number: 20470 </div>
<div style="display:none"> Gerrit-PatchSet: 7 </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-CC: Vadim Yanitskiy <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 13 Oct 2020 02:45:01 +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: Vadim Yanitskiy <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>