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

[Neels Hofmeyr]

Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12020 )

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


Patch Set 5: Code-Review-1

(13 comments)

what, I accidentally commented on an old patch set? but I think the comments still apply.

https://gerrit.osmocom.org/#/c/12020/4/include/osmocom/gsm/gsm0808_utils.h
File include/osmocom/gsm/gsm0808_utils.h:

https://gerrit.osmocom.org/#/c/12020/4/include/osmocom/gsm/gsm0808_utils.h@86
PS4, Line 86:  
(ws)


https://gerrit.osmocom.org/#/c/12020/4/include/osmocom/gsm/gsm0808_utils.h@87
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?


https://gerrit.osmocom.org/#/c/12020/4/src/gsm/gsm0808_utils.c
File src/gsm/gsm0808_utils.c:

https://gerrit.osmocom.org/#/c/12020/4/src/gsm/gsm0808_utils.c@485
PS4, Line 485: /*! Create BSSMAP Global Call Reference, 3GPP TS 48.008 §3.2.2.115
always end the summary with a '.' for doxygen


https://gerrit.osmocom.org/#/c/12020/4/src/gsm/gsm0808_utils.c@493
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.


https://gerrit.osmocom.org/#/c/12020/4/src/gsm/gsm0808_utils.c@499
PS4, Line 499: 	if (enc) {
the general exit-early style would be 'if (!enc) return 0' here instead.


https://gerrit.osmocom.org/#/c/12020/4/src/gsm/gsm0808_utils.c@500
PS4, Line 500: 		len[0] = enc;
(I'd prefer '*len = enc' since it isn't an array)


https://gerrit.osmocom.org/#/c/12020/4/src/gsm/gsm0808_utils.c@507
PS4, Line 507: /*! Decode BSSMAP Global Call Reference, 3GPP TS 29.205 Table B 2.1.9.1
'.'


https://gerrit.osmocom.org/#/c/12020/4/src/gsm/gsm0808_utils.c@518
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?


https://gerrit.osmocom.org/#/c/12020/4/tests/gsm0808/gsm0808_test.c
File tests/gsm0808/gsm0808_test.c:

https://gerrit.osmocom.org/#/c/12020/4/tests/gsm0808/gsm0808_test.c@582
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?


https://gerrit.osmocom.org/#/c/12020/4/tests/gsm0808/gsm0808_test.c@586
PS4, Line 586: 	struct gsm29205_gcr g = { .net_len = 3, .node = 0xDEAD }, p = { 0 };
also looks familiar: just write 'p = {}'


https://gerrit.osmocom.org/#/c/12020/4/tests/gsm0808/gsm0808_test.c@594
PS4, Line 594: 	memset(g.net, 'O', g.net_len);
nicer to use differing bytes


https://gerrit.osmocom.org/#/c/12020/4/tests/gsm0808/gsm0808_test.c@600
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'!


https://gerrit.osmocom.org/#/c/12020/4/tests/gsm0808/gsm0808_test.c@605
PS4, Line 605: 		printf("parsing failed: %s [%s]\n", strerror(-rc), msgb_hexdump(msg));
abort()?



-- 
To view, visit https://gerrit.osmocom.org/12020
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

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 sysmocom.de>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
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: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181130/ff0b5363/attachment.htm>