[asterisk-dev] [Code Review] 3699: RLS: Inbound SUBSCRIBE handling

[Kevin Harwell]

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

Ship it!


Just a couple minor findings.


/team/group/rls/res/res_pjsip_pubsub.c
<https://reviewboard.asterisk.org/r/3699/#comment22748>

    Although unlikely, tree_node_alloc can return NULL (and if it does things are probably already in a bad state), but just in case a NULL check might be prudent.



/team/group/rls/res/res_pjsip_pubsub.c
<https://reviewboard.asterisk.org/r/3699/#comment22751>

    Surround these with #ifdef TEST_FRAMEWORK



/team/group/rls/res/res_pjsip_pubsub.c
<https://reviewboard.asterisk.org/r/3699/#comment22752>

    Surround these with #ifdef TEST_FRAMEWORK


- Kevin Harwell


On July 1, 2014, 6:21 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3699/
> -----------------------------------------------------------
> 
> (Updated July 1, 2014, 6:21 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23868
>     https://issues.asterisk.org/jira/browse/ASTERISK-23868
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This modifies res_pjsip_pubsub to be able to handle inbound SUBSCRIBE requests that set up resource list subscriptions. The main gist of this is creating the tree of ast_sip_subscription structures that will represent the resource list. The changes can be broken into the following categories:
> 
> * Code that gets rid of assumptions that all subscriptions are real subscriptions. Now, code should work equally well with real and virtual subscriptions.
> * Code that has been moved within the file. These are functions that previously were used by only a single caller but now are used by multiple callers. Therefore, the functions have been moved closer to the top of the file so that they may be referenced throughout.
> * Code that builds a resource tree. A resource tree is essentially the front line of handling a resource list SUBSCRIBE. When the SUBSCRIBE is handled the requested resource is looked up, and a tree is built based on the resources in the list. The tree is built by asking subscription handlers if the individual resources in the tree can be subscribed to. Those that can be subscribed to are added to the tree. In addition, as the tree is being built, duplicated resource names and looped lists are eliminated.
> * Code that builds a subscription tree. Once a resource tree is built, it is used to create a tree of ast_sip_subscriptions. It is this tree of subscriptions that will be used throughout the life of the SUBSCRIBE dialog to build notification bodies.
> * XXX comments indicating recognized problem spots that are to be addressed in created ASTERISK issues.
> * Unit tests that check the resource tree algorithms.
> 
> For the most part, I'm satisfied with this change set. The only thing I don't really like is that when subscribing to a resource list, the responses that Asterisk currently can send are either a 200 or 500. I could not think of an elegant way to give more specific responses (e.g. 482 when subscribing to a list that resulted in a loop).
> 
> 
> Diffs
> -----
> 
>   /team/group/rls/res/res_pjsip_pubsub.c 417733 
> 
> Diff: https://reviewboard.asterisk.org/r/3699/diff/
> 
> 
> Testing
> -------
> 
> Given that NOTIFY handling and an rlmi+xml body generator have not yet been written, I can't test this with testsuite tests yet. The best that I could do was to create unit tests for the resource tree algorithms. The tests verify that the resource trees are being built as expected, and they verify that off-nominal test cases (duplicated resources, loops, lists of bad resources) behave as expected. This lends some confidence to the idea that the front line is working, at least.
> 
> 
> Thanks,
> 
> Mark Michelson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140703/ee5b3868/attachment.html>