<p><a href="https://gerrit.osmocom.org/7732">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/7732/1/src/libbsc/acc_ramp.c">File src/libbsc/acc_ramp.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/7732/1/src/libbsc/acc_ramp.c@162">Patch Set #1, Line 162:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">You are deferring an expected nm_statechg_signal_data pointer before knowing if that's the correct s […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Nice catch! Fixed in next patch set.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/7732/1/src/libbsc/acc_ramp.c@166">Patch Set #1, Line 166:</a> <code style="font-family:monospace,monospace">          acc_ramp_trigger(acc_ramp);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Is this really needed? we are only attaching the cb to the SS_NM subsystem right? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Unsure. This was copied from bts_ipa_nm_sig_cb() in bts_ipaccess_nanobts.c. I didn't verify whether it's really required, and I'm happy to drop it (it certainly looks weird).</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/7732/1/src/libbsc/acc_ramp.c@194">Patch Set #1, Line 194:</a> <code style="font-family:monospace,monospace">  acc_ramp->bts = bts;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">What about NM_STATE_SHUTDOWN and NM_STATE_NULL? Would be nice at least adding an explicit case for N […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'll make it abort ramping on SHUTDOWN as well.<br>Not sure what to do about NULL -- for now I'd treat it as a no-op.</p><p style="white-space: pre-wrap; word-wrap: break-word;">An ASSERT on default would be a very bad idea because the administrative state value isn't checked by the lower layers but simply read verbatim from the TLV provided in the received packet. So an assert would introduce an easy way to kill the process remotely. I would rather log a warning instead (see next patch set).</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/7732/1/src/libbsc/bsc_vty.c">File src/libbsc/bsc_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/7732/1/src/libbsc/bsc_vty.c@3280">Patch Set #1, Line 3280:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">You can take the chance to improve the "BTS reconnects" part by adding reference to RSL link.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes this comment is a bit sparse. Improved in next patch set.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/7732">change 7732</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/7732"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-bsc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I4124f1da3dadec003de45c1da8435506ee8f0a34 </div>
<div style="display:none"> Gerrit-Change-Number: 7732 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: Stefan Sperling <stsp@stsp.name> </div>
<div style="display:none"> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Stefan Sperling <stsp@stsp.name> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 11 Dec 2018 12:57:04 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>