Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/12199 )
Change subject: gsm29118: add generator functions for GSM29118 messages ...................................................................... Patch Set 1: (6 comments) 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 complite SGsAP msg. */ typo: complete? https://gerrit.osmocom.org/#/c/12199/1/src/gsm/gsm29118.c@191 PS1, Line 191: return msgb_alloc_headroom(1024, 128, "SGsAP"); So 512 seems far more reasonable, right? https://gerrit.osmocom.org/#/c/12199/1/src/gsm/gsm29118.c@207 PS1, Line 207: len = 55; Probably makes sense to at least print an error if strlen(name) > 55 in this case. https://gerrit.osmocom.org/#/c/12199/1/src/gsm/gsm29118.c@219 PS1, Line 219: msgb_tlv_put(msg, SGSAP_IE_IMSI, len - 2, buf + 2); why not using msgb_put() if we already have the whole TLV in buf? because the TAG is different? https://gerrit.osmocom.org/#/c/12199/1/src/gsm/gsm29118.c@227 PS1, Line 227: msgb_tlv_put(msg, SGSAP_IE_LAI, 5, (uint8_t *) & lai_enc); sizeof(lai_enc) instead of 5 probably? https://gerrit.osmocom.org/#/c/12199/1/src/gsm/gsm29118.c@251 PS1, Line 251: struct msgb *gsm29118_create_dl_ud(const char *imsi, struct msgb *nas_msg) does it make sense to require a msgb here API-wise? probably a pointer + len makes more sense. This way if caller doesn't have a msgb doesn't need to create an extra one. -- 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: 1 Gerrit-Owner: dexter <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-CC: Pau Espin Pedrol <[email protected]> Gerrit-Comment-Date: Fri, 07 Dec 2018 12:06:03 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
