[asterisk-dev] [Code Review]: res_sip and res_sip_session design review

[Mark Michelson]

> On Jan. 7, 2013, 10:06 a.m., jcolp wrote:
> > res_sip_session --
> > 
> > Structures:
> > 
> > 1. How does a session extension persist data on a session? mod_data in pjsip_inv_session?
> > 2. How does a session extension know the life time of the session? Does it know based on the messages? (Should we have common logic which then calls into callbacks in the extension)?
> > 3. I'm not a huge fan of using "extension" in the way you have, I understand why but it has other meanings in our system.
> > 4. The SDP stuff isn't just for INVITEs - it's for 200 OKs, 180s, 183s, etc.
> > 5. If the SDP stuff was extended more we could have the complete logic for each type of media (audio, video, T.38 udptl) separated out into different modules... would make organization cleaner. 
> > 
> > Common SIP methods:
> > 
> > 1. Your ast_sip_session_get_identity is missing the rdata
> > 2. How does something calling ast_sip_session_send_reinvite know the result?

Structures:

1. I'm not sure that mod_data in the pjsip_inv_session would be appropriate because it's likely that res_sip_session will be storing the ast_sip_session in the pjsip_inv_session mod_data. I can think of two possibilities for this:
    a) We copy the "session cookies" model from Asterisk SCF. In other words, given a SIP session, a session extension can add a cookie or retrieve a cookie as desired. The ast_sip_session structure would be where the data is stored.
    b) We add session_begin() and session_end() callbacks to the session extension callbacks so that session extensions can have more clear points to create and destroy session-specific data. The session extensions could then store the data locally where they please.

Personally, I'm a fan of a) over b).

2. Currently, a session extension would have to infer this information based on the content of the messages. Given that this could be error-prone, I think providing callbacks for when a session is begun and ended would be a safer option.

3. Yeah, agreed. I'm certainly open to suggestions for a better name since things can really get confusing if someone mentions a "SIP extension".  Maybe "adjunct", "addendum", or "supplement" (Yay thesauruses!) ?

4. Yep, I'll change language so that words like "offer" and "answer" are used instead of specific SIP methods.

5. Indeed. SDP handling is something that needs a bit more effort put in here. Right now it's encompassed in a single handler but really, it should be separated by stream types. "Audio" "Video" "Image" "Text" and "Other" perhaps? Maybe specifically state T.38 instead of image?

Common SIP methods:

1. Will fix
2. I assume by "result" you mean the response to the reinvite? My assumption is that the majority of entities that would call for a SIP reinvite to be sent would be SIP session extensions. When the response comes in, they'd have their incoming_response() callback called. If something other than a SIP session extension were to send a reinvite, they would not currently be able to see the response to the reinvite. Got any suggestions of how to go about it? Maybe provide an optional callback function in ast_sip_session_send_reinvite() when the response arrives?


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2251/#review7619
-----------------------------------------------------------


On Dec. 20, 2012, 1:17 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2251/
> -----------------------------------------------------------
> 
> (Updated Dec. 20, 2012, 1:17 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and jcolp.
> 
> 
> Summary
> -------
> 
> This is a proposal for a res_sip and res_sip_session API for use in the new SIP channel driver. The pages are located here:
> 
> https://wiki.asterisk.org/wiki/display/AST/res_sip+design
> https://wiki.asterisk.org/wiki/display/AST/res_sip_session+design
> 
> Please let me know what you think of these.
> 
> There are a few things that are not here and that probably should
> * A struct called ast_sip_endpoint is referenced in a few places, but it is not defined. This is because a SIP endpoint is more-or-less defined by the DAL, which is currently under development by Mr. Joshua Colp. Once endpoint configuration and related structures are defined, they can be added in to these pages.
> * There are no functions in res_sip_session for iterating over SDP media streams or attributes, nor are there any functions for aiding in creating SDPs. These likely should exist, but I have not placed them here now since I have difficulty seeing what parameters will be necessary nor what they might return.
> 
> 
> Diffs
> -----
> 
> 
> Diff: https://reviewboard.asterisk.org/r/2251/diff
> 
> 
> Testing
> -------
> 
> The wiki page renders properly.
> 
> 
> Thanks,
> 
> Mark
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130107/84cde6e8/attachment-0001.htm>