Vadim Yanitskiy has posted comments on this change. ( https://gerrit.osmocom.org/13137 )
Change subject: large refactoring: support inter-BSC and inter-MSC Handover ...................................................................... Patch Set 18: (5 comments) Since it was decided to merge this bomb 'as-is', let's move it forward. Please consider my comments as the points for further improvements and follow-up patches, not as merge-blockers. https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c File src/libmsc/call_leg.c: https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@78 PS9, Line 78: talloc_zero > The talloc API does not provide a macro similar to talloc_zero() that names > the struct [...] Of course it does, see talloc(). talloc_zero() is just a wrapper around talloc() + memset(). > What is the reason for this statement, optimization? Exactly. I'm not sure if compiler can optimize out such double zero-initialization. https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@158 PS9, Line 158: ase CALL_LEG > it is to clarify the type of the data argument [...] osmo_fsm_inst_dispatch(fi->proc.parent, cl->parent_event_rtp_addr_available, (struct rtp_stream *) rtps); https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@178 PS9, Line 178: Also same opinion, variable is not needed for that: osmo_fsm_inst_dispatch(fi->proc.parent, cl->parent_event_rtp_complete, (struct call_leg *) cl); > prefer to keep for some degree of "type safety" TBH, I dont't see any "type safety" here. This just indicates the original type of the pointer to code readers. https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@290 PS9, Line 290: > the critical difference is that it calls call_leg_rtp_alloc() only if it > hasn't been called yet on t […] Well, call_leg_rtp_alloc() also would just return 0 if it was already called for a given RTP stream. I still think that this function should be merged into call_leg_rtp_alloc(). https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/gsm_04_08.c File src/libmsc/gsm_04_08.c: https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/gsm_04_08.c@156 PS9, Line 156: DEBUGP > This is legacy logging, not sure if we really need to change all of it now? You're already changing some DEBUGP / LOGP statements to LOG_MSC_A_CAT() in this patch. I would rather change all of them and don't leave this work to somebody else... -- To view, visit https://gerrit.osmocom.org/13137 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: I27e4988e0371808b512c757d2b52ada1615067bd Gerrit-Change-Number: 13137 Gerrit-PatchSet: 18 Gerrit-Owner: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Vadim Yanitskiy <[email protected]> Gerrit-CC: Pau Espin Pedrol <[email protected]> Gerrit-Comment-Date: Wed, 08 May 2019 10:13:38 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
