dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/21862 )
Change subject: bssgp_rim: add encoder/decoder for NACC related RIM containers ...................................................................... Patch Set 7: (18 comments) Thanks for reviewing! https://gerrit.osmocom.org/c/libosmocore/+/21862/6/include/osmocom/gprs/gprs_bssgp_rim.h File include/osmocom/gprs/gprs_bssgp_rim.h: https://gerrit.osmocom.org/c/libosmocore/+/21862/6/include/osmocom/gprs/gprs_bssgp_rim.h@2 PS6, Line 2: > license/copyright statement? Done https://gerrit.osmocom.org/c/libosmocore/+/21862/6/include/osmocom/gprs/gprs_bssgp_rim.h@7 PS6, Line 7: struct { > You could reuse "struct osmo_cell_global_id_ps" from https://gerrit.osmocom. > […] Makes sense, but I could not find a parsing function for the struct. I re-used the existing ones now. https://gerrit.osmocom.org/c/libosmocore/+/21862/6/include/osmocom/gprs/gprs_bssgp_rim.h@73 PS6, Line 73: Si3 > I don't recall the details: But won't we need SI3 as part of the NACC > feature? We have been waiting […] 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) 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. https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c File src/gb/gprs_bssgp_rim.c: https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@4 PS6, Line 4: * (C) 2020 by sysmocom - s.f.m.c. GmbH > -2021 now I guess? Done https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@144 PS6, Line 144: buf++; > so now buf point s to index '1' but the length check above passed with len == > 1, i.e. […] Done https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@145 PS6, Line 145: c > I think we should set cont->err_app_cont to NULL if the length was only '1'? Done https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@159 PS6, Line 159: 1) > shouldnt' this be +1, i.e. […] Done https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@217 PS6, Line 217: DE > I think somebody else already meantioned that those macros don't really look > all that great and shou […] 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. https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@256 PS6, Line 256: if (len < > please try to use less magic numbers but instead sizeof() or offsetof() > macros to gt to those 15 / 3 […] Done https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@259 PS6, Line 259: EN > same here regarding one line Done https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@300 PS6, Line 300: DE > why on one line? Done https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@355 PS6, Line 355: uint > putting 32kBytes on the stack didn't look very good to me. […] Done https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@359 PS6, Line 359: if (le > magic numbers Done https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@362 PS6, Line 362: ENC_RIM > magic numbers Done https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@461 PS6, Line 461: if (le > magic numbers Done https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@466 PS6, Line 466: & s > no space, this looks like a logical "and" otherwise. Done https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@532 PS6, Line 532: if (len > magic numbers Done https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@601 PS6, Line 601: 3 > ? Done -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/21862 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ibbc7fd67658e3040c12abb5706fe9d1f31894352 Gerrit-Change-Number: 21862 Gerrit-PatchSet: 7 Gerrit-Owner: dexter <pma...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: laforge <lafo...@osmocom.org> Gerrit-CC: pespin <pes...@sysmocom.de> Gerrit-Comment-Date: Fri, 08 Jan 2021 22:58:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge <lafo...@osmocom.org> Comment-In-Reply-To: pespin <pes...@sysmocom.de> Gerrit-MessageType: comment
