<p>George Joseph <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/6657">View Change</a></p><p>Patch set 1:</p><p>(2 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/6657/1/res/res_pjsip/pjsip_configuration.c">File res/res_pjsip/pjsip_configuration.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6657/1/res/res_pjsip/pjsip_configuration.c@1310">Patch Set #1, Line 1310:</a> <code style="font-family:monospace,monospace">             persistent->aors = ast_strdup(endpoint->aors);</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Move this outside the 'if (!persistant)' block and add an 'else'<br>block to free the existing aors?</p></blockquote><pre style="font-family: monospace,monospace; white-space: pre-wrap;">Yeah.  If you move it out, then, you can just...<br>if (!persistent->aors || strcmp(persistent->aors, endpoint->aors)) {<br>    ast_free(persistent->aors);<br>    persistent->aors = ast_strdup(endpoint->aors);</pre><p style="white-space: pre-wrap; word-wrap: break-word;">}</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;"><br>In that case if we fail to allocate the aors we'd have to unlink<br>from persistent_endspoints in addition to returning NULL?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">If the persistent endpoint exists then the endpoint already exists as well.  If the apply functions returns an error (via returning NULL here), the existing endpoint object is kept so we need to keep the original persistent endpoint as well.</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;"><br>persistant is allocated without a lock, do we need to lock anything<br>to make it safe to free the existing aors?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think we need to.  The endpoint itself is in flux at this point anyway so i'm not sure locking the persistent version just to update the pointer is necessary.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6657/1/res/res_pjsip/pjsip_configuration.c@1320">Patch Set #1, Line 1320:</a> <code style="font-family:monospace,monospace">  ao2_ref(persistent->endpoint, +1);</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">So maybe this should be moved into the 'if (!persistant)' block?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Hmmm, yeah otherwise we're bumping its ref even on an existing endpoint but you have to know whether this is an apply on an existing endpoint or a new endpoint.  Look at config_transport.  I can't ATM but IIRC it has logic to determine if it's a new object or an existine one.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/6657">change 6657</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/6657"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I59bbfc8da8a14d5f4af8c5bb1e71f8592ae823eb </div>
<div style="display:none"> Gerrit-Change-Number: 6657 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 06 Oct 2017 17:05:45 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>