Change in osmo-msc[master]: large refactoring: support inter-BSC and inter-MSC Handover

Vadim Yanitskiy 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:

(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 <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
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190508/bb55fd9e/attachment.html>


More information about the gerrit-log mailing list