Module: sip-router
Branch: master
Commit: 84119209c6628ca001dffd3a22b009e3b6daccaf
URL: http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=8411920…
Author: Juha Heinanen <jh(a)tutpro.com>
Committer: Juha Heinanen <jh(a)tutpro.com>
Date: Mon Apr 19 20:36:27 2010 +0300
modules/tm: don't print error if branch is dropped in failure route
- I noticed that if I drop a branch after calling t_relay() in failure
route, an error message "w_t_relay_to: t_relay_to failed" was printed
to syslog. This patch suggested by Andrei suppresses this error
message.
---
modules/tm/tm.c | 12 ++++++++----
1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/modules/tm/tm.c b/modules/tm/tm.c
index 28687e6..e2a9cbf 100644
--- a/modules/tm/tm.c
+++ b/modules/tm/tm.c
@@ -1369,6 +1369,7 @@ inline static int _w_t_relay_to(struct sip_msg *p_msg ,
struct proxy_l *proxy, int force_proto)
{
struct cell *t;
+ int res;
if (is_route_type(FAILURE_ROUTE)) {
t=get_t();
@@ -1376,10 +1377,13 @@ inline static int _w_t_relay_to(struct sip_msg *p_msg ,
LOG(L_CRIT, "BUG: w_t_relay_to: undefined T\n");
return -1;
}
- if (t_forward_nonack(t, p_msg, proxy, force_proto)<=0 ) {
- LOG(L_ERR, "ERROR: w_t_relay_to: t_relay_to failed\n");
- /* let us save the error code, we might need it later
- when the failure_route has finished (Miklos) */
+ res = t_forward_nonack(t, p_msg, proxy, force_proto);
+ if (res <= 0) {
+ if (res != E_CFG) {
+ LOG(L_ERR, "ERROR: w_t_relay_to: t_relay_to failed\n");
+ /* let us save the error code, we might need it later
+ when the failure_route has finished (Miklos) */
+ }
tm_error=ser_error;
return -1;
}
THIS IS AN AUTOMATED MESSAGE, DO NOT REPLY.
The following task has been changed. The changes are listed below. For full information about what has changed, visit the URL and click the History tab.
FS#66 - In daemonize mode parent process should exit with error (-1) if main process fails to start
User who did this: Iñaki Baz Castillo (ibc)
Summary: Kamailio/SIP-Router should return error (255) when cannot assign requested address even in fork mode -> In daemonize mode parent process should exit with error (-1) if main process fails to start
Task details edited:
-------
Hi, there is an issue which prevents SR/Kamailio to being managed by software as HeartBeat (which mostly relies on init script return codes):
Let's suppose we configure SR to work in daemonize mode and to bind into a wrong address (i.e. a virtual address which for some reason is down in the server). When running it an error would be logged:
ERROR:core:udp_init: bind(6, 0x813f414, 16) on 1.2.3.4: Cannot assign requested address
However the command returns 0. This is ugly as it would indicate that the server has started correctly while in fact it's not running.
This occurs because the sockets are initialized after daemonizing so the SR main process is forked before detecting such address error. Then the parent process (that we started by executing the command) knows nothing about the failure and just returns 0.
Ok, so how to improve it? I asked for the same feature for Unicorn http server (a very robust http server writen in Ruby which is used by Github along with other sites): http://unicorn.bogomips.org/
The author implemented this feature, and the same could be implemented in SR. The mechanism is as follows (very simple and robust):
* The parent process (launched from a controlling terminal or script) shares a pipe with the SR master process. The parent process reads in the pipe and the master process will write into it.
* The parent process will wait until the master process is ready before returning. It will listen into the pipe until it received data from the master process, or until the pipe is closed by the master process:
* If the master process determines the server can run (no errors) it will communicate it to the parent process via the shared pipe. Then the parent process exits with 0 (OK).
* If instead the master process fails to start (i.e. "Cannot assign requested address") it will close the shared pipe. The parent process would detect such error (as the 'read' function would exit with error) and then it would exit with -1 (error).
I will try to implement it by myself, but would prefer to hear your opinions first :)
-------
Percent Complete: 0% -> 50%
More information can be found at the following URL:
http://sip-router.org/tracker/index.php?do=details&task_id=66
You are receiving this message because you have requested it from the Flyspray bugtracking system. If you did not expect this message or don't want to receive mails in future, you can change your notification settings at the URL shown above.
Module: sip-router
Branch: master
Commit: b663c49cf5c2a0b91453920afb14f9234f186e1e
URL: http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=b663c49…
Author: Miklos Tirpak <miklos(a)iptel.org>
Committer: Miklos Tirpak <miklos(a)iptel.org>
Date: Mon Apr 19 12:44:05 2010 +0200
data lumps: two new add lump functions
The following new functions are introduces that can be used
to add new lumps:
- add_new_lump(): Add a data lump right after the anchor point
into the main lump list. Every "before" lump of the same
anchor point will preceed, every "after" lump will
follow this lump.
- anchor_lump2(): Return the anchor point at the given offset
if exists, otherwise create a new anchor.
---
data_lump.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
data_lump.h | 9 +++++
2 files changed, 105 insertions(+), 1 deletions(-)
diff --git a/data_lump.c b/data_lump.c
index 024f602..433ce2f 100644
--- a/data_lump.c
+++ b/data_lump.c
@@ -81,6 +81,33 @@ struct lump* append_new_lump(struct lump** list, char* new_hdr,
return tmp;
}
+/* adds a header right after an anchor point if exists
+ * returns pointer on success, 0 on error */
+struct lump* add_new_lump(struct lump** list, char* new_hdr,
+ int len, enum _hdr_types_t type)
+{
+ struct lump** t;
+ struct lump* tmp;
+
+
+ t = (*list) ? &((*list)->next) : list;
+
+ tmp=pkg_malloc(sizeof(struct lump));
+ if (tmp==0){
+ LOG(L_ERR, "ERROR: add_new_lump: out of memory\n");
+ return 0;
+ }
+
+ memset(tmp,0,sizeof(struct lump));
+ tmp->type=type;
+ tmp->op=LUMP_ADD;
+ tmp->u.value=new_hdr;
+ tmp->len=len;
+ tmp->next=*t;
+ *t=tmp;
+ return tmp;
+}
+
/* inserts a header to the beginning
@@ -343,7 +370,7 @@ struct lump* anchor_lump(struct sip_msg* msg, int offset, int len, enum _hdr_typ
tmp=pkg_malloc(sizeof(struct lump));
if (tmp==0){
ser_error=E_OUT_OF_MEM;
- LOG(L_ERR, "ERROR: insert_new_lump_before: out of memory\n");
+ LOG(L_ERR, "ERROR: anchor_lump: out of memory\n");
return 0;
}
memset(tmp,0,sizeof(struct lump));
@@ -370,6 +397,74 @@ struct lump* anchor_lump(struct sip_msg* msg, int offset, int len, enum _hdr_typ
return tmp;
}
+/* add an anchor
+ * Similar to anchor_lump() but this function checks whether or not a lump
+ * has already been added to the same position. If an existing lump is found
+ * then it is returned without adding a new one and is_ref is set to 1.
+ *
+ * WARNING: this function adds the lump either to the msg->add_rm or
+ * msg->body_lumps list, depending on the offset being greater than msg->eoh,
+ * so msg->eoh must be parsed (parse with HDR_EOH) if you think your lump
+ * might affect the body!! */
+struct lump* anchor_lump2(struct sip_msg* msg, int offset, int len, enum _hdr_types_t type,
+ int *is_ref)
+{
+ struct lump* tmp;
+ struct lump* prev, *t;
+ struct lump** list;
+
+
+ /* extra checks */
+ if (offset>msg->len){
+ LOG(L_CRIT, "BUG: anchor_lump2: offset exceeds message size (%d > %d)"
+ " aborting...\n", offset, msg->len);
+ abort();
+ }
+ if (len){
+ LOG(L_WARN, "WARNING: anchor_lump2: called with len !=0 (%d)\n", len);
+ if (offset+len>msg->len)
+ LOG(L_WARN, "WARNING: anchor_lump2: offset + len exceeds message"
+ " size (%d + %d > %d)\n", offset, len, msg->len);
+ }
+
+ prev=0;
+ /* check to see whether this might be a body lump */
+ if ((msg->eoh) && (offset> (int)(msg->eoh-msg->buf)))
+ list=&msg->body_lumps;
+ else
+ list=&msg->add_rm;
+
+ for (t=*list;t; prev=t, t=t->next){
+ /* insert it sorted after offset */
+ if (((t->op==LUMP_DEL)||(t->op==LUMP_NOP))&&(t->u.offset>=offset))
+ break;
+ }
+ if (t && (t->u.offset==offset)) {
+ /* A lump with the same offset is found */
+ *is_ref=1;
+ return t;
+ }
+
+ tmp=pkg_malloc(sizeof(struct lump));
+ if (tmp==0){
+ ser_error=E_OUT_OF_MEM;
+ LOG(L_ERR, "ERROR: anchor_lump2: out of memory\n");
+ return 0;
+ }
+ memset(tmp,0,sizeof(struct lump));
+ tmp->op=LUMP_NOP;
+ tmp->type=type;
+ tmp->u.offset=offset;
+ tmp->len=len;
+
+ tmp->next=t;
+
+ if (prev) prev->next=tmp;
+ else *list=tmp;
+
+ *is_ref=0;
+ return tmp;
+}
void free_lump(struct lump* lmp)
diff --git a/data_lump.h b/data_lump.h
index 9fe82f4..5438c97 100644
--- a/data_lump.h
+++ b/data_lump.h
@@ -48,6 +48,11 @@
#include "parser/msg_parser.h"
#include "parser/hf.h"
+
+/* adds a header right after an anchor point if exists */
+struct lump* add_new_lump(struct lump** list, char* new_hdr,
+ int len, enum _hdr_types_t type);
+
/*! \brief adds a header to the end */
struct lump* append_new_lump(struct lump** list, char* new_hdr,
int len, enum _hdr_types_t type);
@@ -71,6 +76,10 @@ struct lump* insert_cond_lump_before(struct lump* after, enum lump_conditions c,
enum _hdr_types_t type);
/*! \brief removes an already existing header */
+/* set an anchor if there is no existing one at the given offset,
+ * otherwise return the existing anchor */
+struct lump* anchor_lump2(struct sip_msg* msg, int offset, int len, enum _hdr_types_t type,
+ int *is_ref);
struct lump* del_lump(struct sip_msg* msg, int offset, int len, enum _hdr_types_t type);
/*! \brief set an anchor */
struct lump* anchor_lump(struct sip_msg* msg, int offset, int len, enum _hdr_types_t type);
Hi, let me describe what I think it's a common case (I've already
experimented it sometimes and know others too):
- A request arrives and it's handled by a worker process.
- There is some memory leak in PKG MEM (or perhaps too few memory
allocated for it).
- The process still can do basic tasks as parsing and so.
- The script creates a dialog and does other operation requiring SHM memory.
- When calling to t_relay() it fails due to non enough PKG mem, so the
transaction is not created.
- Also depending on the memory status it's possible that the process
can not generate a SIP error response (so there is no response).
- The client starts with retransmissions.
- These retransmissions would not match an existing transaction so all
the script process is done again (creating a new dialog and so).
- Again t_relay() fails.
In this case, the dialog statistics show an exorbitant ammount of
dialogs, along with other data retrieved from shared memory.
So what I'm in mind is a new script function to get the current
available PKG mem, so the script can determine not to process the
request and reply (if it can) an error response. This would avoid the
creation of a new dialog, db queries and so on.
A workaround is calling t_newtran() at the beginning of the script,
but it's not a very "polite" solution. So I would prefer a script
funciont "get_pkg_mem()" which return the ammount of bytes available
in PKG MEM. Then I could decide whenever continue or terminate the
request process.
Does it sound feasible and useful? opinions?
Thanks.
--
Iñaki Baz Castillo
<ibc(a)aliax.net>
THIS IS AN AUTOMATED MESSAGE, DO NOT REPLY.
The following task has a new comment added:
FS#66 - Kamailio/SIP-Router should return error (255) when cannot assign requested address even in fork mode
User who did this - Iñaki Baz Castillo (ibc)
----------
I attach 3 patches with modifications to main.c, daemonize.c and daemonize.h which implement the desired feature.
Note however that these patches have been built for kamailio 1.5.4 (rev 6000).
The patches do the following:
When running in daemonize mode the parent process returns the proper status code. This is, if a Kamailio DB connection uses a wrong password, or a listen address is not a local address, then the parent process (which could be invoked as "/etc/init.d/kamailio start") would return -1 error rather than 0.
To achieve it, the patches do the following:
- 'daemonize()' function doesn't exit the parent process, instead the parent process gets blocked in 'main()' function reading a pipe.
- When the main process initializes the modules, sockets and so, it writes into the pipe so the parent process exists with 0.
- If the main process fails to start then the pipe write fd is closed with no more processes writting into it so the parent process detects it and exists with -1 error.
I strongly need this feature (this is the reason I've coded it first for kamailio 1.5.4) as it's required when working with HeartBeat. The reason is simple, a daemon init script *MUST NOT* return 0 (ok) if the daemon failed to start. These patches prevent such wrong behavior.
----------
One or more files have been attached.
More information can be found at the following URL:
http://sip-router.org/tracker/index.php?do=details&task_id=66#comment79
You are receiving this message because you have requested it from the Flyspray bugtracking system. If you did not expect this message or don't want to receive mails in future, you can change your notification settings at the URL shown above.
THIS IS AN AUTOMATED MESSAGE, DO NOT REPLY.
Iñaki Baz Castillo has taken ownership of the following task:
FS#66 - Kamailio/SIP-Router should return error (255) when cannot assign requested address even in fork mode
More information can be found at the following URL:
http://sip-router.org/tracker/index.php?do=details&task_id=66
You are receiving this message because you have requested it from the Flyspray bugtracking system. If you did not expect this message or don't want to receive mails in future, you can change your notification settings at the URL shown above.
i have two lcr gws defined and i call next_gw() followed by t_relay()
first time in route block, but that gw gets drop()ed in branch route:
Apr 18 09:25:35 localhost /usr/sbin/sip-proxy[13916]: INFO: About to relay INVITE <sip:0407058055@192.98.100.2:5060>
Apr 18 09:25:35 localhost /usr/sbin/sip-proxy[13916]: NOTICE: Dropping INVITE <sip:0407058055@192.98.100.2:5060> to gateway
Apr 18 09:25:35 localhost /usr/sbin/sip-proxy[13916]: NOTICE: Relaying
request INVITE <sip:0407058055@192.98.100.2:5060> failed with result
<-6>
then, in the same route block, i call next_gw() again (which appends the
second gw as a new branch), followed by t_relay(), but get an error that
i have not seen before:
Apr 18 09:25:35 localhost /usr/sbin/sip-proxy[13916]: INFO: About to relay INVITE <sip:0407058055@192.98.100.34:5060>
Apr 18 09:25:35 localhost /usr/sbin/sip-proxy[13916]: BUG: tm [t_lookup.c:1556]: tm: t_unref: REQ_ERR DELAYED should have been caught much earlier for 0xb4e6a690: 17 (hex 11)
what does that mean? is it an indication of a tm bug or is there
something wrong what i do in the script as described above?
-- juha
Hi, t_relay() negative return codes is something ugly, more when there
is parallel or serial forking. For example:
- Parallel forking to two different locations.
- The first destination gets a -4: bad destination (unresolvable address).
- The second destination gest a -5 - destination filtered (black listed).
- But t_relay() would reply just one of both codes.
Also when mixing UDP and TCP branches t_relay() return codes get more
complex to evaluate. For the same kind of error in UDP t_relay()
returns true and we get into failure_route, and in TCP t_relay()
returns a nevative value and failure_route is not executed. This is
difficult to manage, too much exotic cases. Mixing it with parallel
forking we get something really terrible.
So I wonder, shouldn't be better if we forgot the t_relay() codes and
instead handle the error of the winning branch (even if it failed or
it was no branch) in failure_route?
This could mean new functions to be executed in failure_route, something like:
- t_no_destination_available:
Returns true if the failure route is executed because no branches were
added or request was already cancelled. It would generate a local 500
error by default.
- t_branch_bad destination:
Returns true if the failure route is executed for a branch whose
destination was an uresolvable address. Generates a local 500 error
(or perhaps other).
- t_branch_destination_filtered:
Returns true if the failure route is executed for a branch whose
destination was black listed. Generates a local 500.
- t_branch_tcp_error:
Returns true if the failure route is executed for a TCP branch for
which the connection was not possible. Generates a local 500.
- t_failed:
Returns true if no request could be sent for this transaction (any of
the above returns true). Generates a local 500.
...and so on.
Then any code we actually run into "if ! t_relay() { ... }" could be
executed in failure_route and we get an easier and solid way to handle
errors (instead of mixing t_relay negative values, local generated
negative responses in failure_route and so).
Also this would mean that t_relay() terminates the request process, it
acts as "exit();". Then we can inspect failure_route to get the result
in case it's negative.
Opinions? I strongly think it would be really better than the current behavior.
--
Iñaki Baz Castillo
<ibc(a)aliax.net>
Hello,
load_tm() function uses a mixture of find_export and direct assignment
to export the API for TM. I would like to update to the last one, since
will cleanup the module exports structure and avoid casting problems
when functions prototypes change.
Then I see that tm uses some defines for the names of the several
functions exported to config, e.g., T_FORWARD_NONACK, ..., which will
have no other usage, I would go for using directly the names in module
exports - makes life easier when browsing the code and will become coherent.
Anyone against?
Thanks,
Daniel
--
Daniel-Constantin Mierla