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