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 <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Max <[email protected]> Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Pau Espin Pedrol <[email protected]> Gerrit-Comment-Date: Fri, 30 Nov 2018 15:49:25 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
