Change in libosmocore[master]: bssgp_rim: add encoder/decoder for NACC related RIM containers

historical

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 <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge at osmocom.org>
Gerrit-CC: pespin <pespin at 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 <laforge at osmocom.org>
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210108/07d8ec28/attachment.htm>