Re: [asterisk-dev] [Code Review] 4515: build_peer peer mailbox management bug
On March 20, 2015, 4:40 p.m., Corey Farrell wrote: /trunk/channels/chan_sip.c, line 30264 https://reviewboard.asterisk.org/r/4515/diff/1/?file=72691#file72691line30264 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. My understanding of the code is that build_peer is only called with devstate_only=true for realtime peers not already cached in the peers table (rtcachefriends=yes) and the state of the peer is being checked via sip_devicestate [1]. So firstpass=false should only happen for realtime peers that had been semi-built (peer exists and peer-the_mark=false) as the result of a sip_devicestate call. [1] sip_unregister also calls with devstate_only=true, but it also sets realtime=false so realtime_peer will not be called. - gareth --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4515/#review14755 --- On March 20, 2015, 4:02 p.m., gareth wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4515/ --- (Updated March 20, 2015, 4: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 -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4515: build_peer peer mailbox management bug
--- 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 -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4515: build_peer peer mailbox management bug
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4515/ --- 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 -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
