[asterisk-dev] [Code Review] 4515: build_peer peer mailbox management bug

[Corey Farrell]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4515/#review14755
-----------------------------------------------------------



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/4515/#comment25337>

    This could result in a change of behaviour.  In build_peer 'first_pass' will no longer decide if the existing mailboxes get cleared, it would only be done when '!dev_state'.
    
    I feel that a safer change would be to just remove the delme variable, let the current logic live on.  I could be convinced otherwise if you can show that the current logic produces the wrong results, but even then we'd have to very careful.


- Corey Farrell


On March 19, 2015, 11:02 p.m., gareth wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4515/
> -----------------------------------------------------------
> 
> (Updated March 19, 2015, 11:02 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24871
>     https://issues.asterisk.org/jira/browse/ASTERISK-24871
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> During a reload, build_peer iterates over the peer's mailboxes and tags them for removal via the delme variable. It adds any new, unique mailboxes to the peer via add_peer_mailboxes and then removes any mailboxes with delme still set.
> 
> However, there isn't any code to unset delme, so this would remove any previously configured mailboxes.
> 
> That is not what happens though because build_peer also calls set_peer_defaults which clears out all of the configured mailboxes using clear_peer_mailboxes making the setting of delme redundant.
> 
> So in the end there is no impact to the user because all the configured mailboxes get added regardless.
> 
> Patch unsets delme for existing, still-configured mailboxes in add_peer_mailboxes and removes call to clear_peer_mailboxes.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 433198 
> 
> Diff: https://reviewboard.asterisk.org/r/4515/diff/
> 
> 
> Testing
> -------
> 
> Added new mailboxes to peer, reloaded chan_sip and verified that existing mailboxes were still there and new mailboxes had been added.
> 
> Removed mailboxes from peer, reloaded chan_sip and verified that those mailboxes were no longer assigned to peer.
> 
> 
> Thanks,
> 
> gareth
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150320/b4f1d3c4/attachment-0001.html>