Change in libosmo-sccp[master]: fix ipa_asp_fsm down state transition
Harald Welte has submitted this change and it was merged. (
https://gerrit.osmocom.org/12364 )
Change subject: fix ipa_asp_fsm down state transition
..
fix ipa_asp_fsm down state transition
Properly transition into IPA_ASP_S_DOWN from IPA_ASP_S_ACTIVE
and fix the mask of legal out states from IPA_ASP_S_ACTIVE.
BSC-sccplite tests are still passing with this change.
Change-Id: Idb8e7bd1c74a4b47080fe32ebe0161c503ead571
Related: OS#3111
---
M src/xua_asp_fsm.c
1 file changed, 27 insertions(+), 22 deletions(-)
Approvals:
Jenkins Builder: Verified
Max: Looks good to me, but someone else must approve
Neels Hofmeyr: Looks good to me, approved
diff --git a/src/xua_asp_fsm.c b/src/xua_asp_fsm.c
index d6edac0..9dc9a82 100644
--- a/src/xua_asp_fsm.c
+++ b/src/xua_asp_fsm.c
@@ -916,6 +916,29 @@
}
}
+static void ipa_asp_fsm_del_route(struct ipa_asp_fsm_priv *iafp)
+{
+ struct osmo_ss7_asp *asp = iafp->asp;
+ struct osmo_ss7_instance *inst = asp->inst;
+ struct osmo_ss7_as *as = osmo_ss7_as_find_by_rctx(inst, 0);
+ struct osmo_ss7_route *rt;
+
+ OSMO_ASSERT(as);
+
+ /* find the route which we have created if we ever reached
ipa_asp_fsm_wait_id_ack2 */
+ rt = osmo_ss7_route_find_dpc_mask(inst->rtable_system,
as->cfg.routing_key.pc, 0xff);
+ /* no route found, bail out */
+ if (!rt)
+ return;
+ /* route points to different AS, bail out */
+ if (rt->dest.as != as)
+ return;
+
+ osmo_ss7_route_destroy(rt);
+ /* FIXME: Why don't we also delete this timer if we return early above?
+* FIXME: Where is this timer even scheduled? */
+ osmo_timer_del(>pong_timer);
+}
/* Server + Client: We're actively transmitting user data */
static void ipa_asp_fsm_active(struct osmo_fsm_inst *fi, uint32_t event, void
*data)
@@ -923,7 +946,8 @@
switch (event) {
case XUA_ASP_E_M_ASP_DOWN_REQ:
case XUA_ASP_E_M_ASP_INACTIVE_REQ:
- /* FIXME: kill ASP and (wait for) re-connect */
+ ipa_asp_fsm_del_route(fi->priv);
+ osmo_fsm_inst_state_chg(fi, IPA_ASP_S_DOWN, 0, 0);
break;
}
}
@@ -1025,8 +1049,7 @@
[IPA_ASP_S_ACTIVE] = {
.in_event_mask = S(XUA_ASP_E_M_ASP_DOWN_REQ) |
S(XUA_ASP_E_M_ASP_INACTIVE_REQ),
- .out_state_mask = S(XUA_ASP_S_INACTIVE) |
- S(XUA_ASP_S_DOWN),
+ .out_state_mask = S(IPA_ASP_S_DOWN),
.name = "ASP_ACTIVE",
.action = ipa_asp_fsm_active,
.onenter = ipa_asp_fsm_active_onenter,
@@ -1035,25 +1058,7 @@
static void ipa_asp_fsm_cleanup(struct osmo_fsm_inst *fi, enum
osmo_fsm_term_cause cause)
{
- struct ipa_asp_fsm_priv *iafp = fi->priv;
- struct osmo_ss7_asp *asp = iafp->asp;
- struct osmo_ss7_instance *inst = asp->inst;
- struct osmo_ss7_as *as = osmo_ss7_as_find_by_rctx(inst, 0);
- struct osmo_ss7_route *rt;
-
- OSMO_ASSERT(as);
-
- /* find the route which we have created if we ever reached
ipa_asp_fsm_wait_id_ack2 */
- rt = osmo_ss7_route_find_dpc_mask(inst->rtable_system,
as->cfg.routing_key.pc, 0xff);
- /* no route found, bail out */
- if (!rt)
- return;
- /* route points to different AS, bail out */
- if (rt->dest.as != as)
- return;
-
- osmo_ss7_route_destroy(rt);
- osmo_timer_del(>pong_timer);
+ ipa_asp_fsm_del_route(fi->priv);
}
struct osmo_fsm ipa_asp_fsm = {
--
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: merged
Gerrit-Change-Id: Idb8e7bd1c74a4b47080fe32ebe0161c503ead571
Gerrit-Change-Number: 12364
Gerrit-PatchSet: 2
Gerrit-Owner: Stefan Sperling
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-Reviewer: Stefan Sperling
Change in libosmo-sccp[master]: fix ipa_asp_fsm down state transition
Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12364 ) Change subject: fix ipa_asp_fsm down state transition .. Patch Set 2: Code-Review+2 -- 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: 2 Gerrit-Owner: Stefan Sperling Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Stefan Sperling Gerrit-Comment-Date: Fri, 21 Dec 2018 02:18:25 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in libosmo-sccp[master]: fix ipa_asp_fsm down state transition
Max has posted comments on this change. ( https://gerrit.osmocom.org/12364 ) Change subject: fix ipa_asp_fsm down state transition .. Patch Set 2: Code-Review+1 -- 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: 2 Gerrit-Owner: Stefan Sperling Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Stefan Sperling Gerrit-Comment-Date: Thu, 20 Dec 2018 15:45:29 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in libosmo-sccp[master]: fix ipa_asp_fsm down state transition
Hello Max, Neels Hofmeyr, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/12364 to look at the new patch set (#2). Change subject: fix ipa_asp_fsm down state transition .. fix ipa_asp_fsm down state transition Properly transition into IPA_ASP_S_DOWN from IPA_ASP_S_ACTIVE and fix the mask of legal out states from IPA_ASP_S_ACTIVE. BSC-sccplite tests are still passing with this change. Change-Id: Idb8e7bd1c74a4b47080fe32ebe0161c503ead571 Related: OS#3111 --- M src/xua_asp_fsm.c 1 file changed, 27 insertions(+), 22 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmo-sccp refs/changes/64/12364/2 -- 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: newpatchset Gerrit-Change-Id: Idb8e7bd1c74a4b47080fe32ebe0161c503ead571 Gerrit-Change-Number: 12364 Gerrit-PatchSet: 2 Gerrit-Owner: Stefan Sperling Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Stefan Sperling
Change in libosmo-sccp[master]: fix ipa_asp_fsm down state transition
Max has posted comments on this change. ( https://gerrit.osmocom.org/12364 ) Change subject: fix ipa_asp_fsm down state transition .. Patch Set 1: (2 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@939 PS1, Line 939: osmo_timer_del(>pong_timer); > This timer is actually never scheduled as far as I can see. […] Sure, simple /* FIXME: ... */ will do as well. https://gerrit.osmocom.org/#/c/12364/1/src/xua_asp_fsm.c@1060 PS1, Line 1060: ipa_asp_fsm_del_route(fi); > In an earlier version of this patch I just called ipa_asp_fsm_cleanup() from > ipa_asp_fsm_active(). […] Ah, now I see. Than your variant with separate function and wrapper is better indeed. I still would prefer not to pass fi to ipa_asp_fsm_del_route() to make the distinction between fsm-specific part and generic cleanup more obvious. -- 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 Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Stefan Sperling Gerrit-Comment-Date: Thu, 20 Dec 2018 12:01:54 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in libosmo-sccp[master]: fix ipa_asp_fsm down state transition
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(>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
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-Reviewer: Stefan Sperling
Gerrit-Comment-Date: Thu, 20 Dec 2018 11:42:45 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
Change in libosmo-sccp[master]: fix ipa_asp_fsm down state transition
Max has posted comments on this change. ( https://gerrit.osmocom.org/12364 )
Change subject: fix ipa_asp_fsm down state transition
..
Patch Set 1: Code-Review-1
(3 comments)
Being unfamiliar with the code helps to highlight things which makes review
difficult :)
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 parameter and let caller take care of fi-priv. This
function have nothing to do with osmo_fsm_inst so it's better if this is
immediately obvious when you look at type signature, which you're effectively
changing compared to ipa_asp_fsm_cleanup() anyway.
https://gerrit.osmocom.org/#/c/12364/1/src/xua_asp_fsm.c@939
PS1, Line 939: osmo_timer_del(>pong_timer);
I don't understand how it works if we bail out before reaching here. Does
pong_timer kept forever? Or it's removed by some other code? Please
double-check and clarify in a comment or commit message.
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 as this wrapper. If we ever have to change
ipa_asp_fsm_del_route() in a way which requires updating its callees than this
wrapper is unnecessary indirection which makes it less trivial than it should
be. Even in this commit you've de-facto dropped unused cause parameter but
because of this indirection it's unclear which parts of the code are affected.
--
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
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-Comment-Date: Thu, 20 Dec 2018 11:28:39 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
Change in libosmo-sccp[master]: fix ipa_asp_fsm down state transition
Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12364 ) Change subject: fix ipa_asp_fsm down state transition .. Patch Set 1: Code-Review+1 -- 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 Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Comment-Date: Wed, 19 Dec 2018 22:46:35 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in libosmo-sccp[master]: fix ipa_asp_fsm down state transition
Stefan Sperling has uploaded this change for review. (
https://gerrit.osmocom.org/12364
Change subject: fix ipa_asp_fsm down state transition
..
fix ipa_asp_fsm down state transition
Properly transition into IPA_ASP_S_DOWN from IPA_ASP_S_ACTIVE
and fix the mask of legal out states from IPA_ASP_S_ACTIVE.
BSC-sccplite tests are still passing with this change.
Change-Id: Idb8e7bd1c74a4b47080fe32ebe0161c503ead571
Related: OS#3111
---
M src/xua_asp_fsm.c
1 file changed, 26 insertions(+), 22 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmo-sccp refs/changes/64/12364/1
diff --git a/src/xua_asp_fsm.c b/src/xua_asp_fsm.c
index d6edac0..b0c672b 100644
--- a/src/xua_asp_fsm.c
+++ b/src/xua_asp_fsm.c
@@ -916,6 +916,28 @@
}
}
+static void ipa_asp_fsm_del_route(struct osmo_fsm_inst *fi)
+{
+ struct ipa_asp_fsm_priv *iafp = fi->priv;
+ struct osmo_ss7_asp *asp = iafp->asp;
+ struct osmo_ss7_instance *inst = asp->inst;
+ struct osmo_ss7_as *as = osmo_ss7_as_find_by_rctx(inst, 0);
+ struct osmo_ss7_route *rt;
+
+ OSMO_ASSERT(as);
+
+ /* find the route which we have created if we ever reached
ipa_asp_fsm_wait_id_ack2 */
+ rt = osmo_ss7_route_find_dpc_mask(inst->rtable_system,
as->cfg.routing_key.pc, 0xff);
+ /* no route found, bail out */
+ if (!rt)
+ return;
+ /* route points to different AS, bail out */
+ if (rt->dest.as != as)
+ return;
+
+ osmo_ss7_route_destroy(rt);
+ osmo_timer_del(>pong_timer);
+}
/* Server + Client: We're actively transmitting user data */
static void ipa_asp_fsm_active(struct osmo_fsm_inst *fi, uint32_t event, void
*data)
@@ -923,7 +945,8 @@
switch (event) {
case XUA_ASP_E_M_ASP_DOWN_REQ:
case XUA_ASP_E_M_ASP_INACTIVE_REQ:
- /* FIXME: kill ASP and (wait for) re-connect */
+ ipa_asp_fsm_del_route(fi);
+ osmo_fsm_inst_state_chg(fi, IPA_ASP_S_DOWN, 0, 0);
break;
}
}
@@ -1025,8 +1048,7 @@
[IPA_ASP_S_ACTIVE] = {
.in_event_mask = S(XUA_ASP_E_M_ASP_DOWN_REQ) |
S(XUA_ASP_E_M_ASP_INACTIVE_REQ),
- .out_state_mask = S(XUA_ASP_S_INACTIVE) |
- S(XUA_ASP_S_DOWN),
+ .out_state_mask = S(IPA_ASP_S_DOWN),
.name = "ASP_ACTIVE",
.action = ipa_asp_fsm_active,
.onenter = ipa_asp_fsm_active_onenter,
@@ -1035,25 +1057,7 @@
static void ipa_asp_fsm_cleanup(struct osmo_fsm_inst *fi, enum
osmo_fsm_term_cause cause)
{
- struct ipa_asp_fsm_priv *iafp = fi->priv;
- struct osmo_ss7_asp *asp = iafp->asp;
- struct osmo_ss7_instance *inst = asp->inst;
- struct osmo_ss7_as *as = osmo_ss7_as_find_by_rctx(inst, 0);
- struct osmo_ss7_route *rt;
-
- OSMO_ASSERT(as);
-
- /* find the route which we have created if we ever reached
ipa_asp_fsm_wait_id_ack2 */
- rt = osmo_ss7_route_find_dpc_mask(inst->rtable_system,
as->cfg.routing_key.pc, 0xff);
- /* no route found, bail out */
- if (!rt)
- return;
- /* route points to different AS, bail out */
- if (rt->dest.as != as)
- return;
-
- osmo_ss7_route_destroy(rt);
- osmo_timer_del(>pong_timer);
+ ipa_asp_fsm_del_route(fi);
}
struct osmo_fsm ipa_asp_fsm = {
--
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: newchange
Gerrit-Change-Id: Idb8e7bd1c74a4b47080fe32ebe0161c503ead571
Gerrit-Change-Number: 12364
Gerrit-PatchSet: 1
Gerrit-Owner: Stefan Sperling
