Patch Set 3: Code-Review-1 (9 comments)
https://gerrit.osmocom.org/#/c/2165/3//COMMIT_MSG Commit Message: Line 9: * data structure represnting 3GPP TS 52.021 ยง9.4.62 SW Description (typo) https://gerrit.osmocom.org/#/c/2165/3/include/osmocom/gsm/protocol/gsm_12_21.h File include/osmocom/gsm/protocol/gsm_12_21.h: Line 796: struct osmo_sw_descr { struct of the API is prefixed 'osmo_' and functions 'abis_nm_'? would it make sense to use the same prefix instead? https://gerrit.osmocom.org/#/c/2165/3/src/gsm/abis_nm.c File src/gsm/abis_nm.c: Line 755: * \param[in] msg message buffer isn't that technically a \param[out]? Line 757: * \param[in] put_header boolean, whether to put NM_ATT_SW_DESCR IE or not call it 'put_sw_descr' instead? Line 758: * \param[in] simulate boolean, whether to actually modify msg or not I guess it would make more sense to have the len calculation in a separate function -- no production code would ever simulate, right? Line 765: (sw->file_version_len + 3); (indent -= 1 space?) Where do the magic 3s come from? Line 789: const struct tlv_definition sw_tlvdef = { static? Line 798: itself is considered optional. */ Ah, I'm beginning to understand, the SW DESCR is a TV, where the V is another set of TLVs. Maybe this comment could highlight that. In case tlv_parse() returns zero, there is no outer TV and we can exit further parsing right away. Just noticing: tlv_parse()'s API doc is wrong, it returns the number of parsed TLV elements, not the bytes! I think this should be tightened. Maybe the outer SW DESCR TV should be parsed in a separate stage, feeding the V of FILE_ID and FILE_VERSION to this function only when they are actually present? Because, from above tlv_definition, the SW_DESCR TV could come at any position as far as the tlv_parse()r is concerned: maybe first some FILE_ID, then a SW_DESCR... I guess we should prevent that. https://gerrit.osmocom.org/#/c/2165/3/tests/msgb/msgb_test.c File tests/msgb/msgb_test.c: Line 202: res = abis_nm_get_sw_descr(get, msgb_data(msg), msg->len); I'm pretty sure that this test does not belong in msgb_test.c. This file is for generig msgb testing, not for specific payloads. -- To view, visit https://gerrit.osmocom.org/2165 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib63b6b5e83b8914864fc7edd789f8958cdc993cd Gerrit-PatchSet: 3 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: Max <msur...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Max <msur...@sysmocom.de> Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-HasComments: Yes
