Change in libosmocore[master]: LCLS, TS 48.008: add GCR IE encoding/decoding

Neels Hofmeyr gerrit-no-reply at
Fri Nov 30 15:49:25 UTC 2018

Neels Hofmeyr has posted comments on this change. ( )

Change subject: LCLS, TS 48.008: add GCR IE encoding/decoding

Patch Set 5: Code-Review-1


what, I accidentally commented on an old patch set? but I think the comments still apply.
File include/osmocom/gsm/gsm0808_utils.h:
PS4, Line 86:  
PS4, Line 87: int gsm0808_dec_gcr(struct gsm29205_gcr *g, struct tlv_parsed *tp);
constify the tp arg plz.

kind of weird that the function name says 0808 and the struct says 29205. (edit) I think I get it now, this is the GCR 0808 message, and the IE is defined in 29.205?
File src/gsm/gsm0808_utils.c:
PS4, Line 485: /*! Create BSSMAP Global Call Reference, 3GPP TS 48.008 §
always end the summary with a '.' for doxygen
PS4, Line 493: 	len = msgb_v_put(msg, GSM0808_IE_GLOBAL_CALL_REF);
msbg_v_put works, but the name indicates that you are putting a V, a value, instead of this T, a tag.
I see in the rest of the file that we often use mgb_v_put() to add the message discriminator.
Which is actually accurate, because it is the start of the 0808 "value".

But for IE tags, let's use something with 't' in the name. There must be other places in this file that set the length after writing?

hmm, looking around, we should have a msgb_tl_put() function for these situations but it doesn't exist, and it seems code is working around variable-length values in different ways.

unless you find something nicer still, maybe rather use msgb_put_u8() instead of msgb_v_put(). Same thing but less misleading name.
PS4, Line 499: 	if (enc) {
the general exit-early style would be 'if (!enc) return 0' here instead.
PS4, Line 500: 		len[0] = enc;
(I'd prefer '*len = enc' since it isn't an array)
PS4, Line 507: /*! Decode BSSMAP Global Call Reference, 3GPP TS 29.205 Table B
PS4, Line 518: 	return 2 + gsm29205_dec_gcr(gcr, buf, TLVP_LEN(tp, GSM0808_IE_GLOBAL_CALL_REF));
are you sure gsm29205_dec_gcr can never and will never in the future return negative error / zero?
File tests/gsm0808/gsm0808_test.c:
PS4, Line 582: 		0x4e, 0x4e, 0x4e, 0x4e, 0x4e /* .cr - Call. Ref. */
nicer to use different byte value in each position, to ensure correct ordering. wait, didn't I say that before?
PS4, Line 586: 	struct gsm29205_gcr g = { .net_len = 3, .node = 0xDEAD }, p = { 0 };
also looks familiar: just write 'p = {}'
PS4, Line 594: 	memset(, 'O', g.net_len);
nicer to use differing bytes
PS4, Line 600: 	if (!msgb_cmp_data_print(msg, res, ARRAY_SIZE(res)))
"!foo_cmp(a, b)" in C convention means "a == b". If you want to use bool return value, don't call it 'cmp'!
PS4, Line 605: 		printf("parsing failed: %s [%s]\n", strerror(-rc), msgb_hexdump(msg));

To view, visit
To unsubscribe, or for help writing mail filters, visit

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82ce0207dc8de50689a8806c6471ad7fbae6219d
Gerrit-Change-Number: 12020
Gerrit-PatchSet: 5
Gerrit-Owner: Max <msuraev at>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Max <msuraev at>
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at>
Gerrit-Comment-Date: Fri, 30 Nov 2018 15:49:25 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <>

More information about the gerrit-log mailing list