[Asterisk-code-review] res pjsip pubsub: Add more locking and checking around NOTIFY (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: res_pjsip_pubsub: Add more locking and checking around NOTIFY
......................................................................


Patch Set 4: Code-Review-1

(8 comments)

Looks OK other than minor findings.

https://gerrit.asterisk.org/#/c/3018/4/res/res_pjsip_pubsub.c
File res/res_pjsip_pubsub.c:

Line 2271: 	int force_full_state;
This is never set to anything other than zero and can be eliminated.


PS4, Line 2280: 	/* It's possible that between when the notification was scheduled
              : 	 * and now, that a new SUBSCRIBE arrived, requiring full state to be
              : 	 * sent out in an immediate NOTIFY. If that has happened, we need to
              : 	 * bail out here instead of sending the batched NOTIFY.
              : 	 */
This comment block needs to remain associated with the next if test.  (if notify_params->scheduled...)


Line 2318: 	ast_sip_push_task(notify_params->sub_tree->serializer, serialized_send_notify, notify_params);
Push task can fail so we need to drop the notify_params ref if that happens.


Line 2337: 	notify_params->sub_tree->send_scheduled_notify = 1;
This is pre-existing:  Setting send_scheduled_notify after scheduling is a race condition because sched_cb could execute before you can set the flag here.  This is more likely to happen if notification_batch_interval is 1.

It is best to set the flag before scheduling and clear it on failure to schedule.  The dialog lock protects against other threads messing with the flag.


Line 2341: static void send_notify_params_destructor(void *data) {
Curly position.


Line 2372: 	notify_params = ao2_alloc(sizeof(*notify_params), send_notify_params_destructor);
Lock not needed with this ao2 object.


Line 3427:  * step on each others' toes. The dialog lock NOT held when this
s/NOT/is NOT/


PS4, Line 3469: 	/* Remove evsub's reference to the sub_tree */
              : 	ao2_ref(sub_tree, -1);
              : 	pjsip_dlg_dec_lock(sub_tree->dlg);
For safety I think you should unlock before unreffing in case this is the last ref to sub_tree.


-- 
To view, visit https://gerrit.asterisk.org/3018
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I779d11802cf672a51392e62a74a1216596075ba1
Gerrit-PatchSet: 4
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes