laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/20334 )
Change subject: add BSSMAP-LE coding for Location Services ...................................................................... Patch Set 1: Code-Review-1 (5 comments) https://gerrit.osmocom.org/c/libosmocore/+/20334/1/include/osmocom/gsm/bssmap_le.h File include/osmocom/gsm/bssmap_le.h: https://gerrit.osmocom.org/c/libosmocore/+/20334/1/include/osmocom/gsm/bssmap_le.h@43 PS1, Line 43: OSMO_BSSMAP_LE_MSGT_PERFORM_LOC_REQ = 0x2b, same comment as before in the BSSLAP patch. Those should be 3GPP constants and hence not be in this header file, nor have an osmo prefix. https://gerrit.osmocom.org/c/libosmocore/+/20334/1/src/gsm/bssmap_le.c File src/gsm/bssmap_le.c: https://gerrit.osmocom.org/c/libosmocore/+/20334/1/src/gsm/bssmap_le.c@47 PS1, Line 47: OSMO_BSSMAP_LE_IEI_GANSS_LOCATION_TYPE = 0x82, again those look like 3GPP related #defines that should go into a /protocol/ header file, not have OSMO prefix, ... https://gerrit.osmocom.org/c/libosmocore/+/20334/1/src/gsm/bssmap_le.c@317 PS1, Line 317: #define PARSE_ERR(ERRMSG) \ this is highly untypical for our code. I would prefer such functions to retnurn a machine-readable error (like a cause value, either spec-compliant or osmocom-specific) so that the caller can actually interpret the result rather than receiving just a human-readable string. Transformation from cause/error code to human-readable string can then be done separately by value_string. https://gerrit.osmocom.org/c/libosmocore/+/20334/1/src/gsm/bssmap_le.c@335 PS1, Line 335: msgb_put_u8(msg, OSMO_BSSMAP_LE_IEI_APDU); : l = msgb_put(msg, 2); : old_tail = msg->tail; : msgb_put_u8(msg, OSMO_BSSMAP_LE_APDU_PROT_BSSLAP); : int rc = osmo_bsslap_enc(msg, bsslap); : if (rc <= 0) : return -EINVAL; : osmo_store16be(msg->tail - old_tail, l); you can probably avoid the "let's first reserve two bytes and store a local pointer to whcih we later manually encode a 16bit length" if you change the order: * first msgb_put the payload of the message * then msgb_push the header (including length) in front, at a time where you already know the length of the payload https://gerrit.osmocom.org/c/libosmocore/+/20334/1/src/gsm/bssmap_le.c@349 PS1, Line 349: : if (len < 1) : return "APDU too short"; : : proto = data[0]; : : switch (proto) { : case OSMO_BSSMAP_LE_APDU_PROT_BSSLAP: : return osmo_bsslap_dec(bsslap, data + 1, len - 1); : default: : return "Unimplemented APDU type likewise here. Please return error values (cause values, errno, custom error codes, ...) and not strings. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/20334 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I271e59b794bafc0a7ae0eabbf58918f6d7df431d Gerrit-Change-Number: 20334 Gerrit-PatchSet: 1 Gerrit-Owner: neels <nhofm...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <lafo...@osmocom.org> Gerrit-Comment-Date: Thu, 01 Oct 2020 10:09:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
