laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/19417 )
Change subject: Gb: add a second NS implementation ...................................................................... Patch Set 23: Code-Review+1 (6 comments) 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? https://gerrit.osmocom.org/c/libosmocore/+/19417/23/include/osmocom/gprs/gprs_ns2.h File include/osmocom/gprs/gprs_ns2.h: https://gerrit.osmocom.org/c/libosmocore/+/19417/23/include/osmocom/gprs/gprs_ns2.h@86 PS23, Line 86: link_selector_parameter 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. https://gerrit.osmocom.org/c/libosmocore/+/19417/23/src/gb/Makefile.am File src/gb/Makefile.am: https://gerrit.osmocom.org/c/libosmocore/+/19417/23/src/gb/Makefile.am@25 PS23, Line 25: gprs_ns2_message.c gprs_ns2_vty.c \ 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. https://gerrit.osmocom.org/c/libosmocore/+/19417/23/src/gb/gprs_ns2.c File src/gb/gprs_ns2.c: https://gerrit.osmocom.org/c/libosmocore/+/19417/23/src/gb/gprs_ns2.c@187 PS23, Line 187: oid gprs_ns2_set_log_ss(int ss) : { : DNS = ss; : } we could just add a global "DLNS" to libosmocore, but we can also keep this style. https://gerrit.osmocom.org/c/libosmocore/+/19417/23/src/gb/gprs_ns2.c@876 PS23, Line 876: /* TODO: remove this global, why the hell do I use it */ : static bool gprs_fsm_vc_registered = false; : static bool gprs_fsm_sns_registered = false; indeed ;) https://gerrit.osmocom.org/c/libosmocore/+/19417/23/src/gb/gprs_ns2_message.c File src/gb/gprs_ns2_message.c: https://gerrit.osmocom.org/c/libosmocore/+/19417/23/src/gb/gprs_ns2_message.c@64 PS23, Line 64: int gprs_ns2_validate_reset(struct gprs_ns2_vc *nsvc, struct msgb *msg, struct tlv_parsed *tp, uint8_t *cause) 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 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. In any case, the code here can and should stay as-is. We can switch to a different data-driven verification codebase later on. https://gerrit.osmocom.org/c/libosmocore/+/19417/23/src/gb/gprs_ns2_message.c@74 PS23, Line 74: int gprs_ns2_validate_reset_ack(struct gprs_ns2_vc *nsvc, struct msgb *msg, struct tlv_parsed *tp, uint8_t *cause) shouldn't all these be 'static' and called by the dispatcher function below? -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/19417 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I3525beef205588dfab9d3880a34115f1a2676e48 Gerrit-Change-Number: 19417 Gerrit-PatchSet: 23 Gerrit-Owner: lynxis lazus <lyn...@fe80.eu> Gerrit-Assignee: daniel <dan...@totalueberwachung.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillm...@sysmocom.de> Gerrit-Reviewer: laforge <lafo...@osmocom.org> Gerrit-Reviewer: lynxis lazus <lyn...@fe80.eu> Gerrit-CC: daniel <dan...@totalueberwachung.de> Gerrit-CC: pespin <pes...@sysmocom.de> Gerrit-Comment-Date: Tue, 08 Sep 2020 16:25:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
