<p>Kevin Harwell <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/8604">View Change</a></p><p>Patch set 4:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p>(5 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c">File main/data_buffer.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c@138">Patch Set #4, Line 138:</a> <code style="font-family:monospace,monospace"> ast_free(existing_payload);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Whenever the data buffer entry is freed, it's contained payload may need release as well. You'll need to call free_fn here too.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Since this is done in a few places I'd recommend creating a function to handle freeing the entry and it contents.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c@141">Patch Set #4, Line 141:</a> <code style="font-family:monospace,monospace"> node++;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">No reason to continue incrementing node if node > size. Can wrap in an "else" or use a "continue" in the above "if".</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c@186">Patch Set #4, Line 186:</a> <code style="font-family:monospace,monospace"> ast_free(buffer_payload);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Another spot where the payload needs free_fn called.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c@228">Patch Set #4, Line 228:</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;"> ast_data_buffer_cache_adjust(buffer);<br> buffer_payload = data_buffer_payload_alloc(payload, pos);<br> if (!buffer_payload) {<br> return -1;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think this is wrong? The cache is adjusted which should grow it by 'n'. When it grows payloads are alloc'ed, so I think after adjusting you'd just want to grab it out of the cache instead of creating it here maybe?</p><p style="white-space: pre-wrap; word-wrap: break-word;">As is this adjust the cache then adds a new data buffer entry to the buffer list and the cache and data buffer are out of sync size wise (but maybe that is okay here).</p><p style="white-space: pre-wrap; word-wrap: break-word;">Or maybe just adjust it later after inserting?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c@260">Patch Set #4, Line 260:</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 (pos == -1) {<br> buffer_payload = AST_LIST_FIRST(&buffer->payloads);<br><br> if (buffer_payload) {<br> return buffer_payload->payload;<br> }<br> } else {<br> AST_LIST_TRAVERSE(&buffer->payloads, buffer_payload, list) {<br> if (buffer_payload->pos == pos) {<br> return buffer_payload->payload;<br> }<br> }<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Since this is suppose to act as a ring buffer that seems more like a queue to me. Should we just only be "getting" from the head and once retrieved, the head then becomes a cacheable item? Then we don't even need a separate list that acts as a cache.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Otherwise how do items retire and get put into the cache? Or I may have missed that.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/8604">change 8604</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/8604"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Iff13c5d4795d52356959fe2a57360cd57dfade07 </div>
<div style="display:none"> Gerrit-Change-Number: 8604 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Benjamin Keith Ford <bford@digium.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-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 21 Mar 2018 14:41:56 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>