<p><a href="https://gerrit.osmocom.org/c/osmo-smlc/+/20470">View Change</a></p><p>22 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/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 style="white-space: pre-wrap; word-wrap: break-word;">Do we really need to enforce 'uint32_t' here? The maximum would be 63 * 550 == 34650, so 'uint16_t' or even generic 'unsigned int' / 'unsigned short' is sufficient.</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@55">Patch Set #6, Line 55:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ws</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 style="white-space: pre-wrap; word-wrap: break-word;">Would be nice to attach 'when-it-applies' attributes to all configuration commands.</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 style="white-space: pre-wrap; word-wrap: break-word;">No need for a line break.</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 style="white-space: pre-wrap; word-wrap: break-word;">Hmm, so this doesn't compile?</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@35">Patch Set #6, Line 35:</a> <code style="font-family:monospace,monospace">lb_peer_init</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I would expect to see this function defined in the end of this file, just like you do in smlc_loc_req.c.</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@50">Patch Set #6, Line 50:</a> <code style="font-family:monospace,monospace">talloc_zero</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Makes sense to use regular talloc() here, because you initialize the whole structure 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-smlc/+/20470/6/src/osmo-smlc/lb_peer.c@161">Patch Set #6, Line 161:</a> <code style="font-family:monospace,monospace">sccp_lb_down_l2_cl</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Looks like we have a memleak here? If so, it makes sense to start using OTC_SELECT.</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@172">Patch Set #6, Line 172:</a> <code style="font-family:monospace,monospace">struct msgb *msg = ctx->msg</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Who is responsible for free()ing this msgb? The sender of a 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-smlc/+/20470/6/src/osmo-smlc/lb_peer.c@204">Patch Set #6, Line 204:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ws</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@247">Patch Set #6, Line 247:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ws</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@283">Patch Set #6, Line 283:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ws</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 style="white-space: pre-wrap; word-wrap: break-word;">I find it hard to read, let's use line wraps please.</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@68">Patch Set #6, Line 68:</a> <code style="font-family:monospace,monospace">smlc_loc_req_fail</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why not to use a function here? I guess compiler would most likely inline it anyway.</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/smlc_loc_req.c@86">Patch Set #6, Line 86:</a> <code style="font-family:monospace,monospace">OSMO_ASSERT</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Looks like we need talloc_or_die() / talloc_zero_or_die() API ;)</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/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 style="white-space: pre-wrap; word-wrap: break-word;">This could be twice shorter:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  smlc_loc_req->lb_conn = lb_conn;<br>  smlc_loc_req->req = *loc_req_pdu;</pre><p style="white-space: pre-wrap; word-wrap: break-word;">I don't see a reason to overwrite the whole structure twice...</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/smlc_loc_req.c@175">Patch Set #6, Line 175:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ws</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/smlc_loc_req.c@212">Patch Set #6, Line 212:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ws</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/smlc_loc_req.c@294">Patch Set #6, Line 294:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ws</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 style="white-space: pre-wrap; word-wrap: break-word;">AFAIR, it's safe to omit this filed: libosmocore would apply LOGL_NOTICE automatically.</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 style="white-space: pre-wrap; word-wrap: break-word;">This shall not be a part of the transcript test: adding new logging categories to libosmocore would make it fail. The whole block till line #106 should be removed.</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 style="white-space: pre-wrap; word-wrap: break-word;">Empty config? Why not just do:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  touch $(top_builddir)/tests/osmo-smlc.cfg</pre><p style="white-space: pre-wrap; word-wrap: break-word;">in the test suite?</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: 6 </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: Sat, 10 Oct 2020 21:25:33 +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>