[Asterisk-code-review] res_config_odbc: Avoid deadlock when max_connections = 1 (...asterisk[master])

[Kevin Harwell]

Kevin Harwell has submitted this change and it was merged. ( https://gerrit.asterisk.org/c/asterisk/+/11076 )

Change subject: res_config_odbc: Avoid deadlock when max_connections = 1
......................................................................

res_config_odbc: Avoid deadlock when max_connections = 1

Rather than calling ast_odbc_find_table() in the prepare callback, call
it beforehand and pass it in to the callback to avoid the need for a
second connection.

ASTERISK-28166 #close

Change-Id: I6f8a0b9990d636fd6bc1a92ed70f7050d2436202
---
M res/res_config_odbc.c
1 file changed, 21 insertions(+), 16 deletions(-)

Approvals:
  Benjamin Keith Ford: Looks good to me, but someone else must approve
  Joshua Colp: Looks good to me, but someone else must approve
  Kevin Harwell: Looks good to me, approved; Approved for Submit



diff --git a/res/res_config_odbc.c b/res/res_config_odbc.c
index 62d8dc3..09eff21 100644
--- a/res/res_config_odbc.c
+++ b/res/res_config_odbc.c
@@ -574,6 +574,7 @@
 	const char *table;
 	const struct ast_variable *lookup_fields;
 	const struct ast_variable *update_fields;
+	struct odbc_cache_tables *tableptr;
 };
 
 static SQLHSTMT update2_prepare(struct odbc_obj *obj, void *data)
@@ -583,29 +584,21 @@
 	const struct ast_variable *field;
 	struct ast_str *sql = ast_str_thread_get(&sql_buf, SQL_BUF_SIZE);
 	SQLHSTMT stmt;
-	struct odbc_cache_tables *tableptr;
 
 	if (!sql) {
 		return NULL;
 	}
 
-	tableptr = ast_odbc_find_table(ups->database, ups->table);
-	if (!tableptr) {
-		ast_log(LOG_ERROR, "Could not retrieve metadata for table '%s@%s'.  Update will fail!\n", ups->table, ups->database);
-		return NULL;
-	}
-
 	res = SQLAllocHandle(SQL_HANDLE_STMT, obj->con, &stmt);
 	if ((res != SQL_SUCCESS) && (res != SQL_SUCCESS_WITH_INFO)) {
 		ast_log(LOG_WARNING, "SQL Alloc Handle failed!\n");
-		ast_odbc_release_table(tableptr);
 		return NULL;
 	}
 
 	ast_str_set(&sql, 0, "UPDATE %s SET ", ups->table);
 
 	for (field = ups->update_fields; field; field = field->next) {
-		if (ast_odbc_find_column(tableptr, field->name)) {
+		if (ast_odbc_find_column(ups->tableptr, field->name)) {
 			ast_str_append(&sql, 0, "%s%s=? ", first ? "" : ", ", field->name);
 			SQLBindParameter(stmt, x++, SQL_PARAM_INPUT, SQL_C_CHAR, SQL_CHAR, strlen(field->name), 0, (void *)field->value, 0, NULL);
 			first = 0;
@@ -618,9 +611,8 @@
 	first = 1;
 
 	for (field = ups->lookup_fields; field; field = field->next) {
-		if (!ast_odbc_find_column(tableptr, field->name)) {
+		if (!ast_odbc_find_column(ups->tableptr, field->name)) {
 			ast_log(LOG_ERROR, "One or more of the criteria columns '%s' on '%s@%s' for this update does not exist!\n", field->name, ups->table, ups->database);
-			ast_odbc_release_table(tableptr);
 			SQLFreeHandle(SQL_HANDLE_STMT, stmt);
 			return NULL;
 		}
@@ -629,9 +621,6 @@
 		first = 0;
 	}
 
-	/* Done with the table metadata */
-	ast_odbc_release_table(tableptr);
-
 	res = ast_odbc_prepare(obj, stmt, ast_str_buffer(sql));
 	if ((res != SQL_SUCCESS) && (res != SQL_SUCCESS_WITH_INFO)) {
 		if (res == SQL_ERROR) {
@@ -663,20 +652,36 @@
 {
 	struct odbc_obj *obj;
 	SQLHSTMT stmt;
-	struct update2_prepare_struct ups = { .database = database, .table = table, .lookup_fields = lookup_fields, .update_fields = update_fields, };
+	struct update2_prepare_struct ups = {
+		.database = database,
+		.table = table,
+		.lookup_fields = lookup_fields,
+		.update_fields = update_fields,
+	};
 	struct ast_str *sql;
 	int res;
 	SQLLEN rowcount = 0;
 
+	ups.tableptr = ast_odbc_find_table(database, table);
+	if (!ups.tableptr) {
+		ast_log(LOG_ERROR, "Could not retrieve metadata for table '%s@%s'. Update will fail!\n", table, database);
+		return -1;
+	}
+
 	if (!(obj = ast_odbc_request_obj(database, 0))) {
+		ast_odbc_release_table(ups.tableptr);
 		return -1;
 	}
 
 	if (!(stmt = ast_odbc_prepare_and_execute(obj, update2_prepare, &ups))) {
 		ast_odbc_release_obj(obj);
+		ast_odbc_release_table(ups.tableptr);
 		return -1;
 	}
 
+	/* We don't need the table anymore */
+	ast_odbc_release_table(ups.tableptr);
+
 	res = SQLRowCount(stmt, &rowcount);
 	SQLFreeHandle(SQL_HANDLE_STMT, stmt);
 	ast_odbc_release_obj(obj);
@@ -690,7 +695,7 @@
 	}
 
 	if (rowcount >= 0) {
-		return (int)rowcount;
+		return (int) rowcount;
 	}
 
 	return -1;

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/11076
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I6f8a0b9990d636fd6bc1a92ed70f7050d2436202
Gerrit-Change-Number: 11076
Gerrit-PatchSet: 1
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190301/322e8f31/attachment.html>