<p style="white-space: pre-wrap; word-wrap: break-word;">I would say let's merge it soon unless somebody has major concerns.  We then can start porting/writing applications to it and still modifiy the API as needed until we tag the next release.  But then, we will need to tag releases at the end of the month - maybe we should create a 'ns2' branch of libosmocore.git and push this change there until everything has been ported and we can merge ns2 to master?</p><p>Patch set 23:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #d4ffd4;">Code-Review +1</span></p><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/19417">View Change</a></p><p>6 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/19417/23/include/osmocom/gprs/gprs_ns2.h">File include/osmocom/gprs/gprs_ns2.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/libosmocore/+/19417/23/include/osmocom/gprs/gprs_ns2.h@86">Patch Set #23, Line 86:</a> <code style="font-family:monospace,monospace">link_selector_parameter</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think we should have pointers in primitives.  Right now the idea of primitives is that you could memcpy them, without having to worry about reference counts to other objects.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/19417/23/src/gb/Makefile.am">File src/gb/Makefile.am:</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/libosmocore/+/19417/23/src/gb/Makefile.am@25">Patch Set #23, Line 25:</a> <code style="font-family:monospace,monospace">           gprs_ns2_message.c gprs_ns2_vty.c \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm not sure if it wouldn't be worth to move this code to a new library?  But then the problem is, the BSSGP code is still in libosmogb and hence we'd need both anyway. So keep as-is.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/19417/23/src/gb/gprs_ns2.c">File src/gb/gprs_ns2.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/libosmocore/+/19417/23/src/gb/gprs_ns2.c@187">Patch Set #23, Line 187:</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;">oid gprs_ns2_set_log_ss(int ss)<br>{<br>   DNS = ss;<br>}<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">we could just add a global "DLNS" to libosmocore, but we can also keep this style.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/19417/23/src/gb/gprs_ns2.c@876">Patch Set #23, Line 876:</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;">/* TODO: remove this global, why the hell do I use it */<br>static bool gprs_fsm_vc_registered = false;<br>static bool gprs_fsm_sns_registered = false;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">indeed ;)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/19417/23/src/gb/gprs_ns2_message.c">File src/gb/gprs_ns2_message.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/libosmocore/+/19417/23/src/gb/gprs_ns2_message.c@64">Patch Set #23, Line 64:</a> <code style="font-family:monospace,monospace">int gprs_ns2_validate_reset(struct gprs_ns2_vc *nsvc, struct msgb *msg, struct tlv_parsed *tp, uint8_t *cause)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">we have a more data driven approach for this, see https://git.osmocom.org/libosmo-sccp/tree/src/m3ua.c#n129 which is then executed by https://git.osmocom.org/libosmo-sccp/tree/src/xua_msg.c#n449</p><p style="white-space: pre-wrap; word-wrap: break-word;">That is tied particularly to the xUA series of protocols, but I long wanted to expand this to our normal tlvp style protocol parsers.  So something similar in spirit that is part of libosmocore and whihc can be used by arious protocol modules all around.</p><p style="white-space: pre-wrap; word-wrap: break-word;">In any case, the code here can and should stay as-is.  We can switch to a different data-driven verification codebase later on.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/19417/23/src/gb/gprs_ns2_message.c@74">Patch Set #23, Line 74:</a> <code style="font-family:monospace,monospace">int gprs_ns2_validate_reset_ack(struct gprs_ns2_vc *nsvc, struct msgb *msg, struct tlv_parsed *tp, uint8_t *cause)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">shouldn't all these be 'static' and called by the dispatcher function below?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmocore/+/19417">change 19417</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/libosmocore/+/19417"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I3525beef205588dfab9d3880a34115f1a2676e48 </div>
<div style="display:none"> Gerrit-Change-Number: 19417 </div>
<div style="display:none"> Gerrit-PatchSet: 23 </div>
<div style="display:none"> Gerrit-Owner: lynxis lazus <lynxis@fe80.eu> </div>
<div style="display:none"> Gerrit-Assignee: daniel <daniel@totalueberwachung.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: lynxis lazus <lynxis@fe80.eu> </div>
<div style="display:none"> Gerrit-CC: daniel <daniel@totalueberwachung.de> </div>
<div style="display:none"> Gerrit-CC: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 08 Sep 2020 16:25:29 +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>