----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3723/#review13033 -----------------------------------------------------------
/team/group/rls/res/res_pjsip_exten_state.c <https://reviewboard.asterisk.org/r/3723/#comment23462> exten_state_data.device_state_info setup could be simplified this way: task_data->exten_state_data.device_state_info = ao2_bump(info->device_state_info); /team/group/rls/res/res_pjsip_exten_state.c <https://reviewboard.asterisk.org/r/3723/#comment23464> Off nominal leak of info if exten_state_data->pool creation fails. Info can be eliminated along with the leak by passing &exten_state_data->device_state_info instead of info to ast_extension_state_extended(). Also it would be better if the assignment inside the if were extracted as two statements. var = func(); if (var < 0) /team/group/rls/res/res_pjsip_mwi.c <https://reviewboard.asterisk.org/r/3723/#comment23465> Extraneous blank line. /team/group/rls/res/res_pjsip_mwi.c <https://reviewboard.asterisk.org/r/3723/#comment23466> "MWI datastore" should be a define or global string variable. As it is, the string could have a typo and the compiler cannot catch it. /team/group/rls/res/res_pjsip_pubsub.c <https://reviewboard.asterisk.org/r/3723/#comment23476> AST_VECTOR_APPEND() can fail. child subscription leaked. /team/group/rls/res/res_pjsip_pubsub.c <https://reviewboard.asterisk.org/r/3723/#comment23472> 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. /team/group/rls/res/res_pjsip_pubsub.c <https://reviewboard.asterisk.org/r/3723/#comment23467> This is leaked when allocate_subscription() is called below. /team/group/rls/res/res_pjsip_pubsub.c <https://reviewboard.asterisk.org/r/3723/#comment23471> sub_tree leaked? /team/group/rls/res/res_pjsip_pubsub.c <https://reviewboard.asterisk.org/r/3723/#comment23470> sub_tree leaked? /team/group/rls/res/res_pjsip_pubsub.c <https://reviewboard.asterisk.org/r/3723/#comment23469> sub_tree was already added when it was allocated by allocate_subscription_tree() above. This just trashes the linked list. /team/group/rls/res/res_pjsip_pubsub.c <https://reviewboard.asterisk.org/r/3723/#comment23468> sub is leaked here. sub_tree may be leaked? /team/group/rls/res/res_pjsip_pubsub.c <https://reviewboard.asterisk.org/r/3723/#comment23477> Use ast_free instead of ast_free_ptr. You don't need to pass a pointer since the RAII_VAR() macro creates a local function that actually calls ast_free(). In fact, using ast_free() allows MALLOC_DEBUG to know where the block was actually freed. - rmudgett On Aug. 6, 2014, 10:44 a.m., Mark Michelson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3723/ > ----------------------------------------------------------- > > (Updated Aug. 6, 2014, 10:44 a.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
