[Home]

Summary:ASTERISK-03027: acl.c:ast_ouraddrfor is grossly complex and can be significantly simplified
Reporter:sailer (sailer)Labels:
Date Opened:2004-12-18 09:05:38.000-0600Date Closed:2008-01-15 15:17:02.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) asterisk-ouraddrfor.patch
( 1) asterisk-ouraddrfor.txt
( 2) x.c
Description:ast_ouraddrfor tries to determine the IP address the kernel uses as its own address when sending to another IP address. The routine currently implements this by mimicing kernel routing algorithms.

This routine can be simplified by letting the kernel do its task and just asking it for the address it uses. See the attached patch for how to do this. The new routine just creates an UDP socket and connects to the peer. The connect syscall forces the kernel to perform the routing decisions. The code then asks the kernel for the source address it will be using when sending to the peer by using the getsocknam syscall. The advantages of the new code:

* simplicity
* one version should work on any reasonably modern unix system (BSD >= 4.2, SvR >=4, according to manpage)
* no fragile parsing of /proc text files

Note this also applies to current CVS (as of yesterday)

Comments:By: sailer (sailer) 2004-12-18 09:11:49.000-0600

This is a test program for the getsocknam syscall. (note file uploads in mantis do not seem to work (config error))

#include <sys/socket.h>
#include <netinet/in.h>
#include <netdb.h>
#include <arpa/inet.h>
#include <stdio.h>
#include <errno.h>
#include <unistd.h>

int main(int argc, char *argv[])
{
       int s;
       struct hostent *h;
       struct sockaddr_in sin;
       socklen_t slen;

       if (argc < 2) {
               fprintf(stderr, "Usage: ipaddrtest <remotehost>\n");
               return 1;
       }
       s = socket(PF_INET, SOCK_DGRAM, 0);
       if (s == -1) {
               perror("socket");
               return 1;
       }
       if (!inet_aton(argv[1], &sin.sin_addr)) {
               h = gethostbyname(argv[1]);
               if (!h) {
                       fprintf(stderr, "Cannot resolve \"%s\"\n", argv[1]);
                       return 1;
               }
               if (h->h_addrtype != AF_INET) {
                       fprintf(stderr, "%s: wrong address type\n", h->h_name);
                       return 1;
               }
               sin.sin_addr = *(struct in_addr *)h->h_addr_list[0];
               endhostent();
       }
       sin.sin_family = AF_INET;
       sin.sin_port = 5060;
       printf("0x%08x:%u\n", sin.sin_addr.s_addr, sin.sin_port);
       if (connect(s, (struct sockaddr *)&sin, sizeof(sin))) {
               perror("connect");
               return 1;
       }
       slen = sizeof(sin);
       if (getsockname(s, (struct sockaddr *)&sin, &slen)) {
               perror("getsockname");
               return 1;
       }
       close(s);
       printf("our address: %s:%u\n", inet_ntoa(sin.sin_addr), sin.sin_port);
       return 0;
}
By: Mark Spencer (markster) 2004-12-18 09:13:31.000-0600

Well that's certainly an interesting approach :) Looks workable to me as long as getsockname isn't going to give us back 0's on supported OS's.  Please submit a disclaimer if you haven't already, or confirm you have one if you've already sent one, and we can get this in head and see if it's really as simple and portable as it looks to be!
By: sailer (sailer) 2004-12-18 10:22:36.000-0600

Disclaimer on the way.

There's a brown paper bug in the patch: the shutdown should be a close, otherwise a file descriptor leaks on every call. duh
By: Olle Johansson (oej) 2004-12-18 13:51:53.000-0600

Hmmm. Can this code work with ASTERISK-2326 as well?

How much time does it take to open an UDP socket? Is this critical?
By: sailer (sailer) 2004-12-18 15:13:34.000-0600

Bug 0002358: this patch would probably not help, but this depends on the routing table, which the requester of 2358 hasn't posted AFAIK. The patch supplied in 2358 seems the correct way to solve 2358 to me, regardless of the routingtable. The same effect could probably be achieved by using one fd per peer, and bind/connect, in a more portable and slightly quicker way, at the expense of an fd per peer and some significant code restructuring.

Performance: see the attached program (x.c). On my Athlon 800, the proposed method is about three times faster than reading /proc/net/route, which is part of what the current method does. Conclusion: the proposed method is at least 3 times faster. (Fedora Core 3)

$ ./x mit.edu
our address: 10.0.0.2, time 62us
time to read /proc/net/route 172us
By: Olle Johansson (oej) 2004-12-18 15:52:42.000-0600

Sounds good to me. Just wanted to check :-)
By: Mark Spencer (markster) 2004-12-18 16:59:05.000-0600

Added to CVS, having received a scanned disclaimer via e-mail.  Thanks for your contribution!
By: Russell Bryant (russell) 2004-12-21 15:04:41.000-0600

Unless there is a bug in the way this is currently handled, I'm just going to leave it alone for the stable branch.
By: Digium Subversion (svnbot) 2008-01-15 15:17:02.000-0600

Repository: asterisk
Revision: 4477

U   trunk/acl.c

------------------------------------------------------------------------
r4477 | markster | 2008-01-15 15:17:01 -0600 (Tue, 15 Jan 2008) | 2 lines

Improve ACL performance (thanks sailer) (bug ASTERISK-3027)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=4477