[Asterisk-code-review] This is the actual commit... didn't add the new files in... (asterisk[master])

[Corey Farrell]

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

Change subject: This is the actual commit... didn't add the new files in...
......................................................................


Patch Set 1: Code-Review-1

(14 comments)

https://gerrit.asterisk.org/#/c/6816/1//COMMIT_MSG
Commit Message:

https://gerrit.asterisk.org/#/c/6816/1//COMMIT_MSG@7
PS1, Line 7: This is the actual commit... didn't add the new files in...
The commit message needs to say what we're doing.  Maybe:
cdr: Add support for beanstalkd backend.


https://gerrit.asterisk.org/#/c/6816/1/cdr/cdr_beanstalkd.c
File cdr/cdr_beanstalkd.c:

https://gerrit.asterisk.org/#/c/6816/1/cdr/cdr_beanstalkd.c@4
PS1, Line 4:  * Copyright (C) 2004 - 2005
2017


https://gerrit.asterisk.org/#/c/6816/1/cdr/cdr_beanstalkd.c@202
PS1, Line 202:     char *cat = NULL;
No need to initialize cat.


https://gerrit.asterisk.org/#/c/6816/1/cdr/cdr_beanstalkd.c@241
PS1, Line 241:                 if (!strcasecmp(v->name, "enabled"))
Need brackets for each conditional block.  Also the remaining options should be chained together with 'else if' blocks so we stop comparing v->name once we find a match.


https://gerrit.asterisk.org/#/c/6816/1/cdr/cdr_beanstalkd.c@244
PS1, Line 244:                 if (!newenablecdr) {
This will disable cdr_beanstalkd if 'enabled=true' is not the first option in the config file.  This block should just be removed, you already do the right thing with newenablecdr after you destroy the config.


https://gerrit.asterisk.org/#/c/6816/1/cdr/cdr_beanstalkd.c@282
PS1, Line 282:     char strStartTime[80] = "";
This is unconditionally set below so it doesn't need to be initialized here.


https://gerrit.asterisk.org/#/c/6816/1/cdr/cdr_beanstalkd.c@284
PS1, Line 284:     char strEndTime[80] = "";
This is unconditionally set below so it doesn't need to be initialized here.


https://gerrit.asterisk.org/#/c/6816/1/cdr/cdr_beanstalkd.c@288
PS1, Line 288:     if (!enablecdr)
Brackets


https://gerrit.asterisk.org/#/c/6816/1/cdr/cdr_beanstalkd.c@291
PS1, Line 291:     int bs_id, bs_socket = bs_connect(bs_host, bs_port);
All variable declarations are required to be at the top of the code block.

Also we prefer that each variable get it's own line and statement:
int bs_id;
int bs_socket;

Do we need to check the result of bs_connect or will bs_use react correctly to bs_socket==-1?


https://gerrit.asterisk.org/#/c/6816/1/cdr/cdr_beanstalkd.c@325
PS1, Line 325:     snprintf(cdr_buffer, BEANSTALK_JOB_SIZE,
snprintf won't encode quotes (") for you, so if any field contains a quote it will not be valid JSON. I suspect you will need to use ast_json_pack and ast_json_dump_string.


https://gerrit.asterisk.org/#/c/6816/1/cdr/cdr_beanstalkd.c@339
PS1, Line 339:     if (bs_id > 0)
Brackets


https://gerrit.asterisk.org/#/c/6816/1/cdr/cdr_beanstalkd.c@353
PS1, Line 353:     if (customfields)
Brackets.


https://gerrit.asterisk.org/#/c/6816/1/cdr/cdr_beanstalkd.c@377
PS1, Line 377: .support_level = AST_MODULE_SUPPORT_CORE,
             : .load = load_module,
             : .unload = unload_module,
             : .reload = reload,
             : .load_pri = AST_MODPRI_CDR_DRIVER,
Please tab indent these lines.


https://gerrit.asterisk.org/#/c/6816/1/configs/samples/cdr_beanstalkd.conf.sample
File configs/samples/cdr_beanstalkd.conf.sample:

PS1: 
The sample config should comment-out all options.



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b7d2964ae5b39a35ec23da460a725e6c0d7c214
Gerrit-Change-Number: 6816
Gerrit-PatchSet: 1
Gerrit-Owner: Nir Simionovich (GreenfieldTech - Israel) <nirs at greenfieldtech.net>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Comment-Date: Mon, 16 Oct 2017 17:06:44 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171016/7599bf68/attachment.html>