----------------------------------------------------------- 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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
