[Asterisk-code-review] Add data buffer API to store packets. (asterisk[15])

[Matthew Fredrickson]

Matthew Fredrickson has posted comments on this change. ( https://gerrit.asterisk.org/8604 )

Change subject: Add data buffer API to store packets.
......................................................................


Patch Set 5: Code-Review-1

(3 comments)

One more round of thoughts.  It's looking better though :-)

https://gerrit.asterisk.org/#/c/8604/5/main/data_buffer.c
File main/data_buffer.c:

https://gerrit.asterisk.org/#/c/8604/5/main/data_buffer.c@224
PS5, Line 224: 	if (!buffer_payload) {
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.


https://gerrit.asterisk.org/#/c/8604/5/main/data_buffer.c@233
PS5, Line 233: 		if (existing_payload->pos > pos) {
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.


https://gerrit.asterisk.org/#/c/8604/5/main/data_buffer.c@299
PS5, Line 299: ast_data_buffer_cache_count
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.



-- 
To view, visit https://gerrit.asterisk.org/8604
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff13c5d4795d52356959fe2a57360cd57dfade07
Gerrit-Change-Number: 8604
Gerrit-PatchSet: 5
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Matthew Fredrickson <creslin at digium.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 16:12:18 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180322/bcc6f0f5/attachment-0001.html>