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

[Kevin Harwell]

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

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


Patch Set 4: Code-Review-1

(5 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@138
PS4, Line 138: 				ast_free(existing_payload);
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.

Since this is done in a few places I'd recommend creating a function to handle freeing the entry and it contents.


https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c@141
PS4, Line 141: 			node++;
No reason to continue incrementing node if node > size. Can wrap in an "else" or use a "continue" in the above "if".


https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c@186
PS4, Line 186: 				ast_free(buffer_payload);
Another spot where the payload needs free_fn called.


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;
             : 		}
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?

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).

Or maybe just adjust it later after inserting?


https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c@260
PS4, Line 260: 	if (pos == -1) {
             : 		buffer_payload = AST_LIST_FIRST(&buffer->payloads);
             : 
             : 		if (buffer_payload) {
             : 			return buffer_payload->payload;
             : 		}
             : 	} else {
             : 		AST_LIST_TRAVERSE(&buffer->payloads, buffer_payload, list) {
             : 			if (buffer_payload->pos == pos) {
             : 				return buffer_payload->payload;
             : 			}
             : 		}
             : 	}
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.

Otherwise how do items retire and get put into the cache? Or I may have missed that.



-- 
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-Comment-Date: Wed, 21 Mar 2018 14:41:56 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180321/d59660e3/attachment-0001.html>