----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3741/#review13014 -----------------------------------------------------------
Ship it! Minor findings for me, so going to go ahead and ship it. /team/mmichelson/rls-notify/res/res_pjsip_pubsub.c <https://reviewboard.asterisk.org/r/3741/#comment23433> s/param/brief /team/mmichelson/rls-notify/res/res_pjsip_pubsub.c <https://reviewboard.asterisk.org/r/3741/#comment23434> This function is generic enough and might be useful elsewhere or in the future for something else. Perhaps move it to a shared location (strings.h)? /team/mmichelson/rls-notify/res/res_pjsip_pubsub.c <https://reviewboard.asterisk.org/r/3741/#comment23437> Looks like some formatting issues here spaces vs. tabs maybe. /team/mmichelson/rls-notify/res/res_pjsip_pubsub.c <https://reviewboard.asterisk.org/r/3741/#comment23435> hmm, on the main diff view this shows as a red blob. - Kevin Harwell On Aug. 5, 2014, 3:10 p.m., Mark Michelson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3741/ > ----------------------------------------------------------- > > (Updated Aug. 5, 2014, 3: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_xpidf_body_generator.c 420055 > /team/mmichelson/rls-notify/res/res_pjsip_pubsub.c 420055 > /team/mmichelson/rls-notify/res/res_pjsip_pidf_body_generator.c 420055 > /team/mmichelson/rls-notify/res/res_pjsip_dialog_info_body_generator.c > 420055 > /team/mmichelson/rls-notify/include/asterisk/res_pjsip_pubsub.h 420055 > /team/mmichelson/rls-notify/include/asterisk/res_pjsip_presence_xml.h > 420055 > > 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
