<p>Matthew Fredrickson <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/8604">View Change</a></p><p>Patch set 5:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p style="white-space: pre-wrap; word-wrap: break-word;">One more round of thoughts.  It's looking better though :-)</p><p>(3 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/8604/5/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/5/main/data_buffer.c@224">Patch Set #5, Line 224:</a> <code style="font-family:monospace,monospace">       if (!buffer_payload) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm still not sure I get why this conditional is necessary - it seems like either we should be reusing the last entry (if it's full) or pulling from the cache (if it's not full).  If we hit this conditional, it seems like an assumption about how the code is supposed to work is broken.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8604/5/main/data_buffer.c@233">Patch Set #5, Line 233:</a> <code style="font-family:monospace,monospace">           if (existing_payload->pos > pos) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Shouldn't we be doing a duplicate packet check here, just in case?  For example, if we get a duplicate seqno coming in, we probably don't want to put the duplicate instance into the queue, it should instead be dropped.  We should also probably have a debug message of some sort be emitted.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8604/5/main/data_buffer.c@299">Patch Set #5, Line 299:</a> <code style="font-family:monospace,monospace">ast_data_buffer_cache_count</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">We should probably make this function static as well, and not expose it via the public API since the consumer shouldn't know about the internals of the caching system.</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: 5 </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-Reviewer: Matthew Fredrickson <creslin@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 22 Mar 2018 16:12:18 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>