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/.
pespin gerrit-no-reply at lists.osmocom.orgpespin 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) YOu need to resolve the merging conflicts. Some of my comments are not really hardlines, just some ideas to improve the code. 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 ? 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? Even better, one struct with a union handling different msg types. It may also make sense to write it in primitive<->msgb alike code like we do in other protocol stacks. 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. 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. Let the compiler decide whether to inline or not. 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. 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) return -EINVAL; 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 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. 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. -- 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: Mon, 04 Jan 2021 09:33:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210104/7ed75f14/attachment.htm>