[Asterisk-code-review] func odbc: single database connection should be optional (asterisk[13])

[Mark Michelson]

Mark Michelson has posted comments on this change.

Change subject: func_odbc: single database connection should be optional
......................................................................


Patch Set 4: Code-Review-1

(2 comments)

https://gerrit.asterisk.org/#/c/2800/4/funcs/func_odbc.c
File funcs/func_odbc.c:

Line 1755: 		ast_log(LOG_NOTICE, "Single database connection per DSN\n");
If you are going to leave this in, please change this to a debug message. This really does not warrant a NOTICE message.


PS4, Line 1833: 	if (cfg && (s = ast_variable_retrieve(cfg, "general", "single_db_connection"))) {
              : 		single_db_connection = ast_true(s);
              : 	} else {
              : 		single_db_connection = DEFAULT_SINGLE_DB_CONNECTION;
              : 	}
              : 
              : 	if (single_db_connection && !dsns) {
              : 		dsns = ao2_container_alloc(DSN_BUCKETS, dsn_hash, dsn_cmp);
              : 		if (!dsns) {
              : 			ast_log(LOG_ERROR, "Could not initialize DSN container\n");
              : 			return 0;
              : 		}
              : 		ast_log(LOG_NOTICE, "Single database connection per DSN\n");
              : 	}
This still has the potential for a race condition. Consider what happens if Asterisk is started with single_db_connection false. Then someone alters the setting so that it is true and reloads Asterisk.

If a query starts between when you have set single_db_connection to true and when you allocate the dsns container, then Asterisk will crash when the query attempts to get a dsn from the container.

The idea I suggested before was to use a read/write lock to protect access of the single_db_connection variable and dsns container.
 * When a query is executed, it read-locks the lock, checks if single_db_connection is true, and acquires its DB handle (either via a dsn or by requesting a connection directly). It then unlocks the read-lock and moves on.
 * When a reload is issued, the reload code write-locks the lock, sets any new values for single_db_connection, and can either allocate or free the dsns container as necessary.

By doing this, queries can execute simultaneously as long as there is no pending reload.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d990616c957dabf7597dea5d5c3148f459dfb6
Gerrit-PatchSet: 4
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Reviewer: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Walter Doekes <walter+asterisk at wjd.nu>
Gerrit-HasComments: Yes