Change in libosmocore[master]: add BSSMAP-LE coding for Location Services

[laforge]

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 <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Comment-Date: Thu, 01 Oct 2020 10:09:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20201001/9756b079/attachment.htm>