[Asterisk-code-review] res_pjsip_refer: Refactor progress locking and serialization (asterisk[16])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/15494 )

Change subject: res_pjsip_refer: Refactor progress locking and serialization
......................................................................


Patch Set 1: Code-Review-1

(3 comments)

https://gerrit.asterisk.org/c/asterisk/+/15494/1//COMMIT_MSG 
Commit Message:

https://gerrit.asterisk.org/c/asterisk/+/15494/1//COMMIT_MSG@20 
PS1, Line 20:   the serializer but did already held the dialog lock
s/held/hold/


https://gerrit.asterisk.org/c/asterisk/+/15494/1/res/res_pjsip_refer.c 
File res/res_pjsip_refer.c:

https://gerrit.asterisk.org/c/asterisk/+/15494/1/res/res_pjsip_refer.c@141 
PS1, Line 141: 	/* Send a deferred initial 100 Trying SIP frag NOTIFY if we haven't already. */
I think we have to move the unlock to before this comment.  I do not think we should be sending the NOTIFY message with the lock held.

The critical part of this function is determining if the subscription is still there.  It is also the critical part of the other function.  Thus the lock is the common mutex mechanism of the two.

Could do below in this function:

lock dialog
if (!sub) {
   ast_debug()
   unlock dialog
   return 0
}

if (notification->state == terminated) {
   ast_debug()
   pjsip_evsub_set_mod_data(sub, refer_progress_module.id, NULL)
   /* This is for dropping the reference on the subscription *?
   ao2_cleanup(notification->progress)
   notification->progress->sub = NULL
}
unlock dialog


https://gerrit.asterisk.org/c/asterisk/+/15494/1/res/res_pjsip_refer.c@317 
PS1, Line 317: 	/* Since it was unlocked it is possible for this to have been removed already, so check again */
             : 		if (pjsip_evsub_get_mod_data(sub, refer_progress_module.id)) {
             : 			pjsip_evsub_set_mod_data(sub, refer_progress_module.id, NULL);
             : 			ao2_cleanup(progress);
             : 		}
Since we are unlinking the subscription and did not release the lock when doing it, we do not need to test.  We can just do.

progress->sub = NULL;
pjsip_evsub_set_mod_data(sub, refer_progress_module.id, NULL);
ao2_cleanup(progress);



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/15494
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: I97a8bb01771a3c85345649b8124507f7622a8480
Gerrit-Change-Number: 15494
Gerrit-PatchSet: 1
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Tue, 23 Feb 2021 22:23:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210223/386b1c25/attachment-0001.html>