On Thursday 11 October 2012, Daniel-Constantin Mierla wrote:
On 10/11/12 2:19 PM, Alex Hermann wrote:
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?
Yes, and for completeness, i also meant TM_ONREPLY_ROUTE instead of 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.
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
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.
In either way, the patch has to updated.
No problem. Additional patch:
commit 5c44d5f5d91f84757586d2af44c9ac016ec9fa96 Author: Alex Hermann alex@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;