Summary:	ASTERISK-25950: [patch]SIP channel does not send PeerStatus events for autocreated peers
Reporter:	Kirill Katsnelson (kkm)	Labels:
Date Opened:	2016-04-21 23:04:44	Date Closed:	2016-04-29 14:30:26
Priority:	Major	Regression?	Yes
Status:	Closed/Complete	Components:	Channels/chan_sip/General Channels/chan_sip/Registration
Versions:	12.0.0 13.0.0 13.8.2	Frequency of Occurrence	Constant
Related Issues:
Environment:	Any	Attachments:
Description:	Autocreated peers on the SIP channel never send any PeerStatus events. Discovered while trying to upgrade from 1.8.8. The reason is that autocreated peers are, well, autocreated using the temp_peer() function. There is a stanza in register_verify() which currently (13.8) looks like {code}    if (!peer && sip_cfg.autocreatepeer != AUTOPEERS_DISABLED) {        /* Create peer if we have autocreate mode enabled */        peer = temp_peer(name);        if (peer) { {code} Since the transition to Stasis endpoints for reporting, an endpoint is initialized in the function build_peer(), but not in temp_peer(). So the autocreated peers do not have their `sip_peer.endpoint` field set. The temp_peer is a horrific misnomer, because those peers are just good normal peers, only autocreated. They are not inferior in any sense. I am mentioning that because next comes the commit 0b83761, AKA SVN changeset r394795, which attempted to fix crashes caused by the unset reporting endpoint--by simply disabling all events from these peers! I would not be surprised if the author was confused by the name of the function and implemented an incorrect fix. The feature has been broken since the introduction of Stasis (12.0?). Since  temp_peer() has been also recruited to create an authentication "(Bogus peer)" (which I do not fully understand), it is probably more correct to just allocate an endpoint for autocreated peers at the site of the call to temp_peer in the above code snippet. Additionally, rolling back 0b83761 is also an option, as long as it can be proven that the "(Bogus peer)" never causes any Stasis communication.
Comments:	By: Asterisk Team (asteriskteam) 2016-04-21 23:04:45.038-0500 Thanks for creating a report! The issue has entered the triage process. That means the issue will wait in this status until a Bug Marshal has an opportunity to review the issue. Once the issue has been reviewed you will receive comments regarding the next steps towards resolution. A good first step is for you to review the [Asterisk Issue Guidelines|https://wiki.asterisk.org/wiki/display/AST/Asterisk+Issue+Guidelines] if you haven't already. The guidelines detail what is expected from an Asterisk issue report. Then, if you are submitting a patch, please review the [Patch Contribution Process|https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process]. By: Kirill Katsnelson (kkm) 2016-04-22 02:36:16.639-0500 I have a working fix for that on our QA system. I'll try to wrap my old brain around Gerrit and send in a patch. By: Rusty Newton (rnewton) 2016-04-22 16:56:50.443-0500 Thanks [~kkm] By: Kirill Katsnelson (kkm) 2016-04-25 12:53:48.921-0500 Rusty, thank you for the quick triage! I was away from Asterisk development for a few years and never used Gerrit before. I submitted a patch against the master branch, and my idea is to send versions against the 12 and 13 branches after the review (if accepted) in case there are changes--is that the right way to proceed? By: Richard Mudgett (rmudgett) 2016-04-25 13:47:45.424-0500 v12 is EOL and no longer receives any patches. https://wiki.asterisk.org/wiki/display/AST/Asterisk+Versions By: Kirill Katsnelson (kkm) 2016-04-25 15:05:59.879-0500 Ah, I see, thanks for the link. So it's currently should be 11 (until October) and 13, correct? PS Edit: As a general question--this particular bug does not affect V11. I am a bit lost in the Asterisk Git tree. By: Richard Mudgett (rmudgett) 2016-04-25 16:35:00.692-0500 When Asterisk migrated from SVN to Git we moved the release branches over: svn/asterisk/trunk -> origin/master svn/asterisk/branches/11 -> origin/11 svn/asterisk/branches/13 -> origin/13 and so on for other release branches Similarly for Asterisk certified release branches though you shouldn't need to worry about them: svn/asterisk/certified/branches/11.6 -> origin/certified/11.6 Patches are only applied to applicable branches according to the policy for the branch.  For this issue, you should put a patch up for the v13 and master branches.  Gerrit helps with this for fairly simple patches.  You simply use the cherry-pick button on the first review and specify which branch the patch should be cherry-picked to: 13 or master. https://wiki.asterisk.org/wiki/display/AST/Gerrit+Usage By: Kirill Katsnelson (kkm) 2016-04-25 16:47:32.289-0500 I see, thanks Richard! I think there are couple of things not really covered in that Wiki page, or I am missing them: 1. Should I do anything besides uploading the review request to Gerrit to have it eventually looked at? (I already did). 2. Should I preferably cherry pick to target branches after the review is done? By: Richard Mudgett (rmudgett) 2016-04-25 17:28:19.214-0500 # You do need to publish the review if you put it up as a draft.  If there are any findings then you need to correct them and put the review up again.  Git allows you to modify commits locally until the patch is how you want it.  Then you put the review up again.  The gerrit change-id is used by gerrit to associate the patch with the specific review and should not be changed between resubmissions.  Once a patch is accepted and merged into the release branch it cannot be altered any more. # There is a separate review for each branch.  It helps if you cherry pick the review to the appropriate branches.  That way the reviews will go into the branches a the same time. https://wiki.asterisk.org/wiki/display/AST/Commit+Messages There are plenty of review examples up on gerrit. By: Kirill Katsnelson (kkm) 2016-04-25 17:35:46.122-0500 I am very comfortable with Git, it's only the Gerrit part that is new to me. I just cherry-picked the change to the 13 branch in Gerrit, and as far as I can tell the review request in not a draft. So looks like it's all done right. Thanks for your help! By: Kirill Katsnelson (kkm) 2016-04-29 14:30:26.368-0500 Merged into master and 13.