[sr-dev] git:master: tm: t_suspend() fixes

Module: sip-router Branch: master Commit: 9431f814d8127a7c6bb82593a75eb6d97fc0d7f6 URL: http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=9431f81… Author: Miklos Tirpak &lt;miklos(a)iptel.org&gt; Committer: Miklos Tirpak &lt;miklos(a)iptel.org&gt; Date: Wed Nov 11 15:21:23 2009 +0100 tm: t_suspend() fixes - t_suspend() and t_continue() checks whether a CANCEL has been processed before the transaction is suspended and before it continues to make sure that the suspend/continue is not done needlessly. - t_continue() verifies that the suspended branch has not timed out yet. - t_cancel_suspend() function is introduced: It can be used for revoking the suspension of the transaction. Useful when any failure occures after the transaction has been suspended and it turns out that the transaction should not have been suspended. --- modules/tm/doc/api.xml | 30 ++++++++++- modules/tm/t_suspend.c | 133 +++++++++++++++++++++++++++++++++++++++++------- modules/tm/t_suspend.h | 4 ++ modules/tm/tm.c | 1 + modules/tm/tm_load.c | 5 ++ modules/tm/tm_load.h | 1 + 6 files changed, 154 insertions(+), 20 deletions(-) diff --git a/modules/tm/doc/api.xml b/modules/tm/doc/api.xml index 51c511c..ee6e03f 100644 --- a/modules/tm/doc/api.xml +++ b/modules/tm/doc/api.xml @@ -239,6 +239,34 @@ end of body </itemizedlist> <para>Return value: 0 - success, &lt;0 - error.</para> </section> - </section> + <section id="t_cancel_suspend"> + <title> + <function>int t_cancel_suspend(unsigned int hash_index, unsigned int label)</function> + </title> + <para> + For programmatic use only. + This function is for revoking t_suspend() from the + same process as it was executed before. t_cancel_suspend() can be + used when something fails after t_suspend() has already been executed + and it turns out that the transcation should not have been + suspended. The function cancels the FR timer of the transacation. + </para> + <para> + The message lumps are saved by t_suspend() which cannot be restored. + </para> + <para>Meaning of the parameters is as follows:</para> + <itemizedlist> + <listitem> + <para><emphasis>hash_index</emphasis> - transaction identifier. + </para> + </listitem> + <listitem> + <para><emphasis>label</emphasis> - transaction identifier. + </para> + </listitem> + </itemizedlist> + <para>Return value: 0 - success, &lt;0 - error.</para> + </section> + </section> </section> diff --git a/modules/tm/t_suspend.c b/modules/tm/t_suspend.c index f6a57d0..19c7f32 100644 --- a/modules/tm/t_suspend.c +++ b/modules/tm/t_suspend.c @@ -64,6 +64,14 @@ int t_suspend(struct sip_msg *msg, return -1; } + if (t->flags & T_CANCELED) { + /* The transaction has already been canceled */ + LOG(L_DBG, "DEBUG: t_suspend: " \ + "trying to suspend an already canceled transaction\n"); + ser_error = E_CANCELED; + return 1; + } + /* send a 100 Trying reply, because the INVITE processing will probably take a long time */ if (msg->REQ_METHOD==METHOD_INVITE && (t->flags&T_AUTO_INV_100) @@ -116,12 +124,20 @@ int t_continue(unsigned int hash_index, unsigned int label, struct sip_msg faked_req; int branch; struct ua_client *uac =NULL; + int ret; if (t_lookup_ident(&t, hash_index, label) < 0) { LOG(L_ERR, "ERROR: t_continue: transaction not found\n"); return -1; } + if (t->flags & T_CANCELED) { + /* The transaction has already been canceled, + * needless to continue */ + UNREF(t); /* t_unref would kill the transaction */ + return 1; + } + /* The transaction has to be locked to protect it * form calling t_continue() multiple times simultaneously */ LOCK_REPLIES(t); @@ -134,13 +150,23 @@ int t_continue(unsigned int hash_index, unsigned int label, if (branch >= 0) { stop_rb_timers(&t->uac[branch].request); + + if (t->uac[branch].last_received != 0) { + /* Either t_continue() has already been + * called or the branch has already timed out. + * Needless to continue. */ + UNLOCK_REPLIES(t); + UNREF(t); /* t_unref would kill the transaction */ + return 1; + } + /* Set last_received to something >= 200, * the actual value does not matter, the branch * will never be picked up for response forwarding. * If last_received is lower than 200, * then the branch may tried to be cancelled later, * for example when t_reply() is called from - * a failure rute => deadlock, because both + * a failure route => deadlock, because both * of them need the reply lock to be held. */ t->uac[branch].last_received=500; uac = &t->uac[branch]; @@ -154,8 +180,8 @@ int t_continue(unsigned int hash_index, unsigned int label, /* fake the request and the environment, like in failure_route */ if (!fake_req(&faked_req, t->uas.request, 0 /* extra flags */, uac)) { LOG(L_ERR, "ERROR: t_continue: fake_req failed\n"); - UNLOCK_REPLIES(t); - return -1; + ret = -1; + goto kill_trans; } faked_env( t, &faked_req); @@ -168,6 +194,7 @@ int t_continue(unsigned int hash_index, unsigned int label, LOG(L_ERR, "ERROR: t_continue: Error in run_top_route\n"); exec_post_script_cb(&faked_req, FAILURE_CB_TYPE); } + /* TODO: save_msg_lumps should clone the lumps to shm mem */ /* restore original environment and free the fake msg */ @@ -193,22 +220,9 @@ int t_continue(unsigned int hash_index, unsigned int label, if (branch == t->nr_of_outgoings) { /* There is not any open branch so there is - * no chance that a final response will be received. - * The script has hopefully set the error code. If not, - * let us reply with a default error. - */ - if ((kill_transaction_unsafe(t, - tm_error ? tm_error : E_UNSPEC)) <=0 - ) { - LOG(L_ERR, "ERROR: t_continue: " - "reply generation failed\n"); - /* The transaction must be explicitely released, - no more timer is running */ - UNLOCK_REPLIES(t); - t_release_transaction(t); - t_unref(t->uas.request); - return 0; - } + * no chance that a final response will be received. */ + ret = 0; + goto kill_trans; } } @@ -218,4 +232,85 @@ int t_continue(unsigned int hash_index, unsigned int label, t_unref(t->uas.request); return 0; + +kill_trans: + /* The script has hopefully set the error code. If not, + * let us reply with a default error. */ + if ((kill_transaction_unsafe(t, + tm_error ? tm_error : E_UNSPEC)) <=0 + ) { + LOG(L_ERR, "ERROR: t_continue: " + "reply generation failed\n"); + /* The transaction must be explicitely released, + * no more timer is running */ + UNLOCK_REPLIES(t); + t_release_transaction(t); + } else { + UNLOCK_REPLIES(t); + } + + t_unref(t->uas.request); + return ret; +} + +/* Revoke the suspension of the SIP request, i.e. + * cancel the fr timer of the blind uac. + * This function can be called when something fails + * after t_suspend() has already been executed in the same + * process, and it turns out that the transaction should + * not have been suspended. + * + * Return value: + * 0 - success + * <0 - failure + */ +int t_cancel_suspend(unsigned int hash_index, unsigned int label) +{ + struct cell *t; + int branch; + + t = get_t(); + if (!t || t == T_UNDEFINED) { + LOG(L_ERR, "ERROR: t_revoke_suspend: " \ + "no active transaction\n"); + return -1; + } + /* Only to double-check the IDs */ + if ((t->hash_index != hash_index) + || (t->label != label) + ) { + LOG(L_ERR, "ERROR: t_revoke_suspend: " \ + "transaction id mismatch\n"); + return -1; + } + /* The transaction does not need to be locked because this + * function is either executed from the original route block + * or from failure route which already locks */ + + reset_kr(); /* the blind UAC of t_suspend has set kr */ + + /* Try to find the blind UAC, and cancel its fr timer. + * We assume that the last blind uac called this function. */ + for ( branch = t->nr_of_outgoings-1; + branch >= 0 && t->uac[branch].request.buffer; + branch--); + + if (branch >= 0) { + stop_rb_timers(&t->uac[branch].request); + /* Set last_received to something >= 200, + * the actual value does not matter, the branch + * will never be picked up for response forwarding. + * If last_received is lower than 200, + * then the branch may tried to be cancelled later, + * for example when t_reply() is called from + * a failure rute => deadlock, because both + * of them need the reply lock to be held. */ + t->uac[branch].last_received=500; + } else { + /* Not a huge problem, fr timer will fire, but CANCEL + will not be sent. last_received will be set to 408. */ + return -1; + } + + return 0; } diff --git a/modules/tm/t_suspend.h b/modules/tm/t_suspend.h index 8a9df8b..1c19be1 100644 --- a/modules/tm/t_suspend.h +++ b/modules/tm/t_suspend.h @@ -39,4 +39,8 @@ int t_continue(unsigned int hash_index, unsigned int label, typedef int (*t_continue_f)(unsigned int hash_index, unsigned int label, struct action *route); +int t_cancel_suspend(unsigned int hash_index, unsigned int label); +typedef int (*t_cancel_suspend_f)(unsigned int hash_index, unsigned int label); + + #endif /* _T_SUSPEND_H */ diff --git a/modules/tm/tm.c b/modules/tm/tm.c index 834517e..bd1b54a 100644 --- a/modules/tm/tm.c +++ b/modules/tm/tm.c @@ -488,6 +488,7 @@ static cmd_export_t cmds[]={ #endif {"t_suspend", (cmd_function)t_suspend, NO_SCRIPT, 0, 0}, {"t_continue", (cmd_function)t_continue, NO_SCRIPT, 0, 0}, + {"t_cancel_suspend", (cmd_function)t_cancel_suspend, NO_SCRIPT, 0, 0}, {0,0,0,0,0} }; diff --git a/modules/tm/tm_load.c b/modules/tm/tm_load.c index 1e38676..19e4bb0 100644 --- a/modules/tm/tm_load.c +++ b/modules/tm/tm_load.c @@ -225,6 +225,11 @@ int load_tm( struct tm_binds *tmb) LOG( L_ERR, LOAD_ERROR "'t_continue' not found\n"); return -1; } + if (! (tmb->t_cancel_suspend=(t_cancel_suspend_f)find_export("t_cancel_suspend", + NO_SCRIPT, 0))) { + LOG( L_ERR, LOAD_ERROR "'t_cancel_suspend' not found\n"); + return -1; + } tmb->t_get_reply_totag = t_get_reply_totag; tmb->t_get_picked_branch = t_get_picked_branch; diff --git a/modules/tm/tm_load.h b/modules/tm/tm_load.h index ff77010..16ed8e3 100644 --- a/modules/tm/tm_load.h +++ b/modules/tm/tm_load.h @@ -135,6 +135,7 @@ struct tm_binds { #endif t_suspend_f t_suspend; t_continue_f t_continue; + t_cancel_suspend_f t_cancel_suspend; tget_reply_totag_f t_get_reply_totag; tget_picked_f t_get_picked_branch; tlookup_callid_f t_lookup_callid;