[asterisk-commits] jrose: branch 10 r354270 - in /branches/10: ./ cdr/cdr_pgsql.c
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Tue Feb 7 09:19:56 CST 2012
Author: jrose
Date: Tue Feb 7 09:19:51 2012
New Revision: 354270
URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=354270
Log:
Fix column duplication bug in module reload for cdr_pgsql.
Prior to this patch, attempts to reload cdr_pgsql.so would cause the column list to keep
its current data and then add a second copy during the reload. This would cause attempts
to log the CDR to the database to fail. This patch also cleans up some unnecessary null
checks for ast_free and deals with a few potential locking problems.
(closes issue ASTERISK-19216)
Reported by: Jacek Konieczny
Review: https://reviewboard.asterisk.org/r/1711/
........
Merged revisions 354263 from http://svn.asterisk.org/svn/asterisk/branches/1.8
Modified:
branches/10/ (props changed)
branches/10/cdr/cdr_pgsql.c
Propchange: branches/10/
------------------------------------------------------------------------------
Binary property 'branch-1.8-merged' - no diff available.
Modified: branches/10/cdr/cdr_pgsql.c
URL: http://svnview.digium.com/svn/asterisk/branches/10/cdr/cdr_pgsql.c?view=diff&rev=354270&r1=354269&r2=354270
==============================================================================
--- branches/10/cdr/cdr_pgsql.c (original)
+++ branches/10/cdr/cdr_pgsql.c Tue Feb 7 09:19:51 2012
@@ -1,7 +1,7 @@
/*
* Asterisk -- An open source telephony toolkit.
*
- * Copyright (C) 2003 - 2006
+ * Copyright (C) 2003 - 2012
*
* Matthew D. Hardeman <mhardemn at papersoft.com>
* Adapted from the MySQL CDR logger originally by James Sharp
@@ -81,6 +81,7 @@
ast_free(sql); \
ast_free(sql2); \
AST_RWLIST_UNLOCK(&psql_columns); \
+ ast_mutex_unlock(&pgsql_lock); \
return -1; \
} \
} \
@@ -94,6 +95,7 @@
ast_free(sql); \
ast_free(sql2); \
AST_RWLIST_UNLOCK(&psql_columns); \
+ ast_mutex_unlock(&pgsql_lock); \
return -1; \
} \
} \
@@ -132,14 +134,10 @@
struct ast_str *sql = ast_str_create(maxsize), *sql2 = ast_str_create(maxsize2);
char buf[257], escapebuf[513], *value;
int first = 1;
-
+
if (!sql || !sql2) {
- if (sql) {
- ast_free(sql);
- }
- if (sql2) {
- ast_free(sql2);
- }
+ ast_free(sql);
+ ast_free(sql2);
return -1;
}
@@ -270,9 +268,10 @@
}
}
first = 0;
- }
+ }
+
+ LENGTHEN_BUF1(ast_str_strlen(sql2) + 2);
AST_RWLIST_UNLOCK(&psql_columns);
- LENGTHEN_BUF1(ast_str_strlen(sql2) + 2);
ast_str_append(&sql, 0, ")%s)", ast_str_buffer(sql2));
ast_verb(11, "[%s]\n", ast_str_buffer(sql));
@@ -334,44 +333,34 @@
return 0;
}
-static int unload_module(void)
+/* This function should be called without holding the pgsql_columns lock */
+static void empty_columns(void)
{
struct columns *current;
-
- ast_cdr_unregister(name);
-
- PQfinish(conn);
-
- if (pghostname) {
- ast_free(pghostname);
- }
- if (pgdbname) {
- ast_free(pgdbname);
- }
- if (pgdbuser) {
- ast_free(pgdbuser);
- }
- if (pgpassword) {
- ast_free(pgpassword);
- }
- if (pgdbport) {
- ast_free(pgdbport);
- }
- if (table) {
- ast_free(table);
- }
- if (encoding) {
- ast_free(encoding);
- }
- if (tz) {
- ast_free(tz);
- }
-
AST_RWLIST_WRLOCK(&psql_columns);
while ((current = AST_RWLIST_REMOVE_HEAD(&psql_columns, list))) {
ast_free(current);
}
AST_RWLIST_UNLOCK(&psql_columns);
+
+}
+
+static int unload_module(void)
+{
+ ast_cdr_unregister(name);
+
+ PQfinish(conn);
+
+ ast_free(pghostname);
+ ast_free(pgdbname);
+ ast_free(pgdbuser);
+ ast_free(pgpassword);
+ ast_free(pgdbport);
+ ast_free(table);
+ ast_free(encoding);
+ ast_free(tz);
+
+ empty_columns();
return 0;
}
@@ -393,9 +382,14 @@
return 0;
}
+ ast_mutex_lock(&pgsql_lock);
+
if (!(var = ast_variable_browse(cfg, "global"))) {
ast_config_destroy(cfg);
- return 0;
+ ast_mutex_unlock(&pgsql_lock);
+ ast_log(LOG_NOTICE, "cdr_pgsql configuration contains no global section, skipping module %s.\n",
+ reload ? "reload" : "load");
+ return -1;
}
if (!(tmp = ast_variable_retrieve(cfg, "global", "hostname"))) {
@@ -403,11 +397,10 @@
tmp = ""; /* connect via UNIX-socket by default */
}
- if (pghostname) {
- ast_free(pghostname);
- }
+ ast_free(pghostname);
if (!(pghostname = ast_strdup(tmp))) {
ast_config_destroy(cfg);
+ ast_mutex_unlock(&pgsql_lock);
return -1;
}
@@ -416,11 +409,10 @@
tmp = "asteriskcdrdb";
}
- if (pgdbname) {
- ast_free(pgdbname);
- }
+ ast_free(pgdbname);
if (!(pgdbname = ast_strdup(tmp))) {
ast_config_destroy(cfg);
+ ast_mutex_unlock(&pgsql_lock);
return -1;
}
@@ -429,11 +421,10 @@
tmp = "asterisk";
}
- if (pgdbuser) {
- ast_free(pgdbuser);
- }
+ ast_free(pgdbuser);
if (!(pgdbuser = ast_strdup(tmp))) {
ast_config_destroy(cfg);
+ ast_mutex_unlock(&pgsql_lock);
return -1;
}
@@ -442,11 +433,10 @@
tmp = "";
}
- if (pgpassword) {
- ast_free(pgpassword);
- }
+ ast_free(pgpassword);
if (!(pgpassword = ast_strdup(tmp))) {
ast_config_destroy(cfg);
+ ast_mutex_unlock(&pgsql_lock);
return -1;
}
@@ -455,11 +445,10 @@
tmp = "5432";
}
- if (pgdbport) {
- ast_free(pgdbport);
- }
+ ast_free(pgdbport);
if (!(pgdbport = ast_strdup(tmp))) {
ast_config_destroy(cfg);
+ ast_mutex_unlock(&pgsql_lock);
return -1;
}
@@ -468,11 +457,10 @@
tmp = "cdr";
}
- if (table) {
- ast_free(table);
- }
+ ast_free(table);
if (!(table = ast_strdup(tmp))) {
ast_config_destroy(cfg);
+ ast_mutex_unlock(&pgsql_lock);
return -1;
}
@@ -481,11 +469,10 @@
tmp = "LATIN9";
}
- if (encoding) {
- ast_free(encoding);
- }
+ ast_free(encoding);
if (!(encoding = ast_strdup(tmp))) {
ast_config_destroy(cfg);
+ ast_mutex_unlock(&pgsql_lock);
return -1;
}
@@ -493,12 +480,12 @@
tmp = "";
}
- if (tz) {
- ast_free(tz);
- tz = NULL;
- }
+ ast_free(tz);
+ tz = NULL;
+
if (!ast_strlen_zero(tmp) && !(tz = ast_strdup(tmp))) {
ast_config_destroy(cfg);
+ ast_mutex_unlock(&pgsql_lock);
return -1;
}
@@ -584,6 +571,7 @@
ast_log(LOG_ERROR, "Failed to query database columns: %s\n", pgerror);
PQclear(result);
unload_module();
+ ast_mutex_unlock(&pgsql_lock);
return AST_MODULE_LOAD_DECLINE;
}
@@ -592,8 +580,12 @@
ast_log(LOG_ERROR, "cdr_pgsql: Failed to query database columns. No columns found, does the table exist?\n");
PQclear(result);
unload_module();
+ ast_mutex_unlock(&pgsql_lock);
return AST_MODULE_LOAD_DECLINE;
}
+
+ /* Clear out the columns list. */
+ empty_columns();
for (i = 0; i < rows; i++) {
fname = PQgetvalue(result, i, 0);
@@ -623,7 +615,9 @@
} else {
cur->hasdefault = 0;
}
+ AST_RWLIST_WRLOCK(&psql_columns);
AST_RWLIST_INSERT_TAIL(&psql_columns, cur, list);
+ AST_RWLIST_UNLOCK(&psql_columns);
}
}
PQclear(result);
@@ -636,12 +630,17 @@
ast_config_destroy(cfg);
- return ast_cdr_register(name, ast_module_info->description, pgsql_log);
+ ast_mutex_unlock(&pgsql_lock);
+ return 0;
}
static int load_module(void)
{
- return config_module(0) ? AST_MODULE_LOAD_DECLINE : 0;
+ if (config_module(0)) {
+ return AST_MODULE_LOAD_DECLINE;
+ }
+ return ast_cdr_register(name, ast_module_info->description, pgsql_log)
+ ? AST_MODULE_LOAD_DECLINE : 0;
}
static int reload(void)
More information about the asterisk-commits
mailing list