Change in libosmocore[master]: LCLS, TS 29.205: add GCR routines

[Neels Hofmeyr]

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

Change subject: LCLS, TS 29.205: add GCR routines
......................................................................


Patch Set 12: Code-Review-1

(15 comments)

https://gerrit.osmocom.org/#/c/11827/12/include/osmocom/gsm/gsm29_205.h
File include/osmocom/gsm/gsm29_205.h:

https://gerrit.osmocom.org/#/c/11827/12/include/osmocom/gsm/gsm29_205.h@1
PS12, Line 1: /*! \defgroup gsm29205 3GPP TS 29.205
(if everything is calld 29205, then maybe also call the file 29205 without _ ... personally I get annoyed by typing gsm_08_08.c and gsm_04_08.c, gsm0808 is so much quicker, that's why my pref would be no underscores... if you agree.)


https://gerrit.osmocom.org/#/c/11827/12/include/osmocom/gsm/gsm29_205.h@32
PS12, Line 32: /*! Parsed representation of Global Call Reference, 3GPP TS 29.205 Table B 2.1.9.1 */
.


https://gerrit.osmocom.org/#/c/11827/12/include/osmocom/gsm/gsm29_205.h@34
PS12, Line 34: 	uint8_t net[5];  /*< doc Network ID, ITU-T Q.1902.3 */
doc


https://gerrit.osmocom.org/#/c/11827/12/src/gsm/gsm29_205.c
File src/gsm/gsm29_205.c:

https://gerrit.osmocom.org/#/c/11827/12/src/gsm/gsm29_205.c@37
PS12, Line 37: /*! Create Global Call Reference
.


https://gerrit.osmocom.org/#/c/11827/12/src/gsm/gsm29_205.c@56
PS12, Line 56: 	msgb_lv_put(msg, 5, g->cr);
(but to reduce magic numbers, maybe sizeof(g->cr)?)


https://gerrit.osmocom.org/#/c/11827/12/src/gsm/gsm29_205.c@62
PS12, Line 62: /*! Decode Global Call Reference, 3GPP TS 29.205 Table B 2.1.9.1
.


https://gerrit.osmocom.org/#/c/11827/12/src/gsm/gsm29_205.c@72
PS12, Line 72: 		return -EBADMSG;
(I guess would also be -EINVAL?)


https://gerrit.osmocom.org/#/c/11827/12/src/gsm/gsm29_205.c@79
PS12, Line 79: 	parsed += (gcr->net_len + 1);
net_len is what you just parsed, and the +1?

If +1 is the following len, then parse and check that instead of ignoring


https://gerrit.osmocom.org/#/c/11827/12/src/gsm/gsm29_205.c@84
PS12, Line 84: 	if (elem[parsed - 1] != 5) /* see Table B 2.1.9.2 */
it's weird to first add +1 and then do -1 again. Rather drop the +1 above, then parse the len at elem[parsed], and then +1 after that.

(nice to use sizeof instead of magic)


https://gerrit.osmocom.org/#/c/11827/12/src/gsm/gsm29_205.c@85
PS12, Line 85: 		return -ENOENT;
I guess would also be -EINVAL?


https://gerrit.osmocom.org/#/c/11827/12/tests/gsm29205/gsm29205_test.c
File tests/gsm29205/gsm29205_test.c:

https://gerrit.osmocom.org/#/c/11827/12/tests/gsm29205/gsm29205_test.c@41
PS12, Line 41: 		0x46, 0x46, 0x46, 0x46, 0x46 /* .cr - Call. Ref. */
vary byte values


https://gerrit.osmocom.org/#/c/11827/12/tests/gsm29205/gsm29205_test.c@45
PS12, Line 45: 	struct gsm29205_gcr g = { .net_len = 3, .node = 0xDEAD }, p = { 0 };
= {}


https://gerrit.osmocom.org/#/c/11827/12/tests/gsm29205/gsm29205_test.c@53
PS12, Line 53: 	memset(g.net, 'U', g.net_len);
vary bytes


https://gerrit.osmocom.org/#/c/11827/12/tests/gsm29205/gsm29205_test.c@59
PS12, Line 59: 	if (!msgb_cmp_data_print(msg, res, ARRAY_SIZE(res)))
cmp() == 0 means match


https://gerrit.osmocom.org/#/c/11827/12/tests/gsm29205/gsm29205_test.c@64
PS12, Line 64: 		printf("decoding failed: %s [%s]\n", strerror(-rc), msgb_hexdump(msg));
abort(): it does not make sense to parse network id length and so on if not even the p got populated



-- 
To view, visit https://gerrit.osmocom.org/11827
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: Iee95aa4e5c056645b6cb5667e4a067097d52dfbf
Gerrit-Change-Number: 11827
Gerrit-PatchSet: 12
Gerrit-Owner: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
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 16:15: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/4595b19b/attachment.htm>