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

Mon Jan 7 19:17:50 CST 2013

jcolp wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2251/
>
>
>     On January 7th, 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?
>
>     On January 7th, 2013, 11:06 a.m., *Mark Michelson* wrote:
>
>         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?
>
> 1. Agree with'a'
> 2. Agreed
> 3. Hum hum hum, supplement for now?
> 4. Yay!
> 5. I think we should make it generic and have a handler specify a string which is the type carried on the media stream. That way in the future if people want to add even more (such as application) then no code changes needed. As well - being able to support multiple handlers of the same type with order loaded taking preference would be handy... first one that handles it stops processing. Cumulative implementation of types!
>
> 2. I think an optional callback would work nicely. Otherwise code duplication can occur for it.
>
>
> - jcolp
>
>
> On December 20th, 2012, 1:17 p.m., Mark Michelson wrote:
>
> Review request for Asterisk Developers, Matt Jordan and jcolp.
> By Mark Michelson.
>
> /Updated Dec. 20, 2012, 1:17 p.m./
>
>
>   Description
>
> 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.
>
>
>   Testing
>
> The wiki page renders properly.
>
>
>   Diffs
>
> View Diff <https://reviewboard.asterisk.org/r/2251/diff/>
>
> --
> _____________________________________________________________________
> -- Bandwidth and Colocation Provided by http://www.api-digital.com --
>
> asterisk-dev mailing list
> To UNSUBSCRIBE or update options visit:
>     http://lists.digium.com/mailman/listinfo/asterisk-dev

Hi,

Just a couple of comments from a shallow read:

I guess this: int ast_sip_session_get_identity(struct ast_party_id *id);

should be this: int ast_sip_session_get_identity(struct pjsip_rx_data 
*rdata, struct ast_party_id *id);

 From what I read in the doxygen comments I'm not sure if 
ast_sip_session_send_reinvite would be enough to handle advanced 
re-INVITE scenarios when streams are added, for example. Wouldn't it be 
nice if it took a new_sdp as a parameter, and if it's not NULL apply it 
as the new local sdp?

Also, ast_sip_add_body considers the body to be a single thing, but 
there are use cases where there can be a multipart body, maybe the API 
should allow for that? Or have a specific API for handling multipart bodies?

Keep up the good work guys!

-- 
Saúl Ibarra Corretgé
http://saghul.net/blog | http://about.me/saghul