[Asterisk-code-review] cdr mysql: Make sure connection charset is always set (asterisk[13])

[Sean Bright]

Sean Bright has uploaded this change for review. ( https://gerrit.asterisk.org/7756


Change subject: cdr_mysql: Make sure connection charset is always set
......................................................................

cdr_mysql: Make sure connection charset is always set

When the MYSQL_OPT_RECONNECT option is enabled, the MySQL client API
will transparently reconnect when it needs to. Ideally this simplifies
our code, but when this reconnection occurs all connection state is
lost. Because we are not notified that this has happened, we don't know
to set our character set again (with "SET NAMES 'xyz'").

Rather than calling SET NAMES, we instead set the MYSQL_SET_CHARSET_NAME
option which will do it for us under the hood on each connect. This
option has been present in the MySQL C API for at least 15 years, so it
should be safe for most installations.

I also snuck a few other changes into this patch:

* Default the MySQL port to MYSQL_PORT (3306) instead of 0 if it's not
  defined.

* Fix some erroneous and/or silly checks on the contents of the
  configuration ast_str values.

ASTERISK-27366 #close
Reported by: Halil İbrahim YILDIZ

Change-Id: I36bf8dc5d5f83584e803b3b1a151dea9396ab8f5
---
M addons/cdr_mysql.c
1 file changed, 21 insertions(+), 14 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/56/7756/1

diff --git a/addons/cdr_mysql.c b/addons/cdr_mysql.c
index c5cf108..53ea923 100644
--- a/addons/cdr_mysql.c
+++ b/addons/cdr_mysql.c
@@ -128,7 +128,7 @@
 		else
 			snprintf(status, 255, "Connected to %s@%s", ast_str_buffer(dbname), ast_str_buffer(hostname));
 
-		if (!ast_strlen_zero(ast_str_buffer(dbuser)))
+		if (ast_str_strlen(dbuser))
 			snprintf(status2, 99, " with username %s", ast_str_buffer(dbuser));
 		if (ast_str_strlen(dbtable))
 			snprintf(status2, 99, " using table %s", ast_str_buffer(dbtable));
@@ -157,6 +157,16 @@
 static struct ast_cli_entry cdr_mysql_status_cli[] = {
 	AST_CLI_DEFINE(handle_cli_cdr_mysql_status, "Show connection status of cdr_mysql"),
 };
+
+static void configure_connection_charset(void)
+{
+	if (ast_str_strlen(dbcharset)) {
+		const char *charset = ast_str_buffer(dbcharset);
+		if (mysql_options(&mysql, MYSQL_SET_CHARSET_NAME, charset)) {
+			ast_log(LOG_WARNING, "Failed to set connection charset. Data inserted might be invalid.\n");
+		}
+	}
+}
 
 static int mysql_log(struct ast_cdr *cdr)
 {
@@ -190,15 +200,13 @@
 		if (ssl_ca || ssl_cert || ssl_key) {
 			mysql_ssl_set(&mysql, ssl_key ? ast_str_buffer(ssl_key) : NULL, ssl_cert ? ast_str_buffer(ssl_cert) : NULL, ssl_ca ? ast_str_buffer(ssl_ca) : NULL, NULL, NULL);
 		}
+
+		configure_connection_charset();
+
 		if (mysql_real_connect(&mysql, ast_str_buffer(hostname), ast_str_buffer(dbuser), ast_str_buffer(password), ast_str_buffer(dbname), dbport, dbsock && ast_str_strlen(dbsock) ? ast_str_buffer(dbsock) : NULL, ssl_ca ? CLIENT_SSL : 0)) {
 			connected = 1;
 			connect_time = time(NULL);
 			records = 0;
-			if (dbcharset) {
-				ast_str_set(&sql1, 0, "SET NAMES '%s'", ast_str_buffer(dbcharset));
-				mysql_real_query(&mysql, ast_str_buffer(sql1), ast_str_strlen(sql1));
-				ast_debug(1, "SQL command as follows: %s\n", ast_str_buffer(sql1));
-			}
 		} else {
 			ast_log(LOG_ERROR, "Cannot connect to database server %s: (%d) %s\n", ast_str_buffer(hostname), mysql_errno(&mysql), mysql_error(&mysql));
 			connected = 0;
@@ -500,7 +508,7 @@
 	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, 0);
+	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, "");
@@ -540,15 +548,16 @@
 	ast_debug(1, "Got hostname of %s\n", ast_str_buffer(hostname));
 	ast_debug(1, "Got port of %d\n", dbport);
 	ast_debug(1, "Got a timeout of %d\n", timeout);
-	if (dbsock)
+	if (ast_str_strlen(dbsock)) {
 		ast_debug(1, "Got sock file of %s\n", ast_str_buffer(dbsock));
+	}
 	ast_debug(1, "Got user of %s\n", ast_str_buffer(dbuser));
 	ast_debug(1, "Got dbname of %s\n", ast_str_buffer(dbname));
 	ast_debug(1, "Got password of %s\n", ast_str_buffer(password));
 	ast_debug(1, "%sunning in calldate compatibility mode\n", calldate_compat ? "R" : "Not r");
 	ast_debug(1, "Dates and times are localized to %s\n", S_OR(ast_str_buffer(cdrzone), "local timezone"));
 
-	if (dbcharset) {
+	if (ast_str_strlen(dbcharset)) {
 		ast_debug(1, "Got DB charset of %s\n", ast_str_buffer(dbcharset));
 	}
 
@@ -573,6 +582,9 @@
 			NULL, NULL);
 	}
 	temp = dbsock && ast_str_strlen(dbsock) ? ast_str_buffer(dbsock) : NULL;
+
+	configure_connection_charset();
+
 	if (!mysql_real_connect(&mysql, ast_str_buffer(hostname), ast_str_buffer(dbuser), ast_str_buffer(password), ast_str_buffer(dbname), dbport, temp, ssl_ca && ast_str_strlen(ssl_ca) ? CLIENT_SSL : 0)) {
 		ast_log(LOG_ERROR, "Failed to connect to mysql database %s on %s.\n", ast_str_buffer(dbname), ast_str_buffer(hostname));
 		connected = 0;
@@ -582,11 +594,6 @@
 		connected = 1;
 		records = 0;
 		connect_time = time(NULL);
-		if (dbcharset) {
-			snprintf(sqldesc, sizeof(sqldesc), "SET NAMES '%s'", ast_str_buffer(dbcharset));
-			mysql_real_query(&mysql, sqldesc, strlen(sqldesc));
-			ast_debug(1, "SQL command as follows: %s\n", sqldesc);
-		}
 
 		/* Get table description */
 		snprintf(sqldesc, sizeof(sqldesc), "DESC %s", dbtable ? ast_str_buffer(dbtable) : "cdr");

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

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: newchange
Gerrit-Change-Id: I36bf8dc5d5f83584e803b3b1a151dea9396ab8f5
Gerrit-Change-Number: 7756
Gerrit-PatchSet: 1
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171228/288d6ee9/attachment.html>