[Asterisk-code-review] res odbc: Implement a connection pool. (asterisk[13])

[Kevin Harwell]

Kevin Harwell has posted comments on this change.

Change subject: res_odbc: Implement a connection pool.
......................................................................


Patch Set 1: Code-Review-1

(4 comments)

https://gerrit.asterisk.org/#/c/2943/1/res/res_odbc.c
File res/res_odbc.c:

Line 565: 						ast_log(LOG_WARNING, "limit be a positive integer\n");
s/limit be/limit must be/


PS1, Line 818: 				if (odbc_obj_connect(obj) == ODBC_FAIL) {
             : 					break;
             : 				}
I think on connection failure this needs to remove the parent ref and unref the obj structure, which should then be set to NULL. Otherwise you run the risk of a obj->con being NULL and later dereferenced. If it happens to not crash on that then it would be possible for an unconnected connection object to be added to the pool when ast_odbc_release_obj gets called.


PS1, Line 879: res = SQLDisconnect(con);
What happens if it happens to fail to disconnect for some reason? Do we care/would freeing the handle take care of things?


PS1, Line 881: 	if ((res = SQLFreeHandle(SQL_HANDLE_DBC, con)) == SQL_SUCCESS) {
             : 		ast_debug(3, "Database handle %p (connection %p) deallocated\n", obj, con);
             : 	} else {
             : 		SQLGetDiagRec(SQL_HANDLE_DBC, con, 1, state, &err, msg, 100, &mlen);
             : 		ast_log(LOG_WARNING, "Unable to deallocate database handle %p? %d errno=%d %s\n", con, res, (int)err, msg);
             : 	}
Somewhat outside the scope of this change (or if Asterisk would even care), but what happens if freeing the handle fails? Do we leak the handle?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6774bf4bac49a0b30242c76a09c403d2e856ecff
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-HasComments: Yes