[asterisk-dev] [Code Review]: Add update callback to dnsmgr since chan_sip peers need to be re-linked in peers_by_ip when the IP changes

[Mark Michelson]

> On Jan. 27, 2012, 11:16 a.m., Mark Michelson wrote:
> > /branches/1.8/channels/chan_sip.c, lines 30469-30475
> > <https://reviewboard.asterisk.org/r/1691/diff/2/?file=23637#file23637line30469>
> >
> >     I'll be the first to admit I don't know much about astobj1, but in astobj2 it's important to unref the object after you unlock it. Otherwise, you may destroy the object when you unref it, then when you try to unlock it, you'll crush things.
> >     
> >     I know that ASTOBJ_RDLOCK is just a mutex, and I also know that since astobj2 exists, there's no way that ASTOBJ_RDLOCK will ever actually get changed to use a read lock. However, I just have this awful sense of unease seeing RDLOCK here when you're quite clearly altering the data in iterator. Could you change it to WRLOCK?
> 
> Terry Wilson wrote:
>     Here, we should be guaranteed that we have a ref for the iterator (link in the container), and the ref for the dnsmgr and we are removing the one added for dnsmgr. Isn't this equivalent to the sip peer change above where I had the additional refs for the peer and was asked to remove them? The only place things are unlinked from the container is right after this statement.
>     
>     Yeah, the RDLOCK thing irritated me as well. I copied the code from elsewhere that did something similar with a RDLOCK. Since I didn't know much about astobj1, I figured I'd use the style that I saw. A more careful search shows other stuff using WRLOCK, so I'll just use that. :-)

I will begrudgingly agree that what you have is safe, as long as ASTOBJ iterators hold a reference to the containers in the same way that astobj2 iterators do.


- Mark


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


On Jan. 25, 2012, 4:16 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1691/
> -----------------------------------------------------------
> 
> (Updated Jan. 25, 2012, 4:16 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Asterisk's dnsmgr currently takes a pointer to an ast_sockaddr and updates it anytime an address resolves to something different. There are a couple of issues with this. First, the ast_sockaddr is usually the address of an ast_sockaddr inside a refcounted struct and we never bump the refcount of those structs when using dnsmgr. This makes it possible that a refresh could happen after the destructor for that object is called (despite ast_dnsmgr_release being called in that destructor). Second, the module using dnsmgr cannot be aware of an address changing without polling for it in the code. If an action needs to be taken on address update (like re-linking a SIP peer in the peers_by_ip table), then polling for this change negates many of the benefits of having dnsmgr in the first place.
> 
> This patch adds an update callback that is called if a pointer to a callback function is provided to ast_dnsmgr_lookup(). It also moves calls to ast_dnsmgr_release outside of the destructor functions and into cleanup functions that are called when we no longer need the objects and increments the refcount of the objects using dnsmgr since those objects are stored on the ast_dnsmgr_entry struct. A helper function for returning the proper default SIP port (non-tls vs tls) is also added and used.
> 
> The current implementation in the patch is backward compatible with code that doesn't use the update callback. Seeing as this is only two calls in chan_iax2, it might be better to just update chan_iax2 and insist on using the update callback. This would allow us to stop passing in the ast_sockaddr pointer altogether. I'll update the documentation of the dnsmgr functions (and CHANGES) after we decide on the aforementioned point.
> 
> This patch also builds on the registry fixes suggested Timo Teräs in issue ASTERISK-19106 re: the port number being 0 after a dnsmgr update.
> 
> 
> This addresses bug ASTERISK-19106.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19106
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_iax2.c 352519 
>   /branches/1.8/channels/chan_sip.c 352551 
>   /branches/1.8/include/asterisk/dnsmgr.h 352519 
>   /branches/1.8/main/dnsmgr.c 352519 
> 
> Diff: https://reviewboard.asterisk.org/r/1691/diff
> 
> 
> Testing
> -------
> 
> I have tested registration via register => x at peer and via peers defined with callbackextension by having the host=invalid.address and selectively defining invalid.address in /etc/hosts with and without dnsmgr enabled. Timo Teräs in ASTERISK-19106 also reports that it fixes the issues he was experiencing.
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120127/cfee0e4a/attachment-0001.htm>