----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3723/#review12986 -----------------------------------------------------------
/team/group/rls/res/res_pjsip_exten_state.c <https://reviewboard.asterisk.org/r/3723/#comment23387> This can fail, so it needs an off-nominal check as well. /team/group/rls/res/res_pjsip_pubsub.c <https://reviewboard.asterisk.org/r/3723/#comment23392> Should we check for an allocation failure of the new root? /team/group/rls/res/res_pjsip_pubsub.c <https://reviewboard.asterisk.org/r/3723/#comment23394> This involves a malloc/calloc and hence can fail: * \return 0 on success. * \return Non-zero on failure. /team/group/rls/res/res_pjsip_pubsub.c <https://reviewboard.asterisk.org/r/3723/#comment23393> And likewise here /team/group/rls/res/res_pjsip_pubsub.c <https://reviewboard.asterisk.org/r/3723/#comment23396> This is really more of a general question then a specific finding, however: What is the threading model for updating/iterating/destroying children in a tree? What all threads can access a subscription's children? Does a subscription need to be locked? /team/group/rls/res/res_pjsip_pubsub.c <https://reviewboard.asterisk.org/r/3723/#comment23395> I'd put a debug comment at least if child can't be created. If for whatever reason the virtual subscriptions fail, this could be a very strange thing to hunt down without some evidence as to where it failed. - Matt Jordan On July 25, 2014, 3:57 p.m., Mark Michelson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3723/ > ----------------------------------------------------------- > > (Updated July 25, 2014, 3:57 p.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-23869 > https://issues.asterisk.org/jira/browse/ASTERISK-23869 > > > Repository: Asterisk > > > Description > ------- > > This adds NOTIFY support for resource list subscriptions. The way this works > is as follows: > > When an initial SUBSCRIBE arrives and the subscription tree is built, all > leaf nodes are called into in order to generate their initial NOTIFY bodies > and store these on their respective subscription nodes. > > Sending a NOTIFY requires traversing the tree. List subscriptions will > generate a multipart/related body with an RLMI part and parts corresponding > to the leaves of the list (at least they will eventually. ASTERISK-23867 > involves doing this part). Single-resource subscriptions read the stored body > on the subscription and uses that to populate the NOTIFY body. > > Leaf nodes in the subscription tree, when they have a state change occur, > call ast_sip_subscription_notify(), as they previously did. > ast_sip_subscription_notify() creates the NOTIFY body for the subscription > and stores that on the subscription in the body_text ast_str field. The > subscription tree is then told to send a NOTIFY if no batching is enabled or > to start a batched NOTIFY if batching is enabled. > > When a resource resubscribes or terminates their subscription, a NOTIFY is > now automatically generated by the pubsub core instead of calling into > subscription handlers. The NOTIFY is built the same way as previously, using > stored NOTIFY bodies on the subscription. This NOTIFY can also cause batched > notification, when the timer fires, not to actually send their batch since it > would be redundant. > > You'll notice the code has been refactored slightly, and a new struct, > sip_subscription_tree, has invaded res_pjsip_pubsub. This is because, as I > was separating the "real" and "virtual" parts of ast_sip_subscriptions out, I > realized that I essentially had two distinct structures. Thus, I separated > the real/meta/base elements of a subscription into the sip_subscription_tree, > and the resource-specific parts into the ast_sip_subscription struct. > sip_subscription_tree is used more heavily in the pubsub core now, whereas > ast_sip_subscription acts as a handle for subscription handlers to use plus a > repository for resource-specific data. > > > Diffs > ----- > > /team/group/rls/res/res_pjsip_pubsub.c 418321 > /team/group/rls/res/res_pjsip_mwi.c 418321 > /team/group/rls/res/res_pjsip_exten_state.c 418321 > /team/group/rls/include/asterisk/res_pjsip_pubsub.h 418321 > > Diff: https://reviewboard.asterisk.org/r/3723/diff/ > > > Testing > ------- > > With this set of changes, I'm still not able to perform RLS-specific tests > since there is still no method of generating multipart/related or RLMI > bodies. However, with these changes, I did run the gamut of subscription > tests in the testsuite and they all pass. This at least means that there are > no detectable regressions at this point. > > > 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
