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

[Matt Jordan]

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



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

    Thinking about the error conditions here:
    * If the new_subscribe handler returned the SIP response code as an error, that could be returned from this function.
    * The recursive calls to this function would return the result if non-zero
    * That _may_ then allow build_resource_tree to return a more specific error condition if something goes wrong when building out the tree



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

    If a new_subscribe handler fails, should something be done to reject the entire tree?



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

    I'd put some debug messages into the construction of the tree, such that someone looking at a debug log has an indication what the tree of resources looks like for a particular subscribe request. This would also apply to build_node_children



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

    A debug message saying we are removing a subscription may be warranted


- Matt Jordan


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/225e27f8/attachment-0001.html>