[sr-dev] Inconsistent branch numbering between REQUEST_ROUTE and REPLY_ROUTE

[Alex Hermann]

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 at 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