[Asterisk-code-review] This patch adds a beanstalk CDR backend. (asterisk[master])

[Corey Farrell]

Corey Farrell has posted comments on this change. ( https://gerrit.asterisk.org/6819 )

Change subject: This patch adds a beanstalk CDR backend.
......................................................................


Patch Set 7: Code-Review-1

(7 comments)

https://gerrit.asterisk.org/#/c/6819/7/cdr/cdr_beanstalkd.c
File cdr/cdr_beanstalkd.c:

PS7: 
I did not look clearly enough before, I just realized this whole file is space indented.  Coding guidelines [1] call for all indentation to be done with tabs.  One thing about the wiki page don't use ASTERISK_FILE_VERSION.  At this point that only applies to Asterisk 13.

[1] https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines


https://gerrit.asterisk.org/#/c/6819/7/cdr/cdr_beanstalkd.c@42
PS7, Line 42: 	<support_level>exended</support_level>
Missing the 't' in extended.


https://gerrit.asterisk.org/#/c/6819/7/cdr/cdr_beanstalkd.c@117
PS7, Line 117:     bs_host = DEFAULT_BEANSTALK_HOST;
> bs_host / bs_tube have to be allocated:
 > bs_host = ast_strdup(DEFAULT_BEANSTALK_HOST);
 > 
 > Otherwise when you eventually call ast_free it will segfault.

You missed this for bs_host and bs_tube.


https://gerrit.asterisk.org/#/c/6819/7/cdr/cdr_beanstalkd.c@166
PS7, Line 166:     char strAnswerTime[80];
strAnswerTime does have to be pre-initialized because it's only set conditionally (my previous finding was for the other variables).


https://gerrit.asterisk.org/#/c/6819/7/cdr/cdr_beanstalkd.c@241
PS7, Line 241:     if (bs_host) {
You don't need to check if a variable is set to call ast_free.  It's easier to reach if you don't use the conditional.


https://gerrit.asterisk.org/#/c/6819/7/cdr/cdr_beanstalkd.c@245
PS7, Line 245:     if (bs_tube) {
This one too.


https://gerrit.asterisk.org/#/c/6819/7/cdr/cdr_beanstalkd.c@270
PS7, Line 270:     .support_level = AST_MODULE_SUPPORT_CORE,
Support level AST_MODULE_SUPPORT_EXTENDED.



-- 
To view, visit https://gerrit.asterisk.org/6819
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5fe4089a34ab3b39230786d9bbfddafa56715f48
Gerrit-Change-Number: 6819
Gerrit-PatchSet: 7
Gerrit-Owner: Nir Simionovich (GreenfieldTech - Israel) <nirs at greenfieldtech.net>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Nir Simionovich (GreenfieldTech - Israel) <nirs at greenfieldtech.net>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
Gerrit-Comment-Date: Wed, 18 Oct 2017 00:28:27 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171017/70cbdcdc/attachment-0001.html>