<p style="white-space: pre-wrap; word-wrap: break-word;">sorry, I do think there needs to be some improvement</p><p>Patch set 2:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22364">View Change</a></p><p>9 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22364/2/src/gprs_bssgp_rim.c">File src/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/osmo-pcu/+/22364/2/src/gprs_bssgp_rim.c@31">Patch Set #2, Line 31:</a> <code style="font-family:monospace,monospace"> struct gprs_ra_id *raid, uint16_t cid)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">input parameter 'raid' should be const</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22364/2/src/gprs_bssgp_rim.c@45">Patch Set #2, Line 45:</a> <code style="font-family:monospace,monospace"> struct bssgp_ran_information_pdu *req_pdu)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">input parameter 'req_pdu' should be const</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22364/2/src/gprs_bssgp_rim.c@53">Patch Set #2, Line 53:</a> <code style="font-family:monospace,monospace"> struct bssgp_ran_information_pdu *req_pdu)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">input parameter 'req_pdu' should be const</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22364/2/src/gprs_bssgp_rim.c@70">Patch Set #2, Line 70:</a> <code style="font-family:monospace,monospace">static enum bssgp_ran_inf_app_id *get_app_id(struct bssgp_ran_information_pdu *pdu)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">input parameter 'pdu' should be const</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22364/2/src/gprs_bssgp_rim.c@89">Patch Set #2, Line 89:</a> <code style="font-family:monospace,monospace">static bool match_app_id(struct bssgp_ran_information_pdu *pdu, enum bssgp_ran_inf_app_id exp_app_id)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">input parameter (and then also the local variable) should be const</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22364/2/src/gprs_bssgp_rim.c@105">Patch Set #2, Line 105:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">should we OSMO_ASSERT that the SAP/type really is the RIM sap and we didn't get handled some completely different osmo_bssgp_prim here?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22364/2/src/gprs_bssgp_rim.c@110">Patch Set #2, Line 110:</a> <code style="font-family:monospace,monospace"> "BSSGP RIM (NSEI=%u) only GERAN supported, destination cell is not a GERAN cell -- rejected.\n",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">do you notice that every of your log lines starts with "BSSGP RIM (NSEI=%u)"? Please #define a LOGRIM(nsei, level, ...) macro t o avoid re-defining that formatting over and over again. Passing the sub-system to that macro is no long er necessary, it can use DRIM hard-coded.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22364/2/src/gprs_bssgp_rim.c@116">Patch Set #2, Line 116:</a> <code style="font-family:monospace,monospace"> </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why should an UMTS or LTE cell not be able to ask us for NACC related information? As far as I undertand, they use the exact same messages in case they want to tell a UE (over UTRAN/EUTRAN) about SI of a GERAN neighbor cell. For the PCU nothing changes, except that the (opaque) source is of a different type. WE shouldn't care and simply copy the "source" to the "destination" when responding and let the SGSN figure out what to do with it.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22364/2/src/gprs_bssgp_rim.c@128">Patch Set #2, Line 128:</a> <code style="font-family:monospace,monospace"> retu</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">is BSSGP STATUS really the proper response here? IMHO this is one layer too low. Isn't that what the RIM ERROR, or in this case probably even the RIM APPLICATION ERROR is for?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Sending a BSSGP status will end up with the SGSN, who has no clue what the contents of that message was.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Sending a RIM specific error or application error will be routed back to whoever send us the request. And that is the entity that should be notified, as it sent us a non-matching cell-id.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22364">change 22364</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-pcu/+/22364"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-pcu </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Ia0ade0e97ea781ec655439c008b6cefaf3e90dec </div>
<div style="display:none"> Gerrit-Change-Number: 22364 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 22 Jan 2021 10:05:30 +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>