[Asterisk-code-review] mwi_subscribe_replaces_unsolicited: (...asterisk[13])

[George Joseph]

George Joseph has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/11628 )

Change subject: mwi_subscribe_replaces_unsolicited:
......................................................................


Patch Set 1: Code-Review-1

(8 comments)

https://gerrit.asterisk.org/#/c/11628/1//COMMIT_MSG 
Commit Message:

https://gerrit.asterisk.org/#/c/11628/1//COMMIT_MSG@7 
PS1, Line 7: mwi_subscribe_replaces_unsolicited:
Commit title should be the name of the module plus a short description of the fix.
Something like:
res_pjsip_mwi:  Fix multiple NOTIFYs with mwi_subscribe_replaces_unsolicited


https://gerrit.asterisk.org/#/c/11628/1//COMMIT_MSG@9 
PS1, Line 9: Fixes an bug whereby multiple NOTIFYs were replied back to endpoint when a
           : REGISTER was received.  This occurred when parameter
           : mwi_subscribe_replaces_unsolicited is set to yes in pjsip.conf.
           : The underlying problem here is that the
           : endpoint_receives_unsolicited_mwi_for_mailbox() function in res/res_pjsip_mwi.c
           : is sort of an overloaded function. When invoked from mwi_validate_for_aor() you
           : DO want to have the subscribe_replaces_unsolicited() logic occur whereby it
           : unsubscribes from stasis. When invoked from
           : create_mwi_subscriptions_for_endpoint you DO NOT want to have the
           : subscribe_replaces_unsolicited logic occur.The function should have a parameter
           : added to it which determines whether the logic is invoked or not.
           : The bug itself occurs because there are multiple mwi_subscription structures
           : for the mailbox, the logic which sends the NOTIFY when a REGISTER occurs goes
           : through all looking for mwi_subscription structures with the AOR. It finds
           : these multiple structures, calculates message count on them (despite having
           : no stasis subscriptions), and sends a NOTIFY.
           : Note: This fix does not restore back unsolicted NOTIFYs in the event
           : all subscriptions on a given AOR go away.  It was decided to create
           : a new issue for that.
           : 
Adding a few blank lines will make the message easier to read.


https://gerrit.asterisk.org/#/c/11628/1/res/res_pjsip_mwi.c 
File res/res_pjsip_mwi.c:

https://gerrit.asterisk.org/#/c/11628/1/res/res_pjsip_mwi.c@a501 
PS1, Line 501: 
Why was this blank line removed?


https://gerrit.asterisk.org/#/c/11628/1/res/res_pjsip_mwi.c@a713 
PS1, Line 713: 
Why was this blank line removed?


https://gerrit.asterisk.org/#/c/11628/1/res/res_pjsip_mwi.c@683 
PS1, Line 683: 	if (sub->is_subscribe_replaces_unsolicited) {
Is this actually needed?  Wouldn't "is_solicited" be set on this subscription?


https://gerrit.asterisk.org/#/c/11628/1/res/res_pjsip_mwi.c@757 
PS1, Line 757: invokedByValidate
We don't use camel case identifiers.  How about "handle_replace_unsolicited" since that's what it does?


https://gerrit.asterisk.org/#/c/11628/1/res/res_pjsip_mwi.c@761 
PS1, Line 761:  
whitespace


https://gerrit.asterisk.org/#/c/11628/1/res/res_pjsip_mwi.c@787 
PS1, Line 787: 				ao2_cleanup(mwi_stasis);
I think this needs to be outside the "if invoked" otherwise mwi_stasis will be leaked.



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

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: I8f77c33b9b4d374d510aa5ecd4f700a77107d8d4
Gerrit-Change-Number: 11628
Gerrit-PatchSet: 1
Gerrit-Owner: Christian Savinovich <csavinovich at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Comment-Date: Fri, 26 Jul 2019 13:21:58 +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/20190726/9f149fd5/attachment.html>