<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 style="white-space: pre-wrap; word-wrap: break-word;">license/copyright statement?</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 style="white-space: pre-wrap; word-wrap: break-word;">I don't recall the details: But won't we need SI3 as part of the NACC feature? We have been waiting for months for NS2 API to stabilize to finally release a new libosmocore, and we cannot merge an API/ABI now that we expect to break soon after the merge because parts are missing.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/21862/5/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/5/include/osmocom/gprs/gprs_bssgp_rim.h@13">Patch Set #5, Line 13:</a> <code style="font-family:monospace,monospace">       const uint8_t *app_cont;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">is this a pointer to struct bssgp_ran_inf_rim_cont ?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think so, it's opaque user data (of length app_cont_len)</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 style="white-space: pre-wrap; word-wrap: break-word;">-2021 now I guess?</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 style="white-space: pre-wrap; word-wrap: break-word;">so now buf point s to index '1' but the length check above passed with len == 1, i.e. we are pointing one beind the end of the buffer</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 style="white-space: pre-wrap; word-wrap: break-word;">I think we should set cont->err_app_cont to NULL if the length was only '1'?</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 style="white-space: pre-wrap; word-wrap: break-word;">shouldnt' this be +1, i.e. the cause byte plus the container content?</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 style="white-space: pre-wrap; word-wrap: break-word;">I think somebody else already meantioned that those macros don't really look all that great and should be - if possible replaced by functions.</p><p style="white-space: pre-wrap; word-wrap: break-word;">But what I don't understand is why the entire macro and the 'if' statement would need to go into a single line? IS there some logic I'm missing? tis loos like perl syntax "ACTION if CONDITION" whcih obviously is not supported in C.</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 style="white-space: pre-wrap; word-wrap: break-word;">please try to use less magic numbers but instead sizeof() or offsetof() macros to gt to those 15 / 3 / 3.  Alternatively, put a comment in the line above exlpaining what those lengths are.  Otherwise the reader/reviewer has a hard time knowing if those are correct.</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 style="white-space: pre-wrap; word-wrap: break-word;">same here regarding one line</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 style="white-space: pre-wrap; word-wrap: break-word;">why on one line?</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 style="white-space: pre-wrap; word-wrap: break-word;">putting 32kBytes on the stack didn't look very good to me.  But then I read up and determined it should work on most systems today (linux apparently now has a default stack size of 2MB per thread, often 8MB).</p><p style="white-space: pre-wrap; word-wrap: break-word;">If the encoder functions were using msgb, it might have been easier to first put the inner TLV and then later wrap (push) the outer TLV around it.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Without msgb it's probably a bit harder, but it looks like the outer heaer is of fixed length, so you could call bssgp_enc_app_err_cont_nacc() with the exact location after where later the tag/length fields will be?</p><p style="white-space: pre-wrap; word-wrap: break-word;">In any case, not really worth spending time on, given that the 32k on the stack apparently won't hurt.</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 style="white-space: pre-wrap; word-wrap: break-word;">magic numbers</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 style="white-space: pre-wrap; word-wrap: break-word;">magic numbers</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 style="white-space: pre-wrap; word-wrap: break-word;">magic numbers</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 style="white-space: pre-wrap; word-wrap: break-word;">no space, this looks like a logical "and" otherwise.</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 style="white-space: pre-wrap; word-wrap: break-word;">magic numbers</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 style="white-space: pre-wrap; word-wrap: break-word;">?</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: 6 </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: Thu, 07 Jan 2021 10:01:25 +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: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>