[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 4:

(3 comments)

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

https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c@228
PS4, Line 228: 		ast_data_buffer_cache_adjust(buffer);
             : 		buffer_payload = data_buffer_payload_alloc(payload, pos);
             : 		if (!buffer_payload) {
             : 			return -1;
             : 		}
> If you call the adjust function every time it will only adjust if the sizes
I'm starting to think that the cache might not be useful as well.  If we're not ever retiring objects to the cache, it seems to have little point in existing other than for startup memory allocation wait time (or resize allocation wait time).


https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c@237
PS4, Line 237: 		if (existing_payload->pos > pos) {
I suppose that dealing with RTP seqno wrapping will be something that is dealt with outside of this code?


https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c@260
PS4, Line 260: 	if (pos == -1) {
> Good catch. I was considering changing it to a 0 check, but I'm not sure if
NULL is 0 also.  0 will probably be a valid value in some cases, so you're correct in being concerned about using it.  You could add an additional function that grabs from the head of the list instead, if it's important enough.  I'd target our initial use case though - if we don't have a reason to flexibly grab both by pos and from the list head, I'd pick the one we're planning on using first.



-- 
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: 4
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: Wed, 21 Mar 2018 19:40:39 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180321/04a500b2/attachment-0001.html>