Change in libosmocore[master]: gsm29118: add generator functions for GSM29118 messages

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

dexter gerrit-no-reply at lists.osmocom.org
Fri Dec 7 17:35:49 UTC 2018


dexter has posted comments on this change. ( https://gerrit.osmocom.org/12199 )

Change subject: gsm29118: add generator functions for GSM29118 messages
......................................................................


Patch Set 3:

(9 comments)

> Build Started https://jenkins.osmocom.org/jenkins/job/gerrit-libosmocore/1287/

https://gerrit.osmocom.org/#/c/12199/1/src/gsm/gsm29118.c
File src/gsm/gsm29118.c:

https://gerrit.osmocom.org/#/c/12199/1/src/gsm/gsm29118.c@186
PS1, Line 186: /* Allocate an empty message buffer, suitable to hold a complete SGsAP msg. */
> typo: complete?
Done


https://gerrit.osmocom.org/#/c/12199/1/src/gsm/gsm29118.c@191
PS1, Line 191: 	return msgb_alloc_headroom(1024, 512, "SGsAP");
> So 512 seems far more reasonable, right?
Done


https://gerrit.osmocom.org/#/c/12199/1/src/gsm/gsm29118.c@207
PS1, Line 207: 		return -1;
> Probably makes sense to at least print an error if strlen(name) > 55 in this case.
I think we should try to avoid error printing here since we do not have any real reference here. I think its better to have a return code so that the caller can know that an error occurred.


https://gerrit.osmocom.org/#/c/12199/1/src/gsm/gsm29118.c@219
PS1, Line 219: 	/* skip first two bytes (tag+length) so we can use msgb_tlv_put */
> why not using msgb_put() if we already have the whole TLV in buf? because the TAG is different?
Yes its, different, we need to chop it off and use the SGSAP tag.


https://gerrit.osmocom.org/#/c/12199/1/src/gsm/gsm29118.c@227
PS1, Line 227: 	gsm48_generate_lai2(&lai_enc, lai);
> sizeof(lai_enc) instead of 5 probably?
Done


https://gerrit.osmocom.org/#/c/12199/1/src/gsm/gsm29118.c@251
PS1, Line 251:  *  \returns callee-allocated msgb with the encoded message. */
> does it make sense to require a msgb here API-wise? probably a pointer + len makes more sense. […]
In our case it makes sense because in the MSC we already have the data ready as a message buffer. I think this is fine.


https://gerrit.osmocom.org/#/c/12199/2/src/gsm/gsm29118.c
File src/gsm/gsm29118.c:

https://gerrit.osmocom.org/#/c/12199/2/src/gsm/gsm29118.c@207
PS2, Line 207: 		return -1;
> setting a a local value then returning -1? That makes no sense.
Thanks for catching that. I have hopefully fixed it now.


https://gerrit.osmocom.org/#/c/12199/2/src/gsm/gsm29118.c@222
PS2, Line 222: 
> why not using msgb_put() if we already have the whole TLV in buf? because the TAG is different?
Yes, its different. We need to chop it off and use the SGSAP tag. (I responded to this earler, gerrit is swallowing messages again?)


https://gerrit.osmocom.org/#/c/12199/2/src/gsm/gsm29118.c@254
PS2, Line 254: 	struct msgb *msg = gsm29118_msgb_alloc();
> does it make sense to require a msgb here API-wise? probably a pointer + len makes more sense. […]
In osmo-msc we already have the nas_msg as a message buffer, so it makes sense to use a message buffer here as well.



-- 
To view, visit https://gerrit.osmocom.org/12199
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: Ic87f8a771b87b52215d0a7451b67794557b80b8a
Gerrit-Change-Number: 12199
Gerrit-PatchSet: 3
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-Comment-Date: Fri, 07 Dec 2018 17:35:49 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181207/01334027/attachment.htm>


More information about the gerrit-log mailing list