11 Oct
2012
11 Oct
'12
3:19 p.m.
Hello,
i found that $T_barnch_idx returns inconsistent numbers for the same branch.
If printed in REQUEST_ROUTE, the value is alwasy 1 more that in REPLY_ROUTE.
The branch index is set using tm_ctx_set_branch_index(branch) in a lot of
places, but only in t_fwd.c it is set with tm_ctx_set_branch_index(branch+1).
If i change that to the same as elsewhere, the numbering is consistent. I'd
like to get an ACK from a tm guru before i commit this fix, because i have
no idea what side-effects this might have.
--- a/modules/tm/t_fwd.c
+++ b/modules/tm/t_fwd.c
@@ -317,7 +317,7 @@ static int prepare_new_uac( struct cell *t, struct sip_msg *i_req,
/* run branch_route actions if provided */
backup_route_type = get_route_type();
set_route_type(BRANCH_ROUTE);
- tm_ctx_set_branch_index(branch+1);
+ tm_ctx_set_branch_index(branch);
/* no need to backup/set avp lists: the on_branch route is run
only in the main route context (e.g. t_relay() in the main
route) or in the failure route context (e.g. append_branch
&
--
Greetings,
Alex Hermann
11 Oct
11 Oct
11:26 p.m.
New subject: Inconsistent branch numbering between REQUEST_ROUTE and REPLY_ROUTE
Hello,
On 10/11/12 2:19 PM, Alex Hermann wrote:
Hello,
i found that $T_barnch_idx returns inconsistent numbers for the same branch.
If printed in REQUEST_ROUTE, the value is alwasy 1 more that in REPLY_ROUTE.
do you actually mean BRANCH_ROUTE instead of REQUEST_ROUTE?
The branch index is set using
tm_ctx_set_branch_index(branch) in a lot of
places, but only in t_fwd.c it is set with tm_ctx_set_branch_index(branch+1).
If i change that to the same as elsewhere, the numbering is consistent. I'd
like to get an ACK from a tm guru before i commit this fix, because i have
no idea what side-effects this might have.
Indeed it should be same value. Probably when was added first time for
branch_route was more like:
- if 0, then is like unset or before transaction is created
- if >0, then is branch index + 1
To bring proper coherence, I see two options:
- keep 0 for unset and return branch index + 1 otherwise
- use -1 for unset and return branch index otherwise
In either way, the patch has to updated.
Cheers,
Daniel
--- a/modules/tm/t_fwd.c
+++ b/modules/tm/t_fwd.c
@@ -317,7 +317,7 @@ static int prepare_new_uac( struct cell *t, struct sip_msg *i_req,
/* run branch_route actions if provided */
backup_route_type = get_route_type();
set_route_type(BRANCH_ROUTE);
- tm_ctx_set_branch_index(branch+1);
+ tm_ctx_set_branch_index(branch);
/* no need to backup/set avp lists: the on_branch route is run
only in the main route context (e.g. t_relay() in the main
route) or in the failure route context (e.g. append_branch
&
--
Daniel-Constantin Mierla - http://www.asipto.com
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda
Kamailio Advanced Training, Berlin, Nov 5-8, 2012 - http://asipto.com/u/kat
Kamailio Advanced Training, Miami, USA, Nov 12-14, 2012 - http://asipto.com/u/katu
12 Oct
12 Oct
5:04 p.m.
New subject: Inconsistent branch numbering between REQUEST_ROUTE and REPLY_ROUTE
On Thursday 11 October 2012, Daniel-Constantin Mierla wrote:
I prefer the latter option because it corresponds to the internal state.
I think this eases debugging and avoids mistakes. In this way the T_branch_idx
is also the same as the last digit(s) in the added Via header.
On 10/11/12 2:19 PM, Alex Hermann wrote:
Yes, and for completeness, i also meant TM_ONREPLY_ROUTE instead of REPLY_ROUTE.
i found that $T_barnch_idx returns inconsistent
numbers for the same
branch. If printed in REQUEST_ROUTE, the value is alwasy 1 more that in
REPLY_ROUTE.
do you actually mean BRANCH_ROUTE instead of REQUEST_ROUTE? The branch
index is set using tm_ctx_set_branch_index(branch) in a lot of
places, but only in t_fwd.c it is set with
tm_ctx_set_branch_index(branch+1).
If i change that to the same as elsewhere, the numbering is consistent.
I'd like to get an ACK from a tm guru before i commit this fix, because
i have no idea what side-effects this might have.
Indeed it should be same value. Probably when was added first time for
branch_route was more like:
- if 0, then is like unset or before transaction is created
- if >0, then is branch index + 1
To bring proper coherence, I see two options:
- keep 0 for unset and return branch index + 1 otherwise
- use -1 for unset and return branch index otherwise In either way, the patch has to updated.
No
problem. Additional patch:
commit 5c44d5f5d91f84757586d2af44c9ac016ec9fa96
Author: Alex Hermann <alex(a)speakup.nl>
Date: Fri Oct 12 14:06:33 2012 +0200
modules/tm: Set branch_index to T_BR_UNDEFINED when outside BRANCH_ROUTE or
TM_ONREPLY_ROUTE.
The inconsistent value of $T_branch_idx between BRANCH_ROUTE and
TM_ON_REPLY_ROUTE was fixed in an earlier commit, but now the value 0 has a
double meaning (branch 0 or invalid branch). This patch makes the invalid
branch distinguishable by setting it to -1.
Now $T_branch_idx will return the branch number (0-based) in BRANCH_ROUTE
and TM_ON_REPLY_ROUTE and -1 in other route types or if the message is not
part of a transaction.
diff --git a/modules/tm/t_fwd.c b/modules/tm/t_fwd.c
index 6f8661d..90b36b8 100644
--- a/modules/tm/t_fwd.c
+++ b/modules/tm/t_fwd.c
@@ -351,14 +351,14 @@ static int prepare_new_uac( struct cell *t, struct sip_msg *i_req,
/* if DROP was called in cfg, don't forward, jump to
end */
if (unlikely(ctx.run_flags&DROP_R_F))
{
- tm_ctx_set_branch_index(0);
+ tm_ctx_set_branch_index(T_BR_UNDEFINED);
set_route_type(backup_route_type);
/* triggered by drop in CFG */
ret=E_CFG;
goto error03;
}
}
- tm_ctx_set_branch_index(0);
+ tm_ctx_set_branch_index(T_BR_UNDEFINED);
set_route_type(backup_route_type);
}
diff --git a/modules/tm/t_lookup.c b/modules/tm/t_lookup.c
index c070948..a89744e 100644
--- a/modules/tm/t_lookup.c
+++ b/modules/tm/t_lookup.c
@@ -1970,6 +1970,7 @@ tm_ctx_t* tm_ctx_get(void)
void tm_ctx_init(void)
{
memset(&_tm_ctx, 0, sizeof(tm_ctx_t));
+ _tm_ctx.branch_index = T_BR_UNDEFINED;
}
void tm_ctx_set_branch_index(int v)
diff --git a/modules/tm/t_reply.c b/modules/tm/t_reply.c
index e210452..ce1dccb 100644
--- a/modules/tm/t_reply.c
+++ b/modules/tm/t_reply.c
@@ -2363,7 +2363,7 @@ int reply_received( struct sip_msg *p_msg )
} /* provisional replies */
done:
- tm_ctx_set_branch_index(0);
+ tm_ctx_set_branch_index(T_BR_UNDEFINED);
/* we are done with the transaction, so unref it - the reference
* was incremented by t_check() function -bogdan*/
t_unref(p_msg);
diff --git a/modules/tm/tm.c b/modules/tm/tm.c
index fcfa328..8352f3f 100644
--- a/modules/tm/tm.c
+++ b/modules/tm/tm.c
@@ -847,6 +847,10 @@ static int mod_init(void)
#endif /* WITH_EVENT_LOCAL_REQUEST */
if (goto_on_sl_reply && onreply_rt.rlist[goto_on_sl_reply]==0)
WARN("empty/non existing on_sl_reply route\n");
+
+#ifdef WITH_TM_CTX
+ tm_ctx_init();
+#endif
tm_init = 1;
return 0;
}
diff --git a/modules_k/tmx/t_var.c b/modules_k/tmx/t_var.c
index b89d729..7a0c159 100644
--- a/modules_k/tmx/t_var.c
+++ b/modules_k/tmx/t_var.c
@@ -379,7 +379,7 @@ int pv_get_tm_branch_idx(struct sip_msg *msg, pv_param_t *param,
if(tcx != NULL)
idx = tcx->branch_index;
- ch = int2str(idx, &l);
+ ch = sint2str(idx, &l);
res->rs.s = ch;
res->rs.len = l;
--
Met vriendelijke groet,
Alex Hermann
SpeakUp BV
T: 088-SPEAKUP (088-7732587)
F: 088-7732588
4424
days inactive
4425
days old
2 comments
2 participants
participants (2)
-
Alex Hermann -
Daniel-Constantin Mierla