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/.
Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
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 <msuraev at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-HasComments: Yes