<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1691/">https://reviewboard.asterisk.org/r/1691/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 25th, 2012, 3:24 p.m., <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I've made some comments below for specific issues in the SIP code.
More importantly though is that you're proposing this as a 1.8 change. It introduces an API change, which is a no-no. One way that you could alleviate this problem is by amending your patch to be an API addition instead of an API change. Add a function to dnsmgr.h to add a callback to a dnsmgr struct. This would have the benefit of not overloading the functionality of ast_dnsmgr_lookup, and you would not have to patch chan_iax2.c at all.</pre>
</blockquote>
<p>On January 25th, 2012, 4:08 p.m., <b>Terry Wilson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Adding an API change in something that is only used in two files seemed like it might be doable--especially in a feature that is disabled by default anyway. As I mentioned, I'd really like to remove passing in the addr at all. Just updating the address willy-nilly is something I consider a bug in the API.
If we can't (destructively) modify the API, I'll probably just make a new API lookup function that doesn't take an addr and instead takes callback and data pointers. Opinions?</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Yes, I think that's probably the best way to handle this.</pre>
<br />
<p>- Kevin</p>
<br />
<p>On January 25th, 2012, 4:16 p.m., Terry Wilson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Terry Wilson.</div>
<p style="color: grey;"><i>Updated Jan. 25, 2012, 4:16 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I have tested registration via register => x@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.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-19106">ASTERISK-19106</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/1.8/channels/chan_iax2.c <span style="color: grey">(352519)</span></li>
<li>/branches/1.8/channels/chan_sip.c <span style="color: grey">(352551)</span></li>
<li>/branches/1.8/include/asterisk/dnsmgr.h <span style="color: grey">(352519)</span></li>
<li>/branches/1.8/main/dnsmgr.c <span style="color: grey">(352519)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1691/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>