> On Aug. 6, 2014, 9 p.m., rmudgett wrote: > > /team/group/rls/res/res_pjsip_pubsub.c, line 1119 > > <https://reviewboard.asterisk.org/r/3723/diff/6/?file=66266#file66266line1119> > > > > AST_VECTOR_APPEND() can fail. child subscription leaked.
This call to AST_VECTOR_APPEND() cannot fail. The vector of children on the subscription is initialized to be the same size as the current tree node's vector of child nodes. AST_VECTOR_APPEND() can only fail if it has to resize. > On Aug. 6, 2014, 9 p.m., rmudgett wrote: > > /team/group/rls/res/res_pjsip_pubsub.c, lines 1567-1571 > > <https://reviewboard.asterisk.org/r/3723/diff/6/?file=66266#file66266line1567> > > > > sub_tree leaked? > On Aug. 6, 2014, 9 p.m., rmudgett wrote: > > /team/group/rls/res/res_pjsip_pubsub.c, line 1137 > > <https://reviewboard.asterisk.org/r/3723/diff/6/?file=66266#file66266line1137> > > > > It might be better to remove_subscription() first before destroying the > > sub_tree contents. > > > > remove_subscription() also does a module unref which would need to be > > moved to unlink the subscription from the list first. I'm getting a parse error on your second sentence, specificallly "which would need to be moved to unlink the subscription from the list first." The module unref is already happening after the subscription is unlinked from the list. > On Aug. 6, 2014, 9 p.m., rmudgett wrote: > > /team/group/rls/res/res_pjsip_pubsub.c, line 1537 > > <https://reviewboard.asterisk.org/r/3723/diff/6/?file=66266#file66266line1537> > > > > This is leaked when allocate_subscription() is called below. I should note that everything you point out in ast_sip_create_subscription() is currently broken because there's no code that actually exercises it. This function is used to create an outbound subscription, and we don't have anything that actually does that yet. > On Aug. 6, 2014, 9 p.m., rmudgett wrote: > > /team/group/rls/res/res_pjsip_pubsub.c, lines 1557-1563 > > <https://reviewboard.asterisk.org/r/3723/diff/6/?file=66266#file66266line1557> > > > > sub_tree leaked? Note that for all leaks of the sub_tree, I am switching the ao2_ref(sub, -1) to ao2_ref(sub_tree, -1). Your initial reaction upon seeing this may be that I'm now leaking sub, and this would be true in the changeset you're reviewing here. However, in /r/3741, I modified the sub_tree's destructor to destroy its child subscriptions. So cleaning up the sub_tree, should also clean up the sub. - Mark ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3723/#review13033 ----------------------------------------------------------- On Aug. 6, 2014, 3:44 p.m., Mark Michelson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3723/ > ----------------------------------------------------------- > > (Updated Aug. 6, 2014, 3:44 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 420145 > /team/group/rls/res/res_pjsip_mwi.c 420145 > /team/group/rls/res/res_pjsip_exten_state.c 420145 > /team/group/rls/include/asterisk/res_pjsip_pubsub.h 420145 > > 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
