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 <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Pau Espin Pedrol <[email protected]> Gerrit-Reviewer: dexter <[email protected]> Gerrit-Comment-Date: Fri, 07 Dec 2018 17:35:49 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
