laforge 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 6: (18 comments) 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? 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 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. https://gerrit.osmocom.org/c/libosmocore/+/21862/5/include/osmocom/gprs/gprs_bssgp_rim.h File include/osmocom/gprs/gprs_bssgp_rim.h: https://gerrit.osmocom.org/c/libosmocore/+/21862/5/include/osmocom/gprs/gprs_bssgp_rim.h@13 PS5, Line 13: const uint8_t *app_cont; > is this a pointer to struct bssgp_ran_inf_rim_cont ? I don't think so, it's opaque user data (of length app_cont_len) 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? 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. we are pointing one beind the end of the buffer 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'? 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. the cause byte plus the container content? 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 should be - if possible replaced by functions. 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. 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 / 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. https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@259 PS6, Line 259: EN same here regarding one line https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@300 PS6, Line 300: DE why on one line? 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. 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). 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. 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? In any case, not really worth spending time on, given that the 32k on the stack apparently won't hurt. https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@359 PS6, Line 359: if (le magic numbers https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@362 PS6, Line 362: ENC_RIM magic numbers https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@461 PS6, Line 461: if (le magic numbers 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. https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@532 PS6, Line 532: if (len magic numbers https://gerrit.osmocom.org/c/libosmocore/+/21862/6/src/gb/gprs_bssgp_rim.c@601 PS6, Line 601: 3 ? -- 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: 6 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: Thu, 07 Jan 2021 10:01:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pes...@sysmocom.de> Gerrit-MessageType: comment
