<p style="white-space: pre-wrap; word-wrap: break-word;">Thanks for reviewing!</p><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/21862">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/libosmocore/+/21862/6/include/osmocom/gprs/gprs_bssgp_rim.h">File include/osmocom/gprs/gprs_bssgp_rim.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/+/21862/6/include/osmocom/gprs/gprs_bssgp_rim.h@2">Patch Set #6, Line 2:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">license/copyright statement?</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/libosmocore/+/21862/6/include/osmocom/gprs/gprs_bssgp_rim.h@7">Patch Set #6, Line 7:</a> <code style="font-family:monospace,monospace">   struct {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">You could reuse "struct osmo_cell_global_id_ps" from https://gerrit.osmocom. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Makes sense, but I could not find a parsing function for the struct. I re-used the existing ones now.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21862/6/include/osmocom/gprs/gprs_bssgp_rim.h@73">Patch Set #6, Line 73:</a> <code style="font-family:monospace,monospace">Si3</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I don't recall the details: But won't we need SI3 as part of the NACC feature? We have been waiting  […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The SI3 application container is not used by the NACC application, since the NACC application will only use NACC application containers. To me the SI3 application seems a bit redundant. The SI3 can also be requested within an NACC application container. Maybe it is for some other purpose when only SI3 is required. (8c.6.2 SI3 application)</p><p style="white-space: pre-wrap; word-wrap: break-word;">If we add new application containers at some later point we will make the ABI incompatible, but the API will still work. From what I can see SI3 is not needed for NACC and we do not need it now. I have placed the TODOs as a hint where new application containers would go.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c">File src/gb/gprs_bssgp_rim.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/+/21862/6/src/gb/gprs_bssgp_rim.c@4">Patch Set #6, Line 4:</a> <code style="font-family:monospace,monospace"> * (C) 2020 by sysmocom - s.f.m.c. GmbH</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">-2021 now I guess?</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/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@144">Patch Set #6, Line 144:</a> <code style="font-family:monospace,monospace">buf++;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">so now buf point s to index '1' but the length check above passed with len == 1, i.e. […]</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/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@145">Patch Set #6, Line 145:</a> <code style="font-family:monospace,monospace">    c</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think we should set cont->err_app_cont to NULL if the length was only '1'?</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/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@159">Patch Set #6, Line 159:</a> <code style="font-family:monospace,monospace"> 1)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">shouldnt' this be +1, i.e. […]</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/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@217">Patch Set #6, Line 217:</a> <code style="font-family:monospace,monospace">       DE</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think somebody else already meantioned that those macros don't really look all that great and shou […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I have replaced the macros now - I used macros because I was afraid that I couldn't use different struct types as parameters, but I found a way now to cast the structs.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@256">Patch Set #6, Line 256:</a> <code style="font-family:monospace,monospace">   if (len < </code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">please try to use less magic numbers but instead sizeof() or offsetof() macros to gt to those 15 / 3 […]</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/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@259">Patch Set #6, Line 259:</a> <code style="font-family:monospace,monospace">    EN</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">same here regarding one line</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/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@300">Patch Set #6, Line 300:</a> <code style="font-family:monospace,monospace">     DE</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">why on one line?</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/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@355">Patch Set #6, Line 355:</a> <code style="font-family:monospace,monospace"> uint</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">putting 32kBytes on the stack didn't look very good to me. […]</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/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@359">Patch Set #6, Line 359:</a> <code style="font-family:monospace,monospace">   if (le</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">magic numbers</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/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@362">Patch Set #6, Line 362:</a> <code style="font-family:monospace,monospace">        ENC_RIM</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">magic numbers</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/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@461">Patch Set #6, Line 461:</a> <code style="font-family:monospace,monospace">       if (le</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">magic numbers</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/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@466">Patch Set #6, Line 466:</a> <code style="font-family:monospace,monospace">& s</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">no space, this looks like a logical "and" otherwise.</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/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@532">Patch Set #6, Line 532:</a> <code style="font-family:monospace,monospace">      if (len </code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">magic numbers</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/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@601">Patch Set #6, Line 601:</a> <code style="font-family:monospace,monospace">3</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmocore/+/21862">change 21862</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/+/21862"/><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: Ibbc7fd67658e3040c12abb5706fe9d1f31894352 </div>
<div style="display:none"> Gerrit-Change-Number: 21862 </div>
<div style="display:none"> Gerrit-PatchSet: 7 </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-CC: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-CC: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 08 Jan 2021 22:58:14 +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>