<p>Benjamin Keith Ford <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/8604">View Change</a></p><p>Patch set 4:</p><p>(3 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@186">Patch Set #4, Line 186:</a> <code style="font-family:monospace,monospace"> ast_free(buffer_payload);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Another spot where the payload needs free_fn called.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Here, the cache is the only thing being dealt with. Since all payloads are allocated as NULL in the cache, the only thing that needs to be freed is the payload_entry.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think this is wrong? The cache is adjusted which should grow it by 'n'. W</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The reason I put this here is because if for some reason the buffer is not at its max and we have 0 payloads in the cache (maybe it started with N payloads and the buffer max is N*2), allocating to the cache just to pull from it and put in the buffer doesn't make much sense - we may as well just allocate a payload and use it. But we do want to populate the cache again for future use, in this particular scenario. However it could have the wrong cache count now, since we haven't inserted the payload yet... The problem with putting it after an insert is it would attempt to adjust after every put, which would also ruin the point of CACHED_PAYLOADS_START. I'm open to suggestions here.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Since this is suppose to act as a ring buffer that seems more like a queue </blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Here, we want to be able to get a payload from any position, which could be any number (e.g., RTP sequencing numbers for the sake of RTP retransmission). The payloads never retire to the cache, but if the buffer reaches its max size and we attempt to do another put, it will pull the head from the list and recycle it as the new payload.</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 15:30:14 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>