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

Tue Oct 17 12:38:32 CDT 2017

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

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

Patch Set 1: Code-Review-1

(18 comments)

I raised this review on IRC, it was asked: why not just tail the cdr.csv file, transform, and put it into beanstalk?  I have no opinion on this, just sharing a question that was asked.

I believe this is a comprehensive review of technical issues with the current patch.  If you haven't already please take a look at the wiki [1] for instructions on updating this review (instead of abandoning and creating a new one).

[1] https://wiki.asterisk.org/wiki/display/AST/Gerrit+Usage#GerritUsage-UpdatingaReview

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

https://gerrit.asterisk.org/#/c/6819/1//COMMIT_MSG@16
PS1, Line 16: At the same time, I cleaned up some minor syntax correction inside
            : cdr_manager.c, per remarks by Corey Farrel
No need to credit reviewers in the commit message.

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

https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@22
PS1, Line 22:  *
Can we add a line referring to the source repository of the beanstalk client library this links against?  I assume it's https://github.com/deepfryed/beanstalk-client?  We'll need to know so we can verify the license is acceptable for linking in Asterisk.

https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@38
PS1, Line 38: 	<depend>beanstalk</depend>
Need to add a line to the MODULEINFO block:
<support_level>extended</support_level>

https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@42
PS1, Line 42: 	<managerEvent language="en_US" name="Cdr">
Sorry I didn't catch this before, this 'managerEvent' is only for cdr_manager.  I don't think this module needs an xml documentation block.

https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@239
PS1, Line 239: 
If someone has a config file with 'port=13001' but then removes the port setting and reloads, we probably want the port to be reset to default (13000) after the reload.  This is the place to do that.

This is also where we should set default allocated values for bs_host / bs_tube.

https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@245
PS1, Line 245:                 if (!newenablecdr) {
If someone has config:
[general]
host=mybeanstalkd
enabled=true

You will ignore the host setting because newenablecdr will not be true yet.  If someone sets 'enabled=false' but still sets host, the value of bs_host needs to be set per user config even though enablecdr will be false.

https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@251
PS1, Line 251:                         bs_host = ast_strdup(v->value);
This leaks.  We need to do a few things to deal with this leak.

1) Set/reset allocated default values above the while loop.
2) When we get a new "host" or "tube" value we need to ast_free the existing one before setting a new one.
3) ast_free config values during module_unload.

https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@255
PS1, Line 255:                         bs_tube = ast_strdup(v->value);
Leak (comment for bs_host leak).

https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@258
PS1, Line 258:                     }
Did we lose the option for setting customfields?

https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@263
PS1, Line 263:         ast_log(LOG_NOTICE, "Added beanstalkd server %s at port %d with tube %s", bs_host, bs_port, bs_tube);
This seems out of place.  Probably better to put it where we call ast_cdr_backend_unsuspend.  I'd suggest instead of "Added" say "Enabled".

https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@297
PS1, Line 297:     bs_socket = bs_connect(bs_host, bs_port);
The use of bs_host (and bs_tube just below) need to be protected by customfields_lock.  That lock would be more appropriately named 'config_lock' since it needs to prevent use after free for the configured strings.

https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@316
PS1, Line 316:     //cdr_buffer[0] = '\0';
Just delete this line, don't comment out.

https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@352
PS1, Line 352:     cdr_buffer = ast_json_dump_string(t_cdr_json);
You need to ast_json_unref(t_cdr_json) after you are done with it.

https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@354
PS1, Line 354:     /*
Delete this code instead of commenting out.

https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@376
PS1, Line 376:     bs_disconnect(bs_socket);
You need a call to ast_json_free(cdr_buffer) now that you're done with the value or this will be a major memory leak.

https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@385
PS1, Line 385:     if (customfields) {
ast_free is NULL safe so no need to check if it's NULL.  This would be the place to free bs_host and bs_tube as well.

https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@410
PS1, Line 410:     .support_level = AST_MODULE_SUPPORT_CORE,
This module will be AST_MODULE_SUPPORT_EXTENDED.

https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_manager.c
File cdr/cdr_manager.c:

PS1: 
Although I have no objection to these changes they are unrelated and should not be included in the commit with your new module.

-- 
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: 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: Tue, 17 Oct 2017 17:38:32 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171017/c352c38f/attachment.html>