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

[George Joseph]

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

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


Patch Set 2: Code-Review-1

(12 comments)

https://gerrit.asterisk.org/#/c/8604/2/include/asterisk/data_buffer.h
File include/asterisk/data_buffer.h:

https://gerrit.asterisk.org/#/c/8604/2/include/asterisk/data_buffer.h@36
PS2, Line 36: /*!
            :  * \brief The number of cached payloads a data buffer starts out with
            :  */
            : #define CACHED_PAYLOADS_START 2
Since it's used only internally by the data_buffer, this should probably be moved to data_buffer.c


https://gerrit.asterisk.org/#/c/8604/2/include/asterisk/data_buffer.h@56
PS2, Line 56: \param free_fn Callback function to free a data payload
Might need to allow a NULL callback so a caller can do their own memory management.


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

https://gerrit.asterisk.org/#/c/8604/2/main/data_buffer.c@38
PS2, Line 38:  vector
No vector.


https://gerrit.asterisk.org/#/c/8604/2/main/data_buffer.c@40
PS2, Line 40: data_buffer_payload
I'd rename this to payload_list_entry or something.  It's the wrapper for the payload, not the payload itself.


https://gerrit.asterisk.org/#/c/8604/2/main/data_buffer.c@44
PS2, Line 44: int
Since pos is an arbitrary number assigned by the caller it should probably be a size_t.


https://gerrit.asterisk.org/#/c/8604/2/main/data_buffer.c@70
PS2, Line 70: int
size_t


https://gerrit.asterisk.org/#/c/8604/2/main/data_buffer.c@136
PS2, Line 136: 	ast_assert(free_fn != NULL);
You should allow this to be NULL so a caller can do it's own memory management if it needs to.


https://gerrit.asterisk.org/#/c/8604/2/main/data_buffer.c@185
PS2, Line 185: 				buffer->free_fn(existing_payload->payload);
Check for NULL free_fn


https://gerrit.asterisk.org/#/c/8604/2/main/data_buffer.c@198
PS2, Line 198: int
size_t


https://gerrit.asterisk.org/#/c/8604/2/main/data_buffer.c@212
PS2, Line 212: 		buffer->free_fn(buffer_payload->payload);
Check for NULL free_fn


https://gerrit.asterisk.org/#/c/8604/2/main/data_buffer.c@254
PS2, Line 254: int
size_t


https://gerrit.asterisk.org/#/c/8604/2/main/data_buffer.c@284
PS2, Line 284: 		buffer->free_fn(buffer_payload->payload);
Check for NULL free_fn



-- 
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: 2
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-Comment-Date: Tue, 20 Mar 2018 11:47:27 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180320/98971c05/attachment-0001.html>