----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3591/#review12096 -----------------------------------------------------------
I don't see the CHANGES file edits. And I now see why this module is in extended support. I added a couple of remarks about issues that were already there before you came. /trunk/cdr/cdr_pgsql.c <https://reviewboard.asterisk.org/r/3591/#comment22113> I meant one declaration per line. static char *pghostname; static char *pgdbname; ... (No need for =NULL, since static storage is zeroed by default.) /trunk/cdr/cdr_pgsql.c <https://reviewboard.asterisk.org/r/3591/#comment22114> If you add an assertion here that: conn == NULL you will notice that memory leaks when module -reloading cdr_pgsql. It's not freed in config_module. /trunk/cdr/cdr_pgsql.c <https://reviewboard.asterisk.org/r/3591/#comment22115> No need to set this to NULL since this stack is about to get cleaned up anyway. /trunk/cdr/cdr_pgsql.c <https://reviewboard.asterisk.org/r/3591/#comment22117> This could use an extra if(conn){} and a conn=NULL. /trunk/cdr/cdr_pgsql.c <https://reviewboard.asterisk.org/r/3591/#comment22116> Missing PQfinish(conn) and conn=NULL here. /trunk/res/res_config_pgsql.c <https://reviewboard.asterisk.org/r/3591/#comment22121> You touched these lines. Please add braces {}. /trunk/cel/cel_pgsql.c <https://reviewboard.asterisk.org/r/3591/#comment22118> Same as above. /trunk/cel/cel_pgsql.c <https://reviewboard.asterisk.org/r/3591/#comment22119> No need for conn_info = NULL here. /trunk/cel/cel_pgsql.c <https://reviewboard.asterisk.org/r/3591/#comment22112> This lacks a PQfinish + conn=NULL for consistency with the other code. /trunk/configs/res_pgsql.conf.sample <https://reviewboard.asterisk.org/r/3591/#comment22120> Space between ';' and 'Client'. What happens if you use a variable with spaces in it? I bet things will break. A comment stating that you shouldn't do that should be enough for now. And, if I google "client application name postgres", I don't get any hits that describe the feature. "Postgres application_name support (optional)" is probably a better description. These comments apply to the other two files as well. - wdoekes On June 6, 2014, 7:56 p.m., doome wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3591/ > ----------------------------------------------------------- > > (Updated June 6, 2014, 7:56 p.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-23737 > https://issues.asterisk.org/jira/browse/ASTERISK-23737 > > > Repository: Asterisk > > > Description > ------- > > See https://issues.asterisk.org/jira/browse/ASTERISK-23737 > > > Diffs > ----- > > /trunk/res/res_config_pgsql.c 415300 > /trunk/configs/res_pgsql.conf.sample 415300 > /trunk/configs/cel_pgsql.conf.sample 415300 > /trunk/configs/cdr_pgsql.conf.sample 415300 > /trunk/cel/cel_pgsql.c 415300 > /trunk/cdr/cdr_pgsql.c 415300 > > Diff: https://reviewboard.asterisk.org/r/3591/diff/ > > > Testing > ------- > > Used asterisk with the config paramterer pgdbappname and dbappname in > cdr_pgsql.conf and res_config_pgsql.conf respectively, enabled and disabled. > Worked as expected. > > > Thanks, > > doome > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
