Change in osmo-pcu[master]: Initial handling support for RIM messages

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

laforge gerrit-no-reply at lists.osmocom.org
Fri Jan 22 10:05:30 UTC 2021


laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/22364 )

Change subject: Initial handling support for RIM messages
......................................................................


Patch Set 2: Code-Review-1

(9 comments)

sorry, I do think there needs to be some improvement

https://gerrit.osmocom.org/c/osmo-pcu/+/22364/2/src/gprs_bssgp_rim.c 
File src/gprs_bssgp_rim.c:

https://gerrit.osmocom.org/c/osmo-pcu/+/22364/2/src/gprs_bssgp_rim.c@31 
PS2, Line 31: 					   struct gprs_ra_id *raid, uint16_t cid)
input parameter 'raid' should be const


https://gerrit.osmocom.org/c/osmo-pcu/+/22364/2/src/gprs_bssgp_rim.c@45 
PS2, Line 45: 				    struct bssgp_ran_information_pdu *req_pdu)
input parameter 'req_pdu' should be const


https://gerrit.osmocom.org/c/osmo-pcu/+/22364/2/src/gprs_bssgp_rim.c@53 
PS2, Line 53: 				    struct bssgp_ran_information_pdu *req_pdu)
input parameter 'req_pdu' should be const


https://gerrit.osmocom.org/c/osmo-pcu/+/22364/2/src/gprs_bssgp_rim.c@70 
PS2, Line 70: static enum bssgp_ran_inf_app_id *get_app_id(struct bssgp_ran_information_pdu *pdu)
input parameter 'pdu' should be const


https://gerrit.osmocom.org/c/osmo-pcu/+/22364/2/src/gprs_bssgp_rim.c@89 
PS2, Line 89: static bool match_app_id(struct bssgp_ran_information_pdu *pdu, enum bssgp_ran_inf_app_id exp_app_id)
input parameter (and then also the local variable) should be const


https://gerrit.osmocom.org/c/osmo-pcu/+/22364/2/src/gprs_bssgp_rim.c@105 
PS2, Line 105: 
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?


https://gerrit.osmocom.org/c/osmo-pcu/+/22364/2/src/gprs_bssgp_rim.c@110 
PS2, Line 110: 		     "BSSGP RIM (NSEI=%u) only GERAN supported, destination cell is not a GERAN cell -- rejected.\n",
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.


https://gerrit.osmocom.org/c/osmo-pcu/+/22364/2/src/gprs_bssgp_rim.c@116 
PS2, Line 116: 		   
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.


https://gerrit.osmocom.org/c/osmo-pcu/+/22364/2/src/gprs_bssgp_rim.c@128 
PS2, Line 128: 		retu
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?

Sending a BSSGP status will end up with the SGSN, who has no clue what the contents of that message was.

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.



-- 
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/22364
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: Ia0ade0e97ea781ec655439c008b6cefaf3e90dec
Gerrit-Change-Number: 22364
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Comment-Date: Fri, 22 Jan 2021 10:05:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210122/bf89a3c7/attachment.htm>


More information about the gerrit-log mailing list