[Home]

Summary:ASTERISK-13852: [patch] Added ability to perform SRV lookups for AGI URIs
Reporter:Brent Thomson (_brent_)Labels:
Date Opened:2009-03-27 13:24:46Date Closed:2010-01-18 18:30:06.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Resources/res_agi/NewFeature
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20091215__issue14775.diff.txt
( 1) hagi.patch
( 2) hagi-2.patch
( 3) hagi-3.patch
( 4) hagi-5.patch
Description:With this patch, you can run the Agi() dial plan application with a URI beginning with hagi:// (for HA Agi) to indicate that the host name should be resolved as an SRV service instead of a regular host name.

****** ADDITIONAL INFORMATION ******

Regular Agi still works the same. Hagi URIs should look like this in your dial plan:

   exten => _X.,1,Agi(hagi://example.com/foo.agi)

Asterisk will look up _agi._tcp.example.com and will try the normal fastagi routine on each host:port in the result until one accepts the connection or all results fail.
Comments:By: Eliel Sardanons (eliel) 2009-03-27 14:24:22

Please read the coding guidelines (http://svn.digium.com/svn/asterisk/trunk/doc/CODING-GUIDELINES).

I didn't test the functionality just did a simple code review:
*) Always check for errors when allocating memory (ast_strdupa)
*) Use braces on ifs and whiles:
   if (a) {
   }
*) Use Doxygen when documenting functions and remember if it is a static function to put the \internal tag (launch_ha_netscript).
*) You need to use AST_LIST_TRAVERSE_SAFE_BEGIN if you are going to remove an element from the list when traversing it, i don't think this is the case (or i am missing something).
*) Instead of a big
 if (!connection_mode) {

 }
 try to avoid indentation using something like:
 if (connection_mode) {
       continue;
 }
By: Eliel Sardanons (eliel) 2009-03-27 14:24:51

Thanks for the patch! :)
By: Brent Thomson (_brent_) 2009-03-27 16:14:38

1) check
2) check
3) check
4) I don't think you have to use AST_LIST_TRAVERSE_SAFE_BEGIN if you're calling AST_LIST_REMOVE_HEAD, just for AST_LIST_REMOVE_CURRENT (which isn't used here)
5) I put a break in the loop instead of the if(). Is there any reason it wouldn't be safe to break out of an AST_LIST_TRAVERSE_SAFE_BEGIN loop?


You're welcome :-)
By: Leif Madsen (lmadsen) 2009-12-01 11:01:17.000-0600

Could you also add some documentation with how this works, and any other information which would be useful to utilize this feature?

Cool feature! Also, can you verify if you're searching through multiple SRV records and not just the first one returned?
By: Brent Thomson (_brent_) 2009-12-10 15:12:15.000-0600

Sure thing. I'll add some documentation shortly.

My code *does* iterate through the list of results until either 1) one works, or 2) all results have failed. Was that your question?


By: Leif Madsen (lmadsen) 2009-12-10 15:17:46.000-0600

Great, thanks _brent_!
By: Brent Thomson (_brent_) 2009-12-10 19:15:28.000-0600

Hmmm ... I already added Doxygen comments and I'm just getting a new patch ready with a short addition to CHANGES. I don't see any other documentation in the source code for the AGI stuff. Is there somewhere else I should add some instructions?
By: Brent Thomson (_brent_) 2009-12-10 19:27:51.000-0600

Added hagi-3.patch. This patch includes a comment in CHANGES and is in the svn diff format.

My login isn't working on Review Board today. If somebody has a free minute and wants to post this patch there, too, I'd be much obliged. The URL is at the top of this page. I'll check back in a day or two and see if I have access again.
By: Tilghman Lesher (tilghman) 2009-12-15 17:37:19.000-0600

I have updated the patch with a strategy that should address oej's comments in the review.  See what you think of this code strategy.

Unfortunately, I cannot update the patch on reviewboard.  I could create another, but this is up to you.
By: Brent Thomson (_brent_) 2009-12-16 09:43:14.000-0600

Ah, I see. And I like your approach. I'll build an instance with your patch instead of the original and see if we can break anything :-)

Russel said there's some issue with the reviewboard login mechanism, but it should be fixed shortly. I'll check again after testing this for a few days and see if I can update the patch there.

Thanks, tilghman!
By: Brent Thomson (_brent_) 2009-12-17 09:41:49.000-0600

The updated patch appears to be causing seg faults in a couple of places. We'll hopefully have an updated updated patch later today.
By: Brent Thomson (_brent_) 2009-12-17 11:34:01.000-0600

OK, try this patch (hagi-5.patch). It has tilghman's work plus the fixes for segfaults.
By: Tilghman Lesher (tilghman) 2009-12-17 12:26:52.000-0600

I think the patch is good, but let's update reviewboard, if you haven't already done so, so we get a few more eyes on this.
By: Brent Thomson (_brent_) 2009-12-17 13:57:45.000-0600

Yeah, Russel is hoping the authentication issue with reviewboard will be fixed by the weekend. I'll get this up there as soon as I can.
By: Brent Thomson (_brent_) 2009-12-31 11:16:08.000-0600

Review Board login is working again (thanks, Russell!) and the newest patch is up there.
By: Digium Subversion (svnbot) 2010-01-18 18:28:53.000-0600

Repository: asterisk
Revision: 241188

U   trunk/CHANGES
U   trunk/include/asterisk/srv.h
U   trunk/main/srv.c
U   trunk/res/res_agi.c

------------------------------------------------------------------------
r241188 | tilghman | 2010-01-18 18:28:52 -0600 (Mon, 18 Jan 2010) | 9 lines

Create iterative method for querying SRV results, and use that for finding AGI servers.

(closes issue ASTERISK-13852)
Reported by: _brent_
Patches:
      20091215__issue14775.diff.txt uploaded by tilghman (license 14)
      hagi-5.patch uploaded by brent (license 388)
Tested by: _brent_

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

http://svn.digium.com/view/asterisk?view=rev&revision=241188
By: Digium Subversion (svnbot) 2010-01-18 18:30:05.000-0600

Repository: asterisk
Revision: 241188

U   trunk/CHANGES
U   trunk/include/asterisk/srv.h
U   trunk/main/srv.c
U   trunk/res/res_agi.c

------------------------------------------------------------------------
r241188 | tilghman | 2010-01-18 18:28:49 -0600 (Mon, 18 Jan 2010) | 10 lines

Create iterative method for querying SRV results, and use that for finding AGI servers.

(closes issue ASTERISK-13852)
Reported by: _brent_
Patches:
      20091215__issue14775.diff.txt uploaded by tilghman (license 14)
      hagi-5.patch uploaded by brent (license 388)
Tested by: _brent_
Reviewboard: https://reviewboard.asterisk.org/r/378/

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

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