[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 7: Code-Review-1

(4 comments)

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

https://gerrit.asterisk.org/#/c/8604/7/main/data_buffer.c@103
PS7, Line 103: buffer->max - buffer->count
minor, but since this is calculated multiple times in this function you could do it once at the beginning. Also the assigned variable name could give descriptive meaning to the calculation.


https://gerrit.asterisk.org/#/c/8604/7/main/data_buffer.c@112
PS7, Line 112: 			buffer_payload = data_buffer_payload_alloc(NULL, -1);
             : 			if (buffer_payload) {
             : 				AST_LIST_INSERT_TAIL(&buffer->cached_payloads, buffer_payload, list);
             : 				buffer->cache_count++;
             : 			}
             : 
             : 			i++;
If the entry does not get inserted into the cache then "i" still gets incremented, thus potentially not adding the desired number of entries. Might be easiest to just drop the "i" usage and key off "cache_count" only?


https://gerrit.asterisk.org/#/c/8604/7/main/data_buffer.c@125
PS7, Line 125: 			buffer_payload = AST_LIST_REMOVE_HEAD(&buffer->cached_payloads, list);
             : 			if (buffer_payload) {
             : 				ast_free(buffer_payload);
             : 				buffer->cache_count--;
             : 			}
             : 
             : 			i--;
Similar here, to the above. Unlikely, but possible for the number of items removed to be less than expected potentially resulting in a cache > max_size.


https://gerrit.asterisk.org/#/c/8604/7/main/data_buffer.c@178
PS7, Line 178: 			if (remove) {
I would only remove items from the cache if the current cache size is greater than size. Mind as well keep already allocated cached objects unless the max_size of the buffer shrinks to less than the current cache size.



-- 
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: 7
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: Fri, 23 Mar 2018 16:30:43 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180323/41864f01/attachment.html>