Review Board 1.7.16


Initial work for res_sip and res_sip_session: Inbound and outbound calls work

Review Request #2285 - Created Jan. 23, 2013 and submitted

Mark Michelson
/team/group/pimp_my_sip
Reviewers
asterisk-dev
Asterisk
This is the first batch of code to review for the new SIP channel driver work. In this, we have the workings of res_sip, the core application-agnostic SIP API, and res_sip_session, the API and workhorse for SIP INVITE sessions. The goal while writing this code was to get into a state where we could run a call through Asterisk using SIP. This code is far from complete, but we reached the milestone of having working incoming and outgoing calls. The calls are currently limited to coming from a hardcoded endpoint and calling out to a hardcoded endpoint, and only ulaw audio is supported.

When you are reviewing this, the feedback we're interested in is the design. Do you foresee troubles with what is here? Is the documentation of the API clear? What we're not interested in at this point are things that are obviously incomplete and nitpicks.

Here's a short list of things we know are in need of improvement:
1) Any place where there is a XXX or TODO comment. In most cases, these are not necessary for reaching the goal of having working calls or are awaiting for sorcery integration.
2) res_sip_endpoint_identifier_constant is not intended to ever actually go into Asterisk. The real endpoint identifiers will use sorcery in order to find endpoints based on data in the SIP request. If the code there is shoddy, it's not a huge deal because it's going to be vaporized eventually.
3) The threadpool infrastructure is present, but it currently is not used. There are two reasons why:
    a) It's easy to add in threadpool usage after the fact, given how this is designed.
    b) We will need to use a newer version of PJSIP than what currently is in Asterisk.
4) The INVITE session state handler is not especially sophisticated. It currently is a switch statement for different event types, which is just the type of thing that could expand to unreasonable size if not kept in check. An array of callbacks either based on INVITE session state or event type would be much better.
5) The internal functions in res_sip and res_sip_session do not have doxygen yet. They will, eventually.
6) chan_gulp is a working name. It is not necessarily going to be the permanent name for the channel driver.
7) So far, every file that uses PJSIP has required an additional line to be added to the Makefile so that the PJ_FLAGS are added to the _AST_CFLAGS. This probably should be automated in some way so that we do not have to keep adding lines to the Makefile for every file we add.
Joshua and I have been sending calls to Asterisk and receiving calls from Asterisk using chan_gulp. We have also tested error scenarios like calling into a nonexistent extension. 
Review request changed
Updated (Jan. 25, 2013, 6:26 a.m.)
Somehow res_sip_session.h didn't make the upload. It should be here now.
Posted (Jan. 26, 2013, 1:47 a.m.)

   

  
/trunk/channels/chan_gulp.c (Diff revision 2)
 
 
 
 
I started thinking about this this morning and I've realized this could be problematic. If chan_gulp handles an incoming SIP request and starts a PBX thread, then other supplements might run afterwards and do manipulations to the channel.

A possible behavior you might see is that the supplement that extracts callerID might run after chan_gulp's supplement. In the PBX thread, though, the person might call the CALLERID() function in order to try to get or set caller ID on the channel. This presents a conflict.

So on the one hand, we could try to make chan_gulp's supplements be the final ones to be registered so the PBX thread is started as the final step.

On the other hand, having a channel created may be useful for supplements on incoming INVITEs. So maybe PBX thread starting should be its own separate supplement that runs last.
  1. This is tangentially related to the only design problem I've run into so far: What is an Asterisk thread and what is a pjlib thread?
    
    Consider the following shutdown sequence:
    
    static int unload_module(void)
    {
    	ast_res_sip_destroy_sorcery();
    	if (monitor_thread) {
    		stop_monitor_thread();
    	}
    	if (memory_pool) {
    		/* This will cause a crash in Asterisk in debug mode, as the thread
    		 * calling this is not a pjsip thread
    		 */
    		pj_pool_release(memory_pool);
    	}
    	if (ast_pjsip_endpoint) {
    		pjsip_endpt_destroy(ast_pjsip_endpoint);
    	}
    	pj_caching_pool_destroy(&caching_pool);
    	return 0;
    }
    
    This will crash when compiled in -dev-mode and run in the test suite. The thread shutting down Asterisk is not a pjlib thread, so calls to things like pj_pool_release cause assertion errors. Since I'd prefer that we don't have to register all Asterisk threads with pjlib, we'll need some mechanism to pass information between Asterisk and pjlib that is easily used in a variety of locations (I can imagine AMI/CLI needing this as well). This same pattern - a lack of defined barrier between Asterisk thread actions and pjproject thread actions - occurs within chan_gulp as well.
    
    What I think we'll need to have defined are barriers between where Asterisk interacts with objects and where pjlib interacts. When a channel callback occurs, it should create an object that represents the action it wants a channel servant to act upon and pass it to the channel servant via a thread-safe mechanism. The channel servant will have a registered pjlib thread and act on the action. Going the other direction is generally safer, but we may still want to have a way for channel servants to queue up data for the channel and have a particular set of threads service the channel servants.
    
    The best I can think of for module shutdown is to spawn a thread registered with pjlib, have that thread handle the shutdown, and join on the spawned thread in the module shutdown routine to prevent further shutdown actions from occurring.
  2. I think separating the act of setting up a channel and starting the PBX is wise and needed. We really just need to use a similar priority system as pjsip modules, with the PBX starting being last as you mentioned.
Posted (Jan. 28, 2013, 2:12 a.m.)

   

  
/trunk/channels/chan_gulp.c (Diff revision 2)
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
This should be refactored out into its own file.

That way, when oej's earl-gray work gets committed, this can easily take advantage of it.

As a general comment, I think we need to be more aggressive about putting similar functionality into separate files.
  1. Can you clarify a bit more on this? Do you mean an actual file that just gets built into chan_gulp and eventually chan_sip also?
/trunk/include/asterisk/res_sip.h (Diff revision 2)
 
 
 
I noticed this was already removed, as it duplicates the sorcery ID (which is the name of the endpoint).

While I don't think we need to duplicate the name, we should be explicit in the documentation of sorcery managed objects what the type name and key value are, so that people know how to retrieve the item from sorcery.

https://reviewboard.asterisk.org/ runs on a server provided by Digium, Inc. and uses bandwidth donated to the open source Asterisk community by API Digital Communications in Huntsville, AL USA.
Please report problems with this site to asteriskteam@digium.com.