[asterisk-commits] rizzo: branch rizzo/astobj2 r47231 -
/team/rizzo/astobj2/channels/chan_sip.c
asterisk-commits at lists.digium.com
asterisk-commits at lists.digium.com
Mon Nov 6 10:58:23 MST 2006
Author: rizzo
Date: Mon Nov 6 11:58:22 2006
New Revision: 47231
URL: http://svn.digium.com/view/asterisk?rev=47231&view=rev
Log:
remove netlock, saving a lock/unlock pair on every received packet.
The race that could derive from this change is harmless and
only possible during a "sip reload" that changes the bind address,
and communication would be broken anyways so we just live with it
and the occasional error message that may occur.
(note that protection was incomplete, anyways, and did not cover
the IO handler registered on the socket).
On passing, fix a potential null pointer dereference (sipsock_read_id
could be NULL if the initial opening of the socket fails).
This is not a severe bug but probably enough for making this
change a candidate for both trunk and 1.4
Modified:
team/rizzo/astobj2/channels/chan_sip.c
Modified: team/rizzo/astobj2/channels/chan_sip.c
URL: http://svn.digium.com/view/asterisk/team/rizzo/astobj2/channels/chan_sip.c?rev=47231&r1=47230&r2=47231&view=diff
==============================================================================
--- team/rizzo/astobj2/channels/chan_sip.c (original)
+++ team/rizzo/astobj2/channels/chan_sip.c Mon Nov 6 11:58:22 2006
@@ -558,18 +558,6 @@
static int regobjs = 0; /*!< Registry objects */
static struct ast_flags global_flags[2] = {{0}}; /*!< global SIP_ flags */
-
-/*!
- * netlock protects changes to the socket used for SIP communications
- * while it is being used.
- * XXX This is a very expensive solution to use,
- * because it forces us to a lock/unlock around each instance of find_call(),
- * for something (sip_reload with a change of address) which is rarely used.
- * Given that, if we change address, all incoming traffic is bound to
- * fail anyways, we could as well forget about the lock and just drop
- * the transmission if sipsock = -1
- */
-AST_MUTEX_DEFINE_STATIC(netlock);
/*! \brief Protect the monitoring thread, so only one process can kill or start it, and not
when it's doing something critical. */
@@ -1195,7 +1183,22 @@
/* --- Sockets and networking --------------*/
-static int sipsock = -1; /*!< Main socket for SIP network communication */
+
+/*!
+ * __sipsock is shared between the manager thread (which handles reload
+ * requests), the io handler (sipsock_read()) and the user routines that
+ * issue writes (using __sip_xmit()).
+ * The socket is -1 only when opening fails (this is a permanent condition),
+ * or when we are handling a reload() that changes its address (this is
+ * a transient situation during which we might have a harmless race, see
+ * below). Because the conditions for the race to be possible are extremely
+ * rare, we don't want to pay the cost of locking on every I/O.
+ * Rather, we remember that when the race may occur, communication is
+ * bound to fail anyways, so we just live with this event and let
+ * the protocol handle this above us.
+ */
+
+static volatile int __sipsock = -1; /*!< Main socket for SIP network communication */
static struct sockaddr_in bindaddr = { 0, }; /*!< The address we bind to */
static struct sockaddr_in externip; /*!< External IP address if we are behind NAT */
static char externhost[MAXHOSTNAMELEN]; /*!< External host name (possibly with dynamic DNS and DHCP */
@@ -1752,7 +1755,11 @@
{
int res;
const struct sockaddr_in *dst = sip_real_dst(p);
- res = sendto(sipsock, data, len, 0, (const struct sockaddr *)dst, sizeof(struct sockaddr_in));
+ /*
+ * There is a short window during a reload when this might fail.
+ * Just live with it.
+ */
+ res = sendto(__sipsock, data, len, 0, (const struct sockaddr *)dst, sizeof(struct sockaddr_in));
if (res != len)
ast_log(LOG_WARNING, "sip_xmit of %p (len %d) to %s:%d returned %d: %s\n", data, len, ast_inet_ntoa(dst->sin_addr), ntohs(dst->sin_port), res, strerror(errno));
@@ -7568,7 +7575,7 @@
peer->addr.sin_family = AF_INET;
peer->addr.sin_addr = in;
peer->addr.sin_port = htons(port);
- if (sipsock < 0) {
+ if (__sipsock < 0) {
/* SIP isn't up yet, so schedule a poke only, pretty soon */
if (peer->pokeexpire > -1)
ast_sched_del(sched, peer->pokeexpire);
@@ -14644,7 +14651,11 @@
int lockretry;
memset(&req, 0, sizeof(req));
- res = recvfrom(sipsock, req.data, sizeof(req.data) - 1, 0, (struct sockaddr *)&sin, &len);
+ /*
+ * There is a short window during a reload when this might fail.
+ * Just live with it.
+ */
+ res = recvfrom(__sipsock, req.data, sizeof(req.data) - 1, 0, (struct sockaddr *)&sin, &len);
if (res < 0) {
#if !defined(__FreeBSD__)
if (errno == EAGAIN)
@@ -14677,22 +14688,13 @@
if (req.headers < 2) /* Must have at least two headers */
return 1;
- /* Process request, with netlock held, and with usual deadlock avoidance */
- /* XXX we would like to call find_call() first, and then acquire the netlock
- * afterwards. However, in some situations (creating new dialogs, basically)
- * find_call sends a message, so we need to run it with netlock held.
- * This said, netlock is probably useless (see the comment near its
- * declaration) and we could as well forget about it.
- */
+ /* Process request with usual deadlock avoidance */
for (lockretry = 100; lockretry > 0; lockretry--) {
- ast_mutex_lock(&netlock);
-
/* Find the active SIP dialog or create a new one */
p = find_call(&req, &sin, req.method); /* returns p locked */
if (p == NULL) {
if (option_debug)
ast_log(LOG_DEBUG, "Invalid SIP message - rejected , no callid, len %d\n", req.len);
- ast_mutex_unlock(&netlock);
return 1;
}
/* Go ahead and lock the owner if it has one -- we may need it */
@@ -14702,7 +14704,6 @@
if (option_debug)
ast_log(LOG_DEBUG, "Failed to grab owner channel lock, trying again. (SIP call %s)\n", p->callid);
sip_pvt_unlock(p);
- ast_mutex_unlock(&netlock);
/* Sleep for a very short amount of time */
usleep(1);
}
@@ -14729,7 +14730,6 @@
if (p->owner && !nounlock)
ast_channel_unlock(p->owner);
sip_pvt_unlock(p);
- ast_mutex_unlock(&netlock);
if (recount)
ast_update_use_count();
@@ -14868,10 +14868,6 @@
int curpeernum;
int reloading;
- /* Add an I/O event to our SIP UDP socket */
- if (sipsock > -1)
- sipsock_read_id = ast_io_add(io, sipsock, sipsock_read, AST_IO_IN, NULL);
-
/* From here on out, we die whenever asked */
for(;;) {
/* Check for a reload request */
@@ -14883,10 +14879,6 @@
if (option_verbose > 0)
ast_verbose(VERBOSE_PREFIX_1 "Reloading SIP\n");
sip_do_reload(sip_reloadreason);
-
- /* Change the I/O fd of our UDP socket */
- if (sipsock > -1)
- sipsock_read_id = ast_io_change(io, sipsock_read_id, sipsock, NULL, 0, NULL);
}
/* Check for dialogs needing to be killed */
dialoglist_lock();
@@ -16434,43 +16426,51 @@
if (!ntohs(bindaddr.sin_port))
bindaddr.sin_port = ntohs(STANDARD_SIP_PORT);
bindaddr.sin_family = AF_INET;
- ast_mutex_lock(&netlock);
- if ((sipsock > -1) && (memcmp(&old_bindaddr, &bindaddr, sizeof(struct sockaddr_in)))) {
- close(sipsock);
- sipsock = -1;
- }
- if (sipsock < 0) {
- sipsock = socket(AF_INET, SOCK_DGRAM, 0);
- if (sipsock < 0) {
+ /*
+ * handle changes in the socket used for communications.
+ * At the beginning, sipsock = -1 and old_bindaddr = 0:0 so
+ * we detect a change easily.
+ */
+ if (__sipsock == -1 || memcmp(&old_bindaddr, &bindaddr, sizeof(struct sockaddr_in))) {
+ /* address changed, or previous open failed */
+ int my_socket = __sipsock;
+
+ __sipsock = -1; /* it is changing... */
+ if (my_socket > -1) {
+ ast_io_remove(io, sipsock_read_id);
+ close(my_socket);
+ }
+ my_socket = socket(AF_INET, SOCK_DGRAM, 0);
+ if (my_socket < 0) {
ast_log(LOG_WARNING, "Unable to create SIP socket: %s\n", strerror(errno));
} else {
/* Allow SIP clients on the same host to access us: */
const int reuseFlag = 1;
- setsockopt(sipsock, SOL_SOCKET, SO_REUSEADDR,
+ setsockopt(my_socket, SOL_SOCKET, SO_REUSEADDR,
(const char*)&reuseFlag,
sizeof reuseFlag);
- ast_enable_packet_fragmentation(sipsock);
-
- if (bind(sipsock, (struct sockaddr *)&bindaddr, sizeof(bindaddr)) < 0) {
+ ast_enable_packet_fragmentation(my_socket);
+
+ if (bind(my_socket, (struct sockaddr *)&bindaddr, sizeof(bindaddr)) < 0) {
ast_log(LOG_WARNING, "Failed to bind to %s:%d: %s\n",
ast_inet_ntoa(bindaddr.sin_addr), ntohs(bindaddr.sin_port),
strerror(errno));
- close(sipsock);
- sipsock = -1;
+ close(my_socket);
} else {
if (option_verbose > 1)
ast_verbose(VERBOSE_PREFIX_2 "SIP Listening on %s:%d\n",
ast_inet_ntoa(bindaddr.sin_addr), ntohs(bindaddr.sin_port));
- if (setsockopt(sipsock, IPPROTO_IP, IP_TOS, &global_tos_sip, sizeof(global_tos_sip)))
+ if (setsockopt(my_socket, IPPROTO_IP, IP_TOS, &global_tos_sip, sizeof(global_tos_sip)))
ast_log(LOG_WARNING, "Unable to set SIP TOS to %s\n", ast_tos2str(global_tos_sip));
else if (option_verbose > 1)
ast_verbose(VERBOSE_PREFIX_2 "Using SIP TOS: %s\n", ast_tos2str(global_tos_sip));
+ __sipsock = my_socket;
+ sipsock_read_id = ast_io_add(io, __sipsock, sipsock_read, AST_IO_IN, NULL);
}
}
}
- ast_mutex_unlock(&netlock);
/* Add default domains - host name, IP address and IP:port */
/* Only do this if user added any sip domain with "localdomains" */
@@ -17280,7 +17280,7 @@
clear_realm_authentication(authl);
clear_sip_domains();
- close(sipsock);
+ close(__sipsock);
sched_context_destroy(sched);
return 0;
More information about the asterisk-commits
mailing list