[Asterisk-code-review] cdr mysql: run a full reconnect on log if not connected (asterisk[master])

[Kevin Harwell]

Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/7928 )

Change subject: cdr_mysql: run a full reconnect on log if not connected
......................................................................


Patch Set 1: Code-Review-1

(4 comments)

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

https://gerrit.asterisk.org/#/c/7928/1//COMMIT_MSG@12
PS1, Line 12: However it dos not run the full connection startup sequence and thus
s/dos/does


https://gerrit.asterisk.org/#/c/7928/1/addons/cdr_mysql.c
File addons/cdr_mysql.c:

https://gerrit.asterisk.org/#/c/7928/1/addons/cdr_mysql.c@176
PS1, Line 176: 		if (!((hostname || dbsock) && dbuser && password && dbname
             : 					&& dbtable) ) 
If I am reading the code right these values will always be non-null (unless an allocation failed on load). Probably the intention was to check if the ast_str actually had a value.

Also if these values are required for connection then it's probably better to check right after the configuration has been loaded. Maybe even rejecting the configuration (module decline on load, or keep old config in reload case).


https://gerrit.asterisk.org/#/c/7928/1/addons/cdr_mysql.c@188
PS1, Line 188: 		struct ast_config *cfg = ast_config_load(config, config_flags);
You are reloading the config here, but not updating the global configuration variables. I think either you can just call the module's reload here or if those values don't need to be set then not do a config load here.


https://gerrit.asterisk.org/#/c/7928/1/addons/cdr_mysql.c@622
PS1, Line 622: 	res |= my_load_config_string(cfg, "global", "hostname", &hostname, "localhost");
             : 	res |= my_load_config_string(cfg, "global", "dbname", &dbname, "astriskcdrdb");
             : 	res |= my_load_config_string(cfg, "global", "user", &dbuser, "root");
             : 	res |= my_load_config_string(cfg, "global", "sock", &dbsock, "");
             : 	res |= my_load_config_string(cfg, "global", "table", &dbtable, "cdr");
             : 	res |= my_load_config_string(cfg, "global", "password", &password, "");
             : 
             : 	res |= my_load_config_string(cfg, "global", "charset", &dbcharset, "");
             : 
             : 	res |= my_load_config_string(cfg, "global", "ssl_ca", &ssl_ca, "");
             : 	res |= my_load_config_string(cfg, "global", "ssl_cert", &ssl_cert, "");
             : 	res |= my_load_config_string(cfg, "global", "ssl_key", &ssl_key, "");
             : 
             : 	res |= my_load_config_number(cfg, "global", "port", &dbport, MYSQL_PORT);
             : 	res |= my_load_config_number(cfg, "global", "timeout", &timeout, 0);
             : 	res |= my_load_config_string(cfg, "global", "compat", &compat, "no");
             : 	res |= my_load_config_string(cfg, "global", "cdrzone", &cdrzone, "");
If you do end up keeping the config reload stuff and you call this on reconnect then I think these ast_str global variables are being leaked.



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7603c7501dae7070fac35081cf35161579c47590
Gerrit-Change-Number: 7928
Gerrit-PatchSet: 1
Gerrit-Owner: Tzafrir Cohen <tzafrir.cohen at xorcom.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Tzafrir Cohen <tzafrir.cohen at xorcom.com>
Gerrit-Comment-Date: Fri, 12 Jan 2018 23:38:01 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180112/2d91a1b1/attachment.html>