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

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/.

dexter gerrit-no-reply at lists.osmocom.org
Wed Jan 6 09:53:43 UTC 2021


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 5:

(9 comments)

I did have done the API changes (unions, full parse) now. However, I did not update the patch yet since I am not entirely done yet. There are basically two problems left:

The encoder functions currently write to an uint8_t buffer but depending on how the API will be used an msg buffer might be better. I started working on osmo-pcu, there I will use the API, I first want to see how things turn out there. One big adavantage of msg buffers would also be that they have array bounds checking.

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 ?
yes, but to its unparsed binary version. The memory location is inside *buf, which is given as parameter to the decoder function. So, when buf is freed there will be a problem, but I do not see any alternative. The strings can be up to 65535 (or 32767?, I am not sure), so reserving this statically memory is not a good option. Dynamic allocation is also not very attractive here...


https://gerrit.osmocom.org/c/libosmocore/+/21862/5/include/osmocom/gprs/gprs_bssgp_rim.h@73 
PS5, Line 73: 	uint8_t app_id;
> I see lots of structs sharing half of the fields. What about adding a HDR struct or alike? […]
I thought about having a header shared over all message types. However, when looking closely its always a bit different. We would have to put a header that satisfies every message / container type (bool xyz_present members) but then the structs do not match up with the spec anymore. I would like to keep it like this.

I have now put unions into the application container structs, so things become easier at least at that level. We could also add a "master" struct that unifies all possible rim container types, then we could have a single function that decodes/encodes everything. However I think this would be overkill since in the actual application we would still need If or switch depending on a discriminator. Then it is probably better to go by the TLV IEI on that level.


https://gerrit.osmocom.org/c/libosmocore/+/21862/5/src/gb/gprs_bssgp_rim.c 
File src/gb/gprs_bssgp_rim.c:

https://gerrit.osmocom.org/c/libosmocore/+/21862/5/src/gb/gprs_bssgp_rim.c@32 
PS5, Line 32: #define DEC_RIM_CONT_COMMON \
> this looks more like a function tbh.
It was a function...


https://gerrit.osmocom.org/c/libosmocore/+/21862/5/src/gb/gprs_bssgp_rim.c@51 
PS5, Line 51: #define ENC_RIM_CONT_COMMON \
> same, would be a lot clearer (to read and ebug) having a static function. […]
If I do it that way I can only call the function with one struct type, so it does not work for me.

Its probably not that effective anyway. I think its only used in 3 locations anyway.


https://gerrit.osmocom.org/c/libosmocore/+/21862/5/src/gb/gprs_bssgp_rim.c@68 
PS5, Line 68: 	memset(cont, 0, sizeof(*cont));
> this memset not needed afaict.
Its not needed because I set the struct members to NULL / 0 in the else branches below. I think its better to remove the else branches and keep the memset.


https://gerrit.osmocom.org/c/libosmocore/+/21862/5/src/gb/gprs_bssgp_rim.c@106 
PS5, Line 106: 	ENC_RIM_CONT_COMMON
> if (enc_rim_cont_common(cont) < 0) […]
This is all in the macro.


https://gerrit.osmocom.org/c/libosmocore/+/21862/5/src/gb/gprs_bssgp_rim.c@127 
PS5, Line 127: 	memset(cont, 0, sizeof(*cont));
> memset not needed afaict
see above


https://gerrit.osmocom.org/c/libosmocore/+/21862/5/src/gb/gprs_bssgp_rim.c@288 
PS5, Line 288: 	} else
> coding sytle: else clause shoudl also have brackets if "if" clause have them.
Done


https://gerrit.osmocom.org/c/libosmocore/+/21862/5/src/gb/gprs_bssgp_rim.c@514 
PS5, Line 514: 	return (int)(buf_ptr - buf);
> You may need to cast this through intptr_t to avoid warnings.
like this: return (int)((intptr_t)buf_ptr - (intptr_t)buf) ?



-- 
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: 5
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Wed, 06 Jan 2021 09:53:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
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/20210106/28cdd842/attachment.htm>


More information about the gerrit-log mailing list