----------------------------------------------------------- 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 > >
-- _____________________________________________________________________ -- 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
