[asterisk-dev] [Code Review] 3628: RLS: Abstract PJSIP-specific components from the pubsub API

Matt Jordan reviewboard at asterisk.org
Thu Jun 19 14:26:31 CDT 2014


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



/trunk/res/res_pjsip_pubsub.c
<https://reviewboard.asterisk.org/r/3628/#comment22300>

    I'm not against the 'real'/'virtual' nomenclature, but would 'parent'/'child' be better/clearer?



/trunk/res/res_pjsip_pubsub.c
<https://reviewboard.asterisk.org/r/3628/#comment22299>

    I'd go ahead and doxygen these as well.



/trunk/res/res_pjsip_pubsub.c
<https://reviewboard.asterisk.org/r/3628/#comment22301>

    Parent/child aside, I do enjoy the name of this union



/trunk/res/res_pjsip_pubsub.c
<https://reviewboard.asterisk.org/r/3628/#comment22297>

    There's a lot of things later on in the code that seem to really want that resource to exist. Is there any risk here that request_uri->user (and thus resource) is empty or NULL? If this is empty/NULL, what happens when things request the resource later?


- Matt Jordan


On June 18, 2014, 4:12 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3628/
> -----------------------------------------------------------
> 
> (Updated June 18, 2014, 4:12 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23865
>     https://issues.asterisk.org/jira/browse/ASTERISK-23865
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This is step one towards implementing resource lists in Asterisk. Once resource list subscriptions are implemented, an ast_sip_subscription may not directly correspond to an underly pjsip_evsub structure. Currently, subscription handlers will gladly try to reach underneath the ast_sip_subscription to get at the PJSIP-specific structures. This set of changes seeks to abstract the pubsub API further so that subscription handlers do not have any interaction with PJSIP.
> 
> The changes outlined at https://wiki.asterisk.org/wiki/display/AST/PJSIP+Subscription+Abstraction+Plan provide a fairly accurate overview of what this review request entails. Some differences are:
> * I was not able to completely separate subscribers and notifiers into separate structures. Instead, they are now separate sub-structures within the ast_sip_subscription_handler struct. The reason for this is that PJSIP is not flexible enough to register separate entities for clients and servers.
> * The ast_sip_subscription_accept(), ast_sip_subscription_reject(), and ast_sip_subscription_terminate() calls ended up not being needed.
> * All functions on the page that have a body parameter as a const char * are implemented to use a pjsip_msg_body instead. This is a case where the underlying PJSIP structure does not need to be abstracted, and having a parsed structure rather than a text blob will benefit everyone.
> * One thing that is not on that page, but that you will find in this review, is that we now store the SUBSCRIBE request that starts a subscription on the subscription dialog. This allows subscription handlers to be able to retrieve the values of certain headers from the SUBSCRIBE if they wish. This is necessary for the exten_state handler currently since it wants the content of the User-Agent header.
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_pjsip_xpidf_body_generator.c 416652 
>   /trunk/res/res_pjsip_pubsub.exports.in 416652 
>   /trunk/res/res_pjsip_pubsub.c 416652 
>   /trunk/res/res_pjsip_pidf_body_generator.c 416652 
>   /trunk/res/res_pjsip_mwi.c 416652 
>   /trunk/res/res_pjsip_exten_state.c 416652 
>   /trunk/include/asterisk/res_pjsip_pubsub.h 416652 
> 
> Diff: https://reviewboard.asterisk.org/r/3628/diff/
> 
> 
> Testing
> -------
> 
> Since no new functionality has been added, I just ran the current gamut of testsuite tests (tests/channels/pjsip/subscriptions). They all pass.
> 
> 
> Thanks,
> 
> Mark Michelson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140619/9ec3dfcc/attachment-0001.html>


More information about the asterisk-dev mailing list