Change in osmo-msc[master]: large refactoring: support inter-BSC and inter-MSC Handover
gerrit-no-reply at lists.osmocom.org
Wed May 8 10:13:38 UTC 2019
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:
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.
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.
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);
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.
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().
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-Owner: Neels Hofmeyr <nhofmeyr 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: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-CC: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Comment-Date: Wed, 08 May 2019 10:13:38 +0000
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the gerrit-log