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

[Mark Michelson]

> On Aug. 5, 2014, 4:14 p.m., Matt Jordan wrote:
> > /team/mmichelson/rls-notify/res/res_pjsip_pubsub.c, lines 1548-1568
> > <https://reviewboard.asterisk.org/r/3741/diff/2/?file=65312#file65312line1548>
> >
> >     There's a lot of off nominal allocation failures that could occur in here.

We typically don't check for allocation failures when it comes to PJLIB pools. The reason is that the pools are already allocated and are unlikely to expand. It's similar to how in Asterisk we don't check for allocation failure when setting a string field or appending to an ast_str.


> On Aug. 5, 2014, 4:14 p.m., Matt Jordan wrote:
> > /team/mmichelson/rls-notify/res/res_pjsip_pubsub.c, line 1620
> > <https://reviewboard.asterisk.org/r/3741/diff/2/?file=65312#file65312line1620>
> >
> >     Probably need to check for allocation failure here

See previous comment.


> On Aug. 5, 2014, 4:14 p.m., Matt Jordan wrote:
> > /team/mmichelson/rls-notify/res/res_pjsip_pubsub.c, line 1816
> > <https://reviewboard.asterisk.org/r/3741/diff/2/?file=65312#file65312line1816>
> >
> >     If this fails, you probably want to bail early.

See previous comments about not checking for failure when PJLIB pools are involved.


> On Aug. 5, 2014, 4:14 p.m., Matt Jordan wrote:
> > /team/mmichelson/rls-notify/res/res_pjsip_pubsub.c, line 1655
> > <https://reviewboard.asterisk.org/r/3741/diff/2/?file=65312#file65312line1655>
> >
> >     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.

Switching away from the large static buffer is a good idea anyway, but recursion is not a concern here.

build_rlmi_body() does not call any functions that may result in recursion. The function that calls build_rlmi_body(), generate_list_body(), can be called recursively though. Any stack space claimed by build_rlmi_body() is reclaimed before the recursion happens.

I'm going to do some wizardry regarding PJSIP message body callbacks so I can just store the XML directly in the body and be called back when PJSIP needs to print the message body.


- Mark


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


On July 25, 2014, 10 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, 10 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/74e484ce/attachment-0001.html>