Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/11920 )
Change subject: libmsc/gsm_04_11.c: accept MT SMS messages over GSUP ...................................................................... Patch Set 11: Code-Review-1 (8 comments) (in general, I think you can still drop a lot of comments ... if you can be bothered) https://gerrit.osmocom.org/#/c/11920/11/src/libmsc/gsm_04_08.c File src/libmsc/gsm_04_08.c: https://gerrit.osmocom.org/#/c/11920/11/src/libmsc/gsm_04_08.c@1802 PS11, Line 1802: /* GSM 04.11 code implementing MT SMS */ (a comment is always about "code implementing") https://gerrit.osmocom.org/#/c/11920/11/src/libmsc/gsm_04_11.c File src/libmsc/gsm_04_11.c: https://gerrit.osmocom.org/#/c/11920/11/src/libmsc/gsm_04_11.c@162 PS11, Line 162: if (sms != NULL) { /* gsm411_send_rp_data() doesn't set trans->sms.sms */ put the "doesn't set" above the 'if', and the 'Notify...' inside the 'if {' body https://gerrit.osmocom.org/#/c/11920/11/src/libmsc/gsm_04_11.c@1147 PS11, Line 1147: hmm, I sort of expected a TLV structure, but there are no Ts... too late for that I guess. https://gerrit.osmocom.org/#/c/11920/11/src/libmsc/gsm_04_11.c@1148 PS11, Line 1148: /* Encode RP-DA length */ OA? https://gerrit.osmocom.org/#/c/11920/11/src/libmsc/gsm_04_11.c@1154 PS11, Line 1154: memcpy(ptr, sm_rp_oa, sm_rp_oa_len); Rather use msgb_lv_put()? https://gerrit.osmocom.org/#/c/11920/11/src/libmsc/gsm_04_11.c@1158 PS11, Line 1158: ptr[0] = 0x00; msgb_put_u8()? msgb_v_put()? https://gerrit.osmocom.org/#/c/11920/11/src/libmsc/gsm_04_11.c@1166 PS11, Line 1166: memcpy(ptr, sm_rp_ud, sm_rp_ud_len); msgb_lv_put()? https://gerrit.osmocom.org/#/c/11920/11/src/libmsc/gsm_04_11.c@1170 PS11, Line 1170: return gsm411_rp_sendmsg(&trans->sms.smr_inst, msg, API wise it would be better to have separate functions that encode and send. I often faced inflexibility with code that cannot encode without also sending, for example during unit testing. -- To view, visit https://gerrit.osmocom.org/11920 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I57357982ca0e51f6722c24a4aa1d0fb3e6caef88 Gerrit-Change-Number: 11920 Gerrit-PatchSet: 11 Gerrit-Owner: Vadim Yanitskiy <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Vadim Yanitskiy <[email protected]> Gerrit-CC: Stefan Sperling <[email protected]> Gerrit-Comment-Date: Sat, 05 Jan 2019 04:28:14 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
