Change in libosmocore[master]: gsm0808: add encoder for cause codes and use it

Max gerrit-no-reply at lists.osmocom.org
Fri Nov 30 15:32:19 UTC 2018


Max has posted comments on this change. ( https://gerrit.osmocom.org/12044 )

Change subject: gsm0808: add encoder for cause codes and use it
......................................................................


Patch Set 1:

(3 comments)

I think the test should be added as well. It's rather easy and the user of this function are outside of the library so it wouldn't be immediately obvious that smth is broken if we don't test for it.

https://gerrit.osmocom.org/#/c/12044/1/src/gsm/gsm0808_utils.c
File src/gsm/gsm0808_utils.c:

https://gerrit.osmocom.org/#/c/12044/1/src/gsm/gsm0808_utils.c@51
PS1, Line 51: /*! Encode TS 08.08 AoIP Cause IE
I think it's better to reference the same spec as in body of this function.


https://gerrit.osmocom.org/#/c/12044/1/src/gsm/gsm0808_utils.c@67
PS1, Line 67: 		buf[1] = (uint8_t) (cause & 0xff);
I think it's better to use osmo_store16*() - that way it's immediately obvious which endian is used. Plus it's easier to read.


https://gerrit.osmocom.org/#/c/12044/1/src/gsm/gsm0808_utils.c@74
PS1, Line 74: 	return (uint8_t) (msg->tail - old_tail);
You know for sure how many bytes are added - just return explicit number from corresponding if clause, there's no need to bother with pointer arithmetic.



-- 
To view, visit https://gerrit.osmocom.org/12044
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: I71d58fad89502a43532f60717ca022c15c73f8bb
Gerrit-Change-Number: 12044
Gerrit-PatchSet: 1
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Comment-Date: Fri, 30 Nov 2018 15:32:19 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181130/1b1f8362/attachment.html>


More information about the gerrit-log mailing list