Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/11734 )
Change subject: handover_fsm: send HANDOVER PERFORMED msg on internal ho ...................................................................... Patch Set 4: Code-Review-1 (7 comments) I found a few more things, some less important than others... https://gerrit.osmocom.org/#/c/11734/4/src/osmo-bsc/handover_fsm.c File src/osmo-bsc/handover_fsm.c: https://gerrit.osmocom.org/#/c/11734/4/src/osmo-bsc/handover_fsm.c@678 PS4, Line 678: struct gsm0808_handover_performed ho_perf_params; could just write '= {};' instead of the memset below https://gerrit.osmocom.org/#/c/11734/4/src/osmo-bsc/handover_fsm.c@680 PS4, Line 680: struct gsm0808_speech_codec sc; maybe also = {} here? https://gerrit.osmocom.org/#/c/11734/4/src/osmo-bsc/handover_fsm.c@689 PS4, Line 689: if (!cell) no error message or anything? Please do some LOG_HO(conn, LOGL_ERROR, "...") https://gerrit.osmocom.org/#/c/11734/4/src/osmo-bsc/handover_fsm.c@692 PS4, Line 692: ho_perf_params.cell.id_discr = CELL_IDENT_WHOLE_GLOBAL; Please use the proper union member, here id.global. I prefer this notation: ho_perf_params.cell = (struct gsm0808_cell_id){ .id_discr = CELL_IDENT_WHOLE_GLOBAL, .id.global = *cell, } because then you're always sure all unset items become zero. (Also, I think a new version of cgi_for_msc() version should return a struct gsm0808_cell_id directly, again a note to self to fix it.) https://gerrit.osmocom.org/#/c/11734/4/src/osmo-bsc/handover_fsm.c@696 PS4, Line 696: if (!ho_perf_params.chosen_channel) LOG_HO(..,ERROR https://gerrit.osmocom.org/#/c/11734/4/src/osmo-bsc/handover_fsm.c@719 PS4, Line 719: msg = gsm0808_create_handover_performed(&ho_perf_params); check msg for NULL and log error? https://gerrit.osmocom.org/#/c/11734/4/src/osmo-bsc/handover_fsm.c@720 PS4, Line 720: gscon_sigtran_send(conn, msg); check rc and log error? -- To view, visit https://gerrit.osmocom.org/11734 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If26e5807280e0f75a423b3b04f8e3c698c82a351 Gerrit-Change-Number: 11734 Gerrit-PatchSet: 4 Gerrit-Owner: dexter <pma...@sysmocom.de> Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: dexter <pma...@sysmocom.de> Gerrit-CC: Pau Espin Pedrol <pes...@sysmocom.de> Gerrit-Comment-Date: Tue, 27 Nov 2018 01:03:18 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
