Change in osmo-bsc[master]: handover_fsm: send HANDOVER PERFORMED msg on internal ho

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/.

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Tue Nov 27 01:03:18 UTC 2018


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 <pmaier at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-CC: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Comment-Date: Tue, 27 Nov 2018 01:03:18 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181127/39164ef3/attachment.htm>


More information about the gerrit-log mailing list