Change in libosmo-sccp[master]: fix ipa_asp_fsm down state transition

[Stefan Sperling]

Stefan Sperling has posted comments on this change. ( https://gerrit.osmocom.org/12364 )

Change subject: fix ipa_asp_fsm down state transition
......................................................................


Patch Set 1:

(3 comments)

https://gerrit.osmocom.org/#/c/12364/1/src/xua_asp_fsm.c
File src/xua_asp_fsm.c:

https://gerrit.osmocom.org/#/c/12364/1/src/xua_asp_fsm.c@920
PS1, Line 920: {
> So the only use of fi is to get *priv? Than better just pass struct ipa_asp_fsm_priv *iafp as parame […]
Hmm, yes, good suggestion.


https://gerrit.osmocom.org/#/c/12364/1/src/xua_asp_fsm.c@939
PS1, Line 939: 	osmo_timer_del(&iafp->pong_timer);
> I don't understand how it works if we bail out before reaching here. […]
This timer is actually never scheduled as far as I can see. And indeed it is bogus that we won't delete it if returning early. But note that this bug is part of code I merely moved around, so I'd rather fix it in a separate patch.


https://gerrit.osmocom.org/#/c/12364/1/src/xua_asp_fsm.c@1060
PS1, Line 1060: 	ipa_asp_fsm_del_route(fi);
> I'd rather see ipa_asp_fsm_cleanup() moved in place of ipa_asp_fsm_del_route() instead of keeping it […]
In an earlier version of this patch I just called ipa_asp_fsm_cleanup() from ipa_asp_fsm_active().

What made me uncomfortable about that is that it looks like this cleanup handler is supposed to be invoked by the generic FSM code, not from within the FSM itself.
It just so happens that we need to run the same code when the FSM terminates and when we transition ACTIVE->DOWN.

I don't really care though because both approaches work. Would you prefer to call ipa_asp_fsm_cleanup() from ipa_asp_fsm_active()?



-- 
To view, visit https://gerrit.osmocom.org/12364
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmo-sccp
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb8e7bd1c74a4b47080fe32ebe0161c503ead571
Gerrit-Change-Number: 12364
Gerrit-PatchSet: 1
Gerrit-Owner: Stefan Sperling <stsp at stsp.name>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Stefan Sperling <stsp at stsp.name>
Gerrit-Comment-Date: Thu, 20 Dec 2018 11:42:45 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181220/26946a0d/attachment.htm>