Change in libosmocore[master]: LCLS: add GCR routines

[Neels Hofmeyr]

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

Change subject: LCLS: add GCR routines
......................................................................


Patch Set 7: Code-Review-1

(9 comments)

https://gerrit.osmocom.org/#/c/11827/7/include/osmocom/gsm/gsm0808_utils.h
File include/osmocom/gsm/gsm0808_utils.h:

https://gerrit.osmocom.org/#/c/11827/7/include/osmocom/gsm/gsm0808_utils.h@71
PS7, Line 71: 	uint8_t net[5];  /* Network ID, ITU-T Q.1902.3 */
doxygen wise, these comments should be of the form

  /*< doc */

because otherwise they end up being associated to the following item instead of the previous one.

actually ... I guess they get dropped by doxygen...

I also see the rest of this file not doing that, so I guess this is optional.

Maybe we should just drop doxygen altogether...


https://gerrit.osmocom.org/#/c/11827/7/src/gsm/gsm0808.c
File src/gsm/gsm0808.c:

https://gerrit.osmocom.org/#/c/11827/7/src/gsm/gsm0808.c@408
PS7, Line 408: /*! Create BSSMAP Global Call Reference
IIUC the GCR is some IE or IE content, so this should be a gsm0808_enc_gcr() function with similar semantics like the other gsm0808_enc_* functions. i.e. pass a target msgb to this function and make it append the GCR to that, instead of returning an entire msgb just containing the GCR.


https://gerrit.osmocom.org/#/c/11827/7/src/gsm/gsm0808_utils.c
File src/gsm/gsm0808_utils.c:

https://gerrit.osmocom.org/#/c/11827/7/src/gsm/gsm0808_utils.c@675
PS7, Line 675: int gsm0808_dec_gcr(struct gsm0808_gcr *gcr, const struct msgb *msg)
I'm expecting that the GCR is some IE or IE content, so instead it would make more sense to pass {pointer,max_length} to this function and parse that instead of an entire msgb. The current code implies that we decode the same TLV structure all over for every single IE.

See the other *_dec_* functions in this file.

Why is the decoding function in a different .c file than the encoding function? Let's put all of them at the bottom of all the existing gsm0808_{enc,dec} function definitions.


https://gerrit.osmocom.org/#/c/11827/7/src/gsm/gsm0808_utils.c@700
PS7, Line 700: 	if (buf[point] != 5) /* see Table B 2.1.9.2 */
(prefer comment above the 'if')


https://gerrit.osmocom.org/#/c/11827/7/tests/gsm0808/gsm0808_test.c
File tests/gsm0808/gsm0808_test.c:

https://gerrit.osmocom.org/#/c/11827/7/tests/gsm0808/gsm0808_test.c@137
PS7, Line 137: 	struct gsm0808_gcr g = { .net_len = 3, .node = 0xDEAD }, p = { 0 };
(BTW, just writing '{}' is identical to '{ 0 }' and it also works with all other structs and arrays even if the first member is not an int)

(normally we prefer one var def per line)


https://gerrit.osmocom.org/#/c/11827/7/tests/gsm0808/gsm0808_test.c@141
PS7, Line 141: 	memset(g.net, 'U', g.net_len);
... F. U? are you telling me something?? :)


https://gerrit.osmocom.org/#/c/11827/7/tests/gsm0808/gsm0808_test.c@145
PS7, Line 145: 	msg->l3h = msgb_wrap_with_TL(msg, GSM0808_IE_GLOBAL_CALL_REF);
seeing this, I believe the IE discriminator should also be part of gsm0808_enc_gcr().


https://gerrit.osmocom.org/#/c/11827/7/tests/gsm0808/gsm0808_test.c@149
PS7, Line 149: 	if (rc < 0)
(technically this should exit now, but nevermind)


https://gerrit.osmocom.org/#/c/11827/7/tests/gsm0808/gsm0808_test.c@1811
PS7, Line 1811: 	test_create_gcr();
so the function definition is above test_create_reset(), but it gets called after it? why not just put the new test at the bottom, both def and invocation?



-- 
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: 7
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: Wed, 21 Nov 2018 17:13:38 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181121/9e674d97/attachment.htm>