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

[Benjamin Keith Ford]

Benjamin Keith Ford 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@186
PS4, Line 186: 				ast_free(buffer_payload);
> Another spot where the payload needs free_fn called.
Here, the cache is the only thing being dealt with. Since all payloads are allocated as NULL in the cache, the only thing that needs to be freed is the payload_entry.


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'. W
The reason I put this here is because if for some reason the buffer is not at its max and we have 0 payloads in the cache (maybe it started with N payloads and the buffer max is N*2), allocating to the cache just to pull from it and put in the buffer doesn't make much sense - we may as well just allocate a payload and use it. But we do want to populate the cache again for future use, in this particular scenario. However it could have the wrong cache count now, since we haven't inserted the payload yet... The problem with putting it after an insert is it would attempt to adjust after every put, which would also ruin the point of CACHED_PAYLOADS_START. I'm open to suggestions here.


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 
Here, we want to be able to get a payload from any position, which could be any number (e.g., RTP sequencing numbers for the sake of RTP retransmission). The payloads never retire to the cache, but if the buffer reaches its max size and we attempt to do another put, it will pull the head from the list and recycle it as the new payload.



-- 
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 15:30:14 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180321/4d8bf307/attachment.html>