<p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/9802">View Change</a></p><p>11 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c">File res/res_pjsip_registrar.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@131">Patch Set #1, Line 131:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">static int registrar_validate_contacts(const pjsip_rx_data *rdata, pj_pool_t *pool, struct ao2_container *contacts,<br> struct ast_sip_aor *aor, int *added, int *updated, int *deleted)<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">You need to either pass in the permanent contacts count or use the aor to determine the static contacts count line in register_aor_core().</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@151">Patch Set #1, Line 151:</a> <code style="font-family:monospace,monospace"> *deleted = ao2_container_count(contacts);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You need to remove the permanent contacts count from the returned deleted count.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@178">Patch Set #1, Line 178:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> existing = ao2_callback(contacts, 0, registrar_find_contact, &details);<br> ao2_cleanup(existing);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Permanent contacts should not affect the added, updated, deleted counts returned.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@263">Patch Set #1, Line 263:</a> <code style="font-family:monospace,monospace"> if (!path_str) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Pre-existing bug. We are checking the wrong thing.</p><p style="white-space: pre-wrap; word-wrap: break-word;">if (!*path_str) {<br>}</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@538">Patch Set #1, Line 538:</a> <code style="font-family:monospace,monospace"> "Contact Comparison", 256, 256);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should change the initial size to 1024 so we have a larger initial block to carve memory out of without allocating from the heap. Reseting the pool frees any increment and specially allocated large blocks that were allocated. Also the initial block has the pool overhead data automatically carved out of it.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@563">Patch Set #1, Line 563:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /* If there are any permanent contacts configured on the AOR we need to take them into<br> * account when counting contacts.<br> */<br> if (aor->permanent_contacts) {<br> permanent = ao2_container_count(aor->permanent_contacts);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is only used when we are not keeping track of existing contacts.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@578">Patch Set #1, Line 578:</a> <code style="font-family:monospace,monospace"> ast_sorcery_object_id_sort, ast_sorcery_object_id_compare);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why are you sorting the container?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@658">Patch Set #1, Line 658:</a> <code style="font-family:monospace,monospace"> contact = ao2_callback(contacts, OBJ_UNLINK, registrar_find_contact, &details);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">We'll unlink permanent contacts here too! We'll have to put it back into the container later.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@724">Patch Set #1, Line 724:</a> <code style="font-family:monospace,monospace"> } else if (expiration) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Since we may have unlinked a permanent contact we should add it back to the contacts container then go on to the expiration else-if.</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if (!contact) {<br>} else if (is_contact_static(contact) {<br> /* Put it back into the container */<br> ao2_link(contacts, contact);<br>} else if (expiration) {<br>...</pre></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@794">Patch Set #1, Line 794:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /* We reset the pool so we don't need to allocate additional blocks of memory on the next run */<br> pj_pool_reset(details.pool);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">There are several continue statements in this loop after the pool is used by the ao2_callback(registrar_find_contact) line. How about changing to a for loop and reset the pool in the increment statement? That way your comment is true in more cases.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@807">Patch Set #1, Line 807:</a> <code style="font-family:monospace,monospace"> * The contacts container holds the current contacst of the AOR.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/contacst/contacts/</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/9802">change 9802</a>. To unsubscribe, or for help writing mail filters, 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/9802"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I1a78b2d46f9a2045dbbff1a3fd6dba84b612b3cb </div>
<div style="display:none"> Gerrit-Change-Number: 9802 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 02 Aug 2018 20:17:09 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>