[asterisk-dev] [Code Review] 3741: RLS: Add body generation + some bug fixes

[Matt Jordan]

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



/team/mmichelson/rls-notify/res/res_pjsip_pubsub.c
<https://reviewboard.asterisk.org/r/3741/#comment23397>

    Carried over from other review; may need off nominal handling of:
    
    * tree root node allocation failures
    * AST_VECTOR_INIT failure



/team/mmichelson/rls-notify/res/res_pjsip_pubsub.c
<https://reviewboard.asterisk.org/r/3741/#comment23400>

    There's a lot of off nominal allocation failures that could occur in here.



/team/mmichelson/rls-notify/res/res_pjsip_pubsub.c
<https://reviewboard.asterisk.org/r/3741/#comment23399>

    Probably need to check for allocation failure here



/team/mmichelson/rls-notify/res/res_pjsip_pubsub.c
<https://reviewboard.asterisk.org/r/3741/#comment23401>

    This is ... large.
    
    If I'm not mistaken, can't this be called recursively as it walks the tree, particularly in a list of lists situation?
    
    It may be better to calloc this buffer.



/team/mmichelson/rls-notify/res/res_pjsip_pubsub.c
<https://reviewboard.asterisk.org/r/3741/#comment23403>

    This can fail.



/team/mmichelson/rls-notify/res/res_pjsip_pubsub.c
<https://reviewboard.asterisk.org/r/3741/#comment23404>

    If this fails, you probably want to bail early.


- Matt Jordan


On July 25, 2014, 5 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3741/
> -----------------------------------------------------------
> 
> (Updated July 25, 2014, 5 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23867
>     https://issues.asterisk.org/jira/browse/ASTERISK-23867
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This represents the final piece of resource list subscription server support in Asterisk.
> 
> The lion's share of this changeset adds support for generating multipart bodies for resource list subscriptions. While testing, I also came across some other bugs, which are fixed here, too:
> 
> * shutdown_subscriptions - Was attempting to call a handler's subscription_shutdown() on list subscriptions. This only should be done for leaf subscriptions. Also, there was a refleak for list subscriptions since they did not release references to their children.
> * subscription_get_generator_from_rdata - This would not function correctly if multiple Accept headers were present on an incoming SUBSCRIBE.
> 
> 
> Though I say this is the final piece, what I really mean is that it's the final piece necessary to give Asterisk basic support for RLS. There are some avenues worth exploring still:
> * Dynamic lists: Currently, if a subscriber subscribes to a list, the content of that list is determined at subscription time and cannot change. If the resources on a configured list change, then the subscriber must end the current subscription and create a new subscription to the list in order to have the altered list elements appear properly. It is doable, though not necessarily easy, to modify the contents of a list subscription during the lifetime of the subscription dialog.
> * Ad-hoc lists: Lists currently must be configured on the server side. RFC 5367 supports the idea of the subscriber specifying a list of resources to subscribe to. Adding support for this would be nifty, though I am unsure what clients support this.
> * Methods of limiting body size: Currently, when sending full state of a resource list, Asterisk also sends instances of all resources. For lists of generous size, this can mean exceeding the default maximum packet length PJSIP allows. Asterisk could be modified to send just an RLMI body when communicating full state and then sending series of NOTIFYs with partial state in order to communicate the states of all resources in the list.
> 
> 
> Diffs
> -----
> 
>   /team/mmichelson/rls-notify/res/res_pjsip_pubsub.c 418321 
>   /team/mmichelson/rls-notify/include/asterisk/res_pjsip_pubsub.h 418321 
> 
> Diff: https://reviewboard.asterisk.org/r/3741/diff/
> 
> 
> Testing
> -------
> 
> Though I currently do not have testsuite tests I can point to as passing, I have manually performed the test cases covered by /r/3673 and through visual inspection, I can say that they work as intended. In addition to those tests, I also tested subscribing to lists of lists and ensured that the generated body looked correct in that case. I also tested batched notifications to ensure that the notification was batched, that multiple state changes would be reflected in a single batched notification, and that operations that should cancel a batch did so properly.
> 
> 
> Thanks,
> 
> Mark Michelson
> 
>

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