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

[Mark Michelson]

<snip>
> 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);
>

Yep, the doxygen and the code disagree. The code needs the 
pjsip_rx_data. Good catch!

> 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?
>

This method is intended to be very simple. The method will construct a 
bare reinvite and then iterate through the list of session extensions 
and call their outgoing_request() callback. One (or multiple) registered 
session extension(s) will have the task of taking media information on 
the session and constructing an appropriate SDP for the reinvite.

Given this method as a "base" I'd be willing to also offer more 
specialized methods, such as one that takes new media parameters 
(expressed either as an SDP or some other way) so that the session can 
have its media parameters updated and then call the base method. I think 
having the base method defined first will allow for us to easily create 
specializations such as this as they arise.

> 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?
>

Much like ast_sip_session_send_reinvite(), my idea here was that this 
could serve as a base for more specialized functions that may arise. The 
idea I had here was that if you express the body as a string, it can be 
any sort of SIP body you want, whether it's single or multipart. 
However, I'd be willing to add a function specifically for multipart 
bodies that takes an array of strings instead like so:

int ast_sip_add_body_multipart(struct pjsip_tx_data *tdata, const char 
*bodies[]);

This function will construct the body as a string and then just call 
ast_sip_add_body() in order to send the message. Having a function like 
this will mean that construction of multipart bodies will exist in a 
single place in the code and it will be easier to track down errors if 
they exist.

This function may be a later addition though since our current milestone 
is to have code that allows for simple calls to be made.

>
> Keep up the good work guys!
>
Thanks! And thanks for having a look at the design and giving useful 
feedback.

Mark Michelson