[Asterisk-code-review] cel_odbc: Rewritten most of cel_odbc (asterisk[16])

[Dennis]

Dennis has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/14698 )

Change subject: cel_odbc: Rewritten most of cel_odbc
......................................................................


Patch Set 4:

(9 comments)

Commit Message:

https://gerrit.asterisk.org/c/asterisk/+/14698/comment/f55adbf9_a93b198b 
PS3, Line 57: Hopefully I was able to re-implement the "static", "filter" and "alias"
            : features properly. Documentation on how they actually function was
            : somewhat light.
> As such I'd recommend adding more documentation to the cel_odbc.conf. […]
Updated cel_odbc.conf.sample documentation.


Patchset:

PS4: 
Made requested changes, also included ast_sem_timedwait fix mentioned previously. Pushed changes for Asterisk 18 (16 is now security fix only).


File cel/cel_odbc.c:

https://gerrit.asterisk.org/c/asterisk/+/14698/comment/c7f24481_1790dbad 
PS3, Line 581: ast_strdupa
> Stack allocation like this should really never be done in a loop as it increases the potential of bl […]
Stack allocation has been moved into a new function, preventing stack overflow.


https://gerrit.asterisk.org/c/asterisk/+/14698/comment/c8e32769_7085fd3b 
PS3, Line 590: 			char *value = strip_quotes(ast_strip(ast_strdupa(var->name + 6)));
> I believe there can be an unknown number of static values as well? If so then this stack allocation  […]
Stack allocation has been moved into a new function, preventing stack overflow.


https://gerrit.asterisk.org/c/asterisk/+/14698/comment/83fd2cdd_e7d1579d 
PS3, Line 596: 			char *celname = ast_strip(ast_strdupa(var->name + 6));
> Another stack allocation case to consider changing.
Stack allocation has been moved into a new function, preventing stack overflow.


https://gerrit.asterisk.org/c/asterisk/+/14698/comment/785dcd3a_89bc51fd 
PS3, Line 722: static int start_table_manager(void)
             : {
             : 	int errcode = 0;
             : 
             : 	ast_sem_init(&manager_sem, 0, 0);
             : 
             : 	if ((errcode = ast_pthread_create(&manager_thread, NULL, mb_manager_thread, NULL))) {
             : 		ast_log(LOG_ERROR, "Could not create thread: %s\n", strerror(errcode));
             : 		return 0;
             : 	} else {
             : 		return 1;
             : 	}
             : }
             : 
             : static void stop_table_manager(void)
             : {
             : 	ast_sem_post(&manager_sem);
             : 	pthread_join(manager_thread, NULL);
             : 	manager_thread = AST_PTHREADT_NULL;
             : 	ast_sem_destroy(&manager_sem);
             : }
> I don't quite understand the need for this table manager thread. […]
In order to construct all the insert queries, table information needs to be queried. This can be a lengthy process, possibly taking forever when one or more databases remain unavailable. Blocking load_module for that long is a bit unreasonable so this manager does all the work instead.


https://gerrit.asterisk.org/c/asterisk/+/14698/comment/cfe7d4ce_3b32c535 
PS3, Line 767: SQLBindParameter
> Is the return value worth checking for success/failure? […]
SQLBindParameter can fail, yes. I've added checks for SQLBindParameter, SQLGetData, SQLGetDiagField, SQLGetDiagRec and SQLRowCount.


https://gerrit.asterisk.org/c/asterisk/+/14698/comment/36da5470_3bbd0c78 
PS3, Line 768: SQL_TYPE_TIMESTAMP
> Are other time types also deduced here? For example, SQL_DATETIME? Just want to make sure we don't r […]
There is no type deduction going on here. The original version of cel_odbc constructed queries with embedded, raw values and required type deduction for figuring out how to format the values as strings. This rewritten version uses parameterized queries which moves the responsibility of type conversion to the database. Aside from eliminating SQL injections, parameterized queries also remove the need to construct a brand new query for every single insert.

For this specific call to SQLBindParameter, SQL_TYPE_TIMESTAMP defines the type (a timestamp) of the value event_time as it is bound as the index'd parameter.


https://gerrit.asterisk.org/c/asterisk/+/14698/comment/235ef7d8_8a02990c 
PS3, Line 993: 	destroy_tables_safely();
             : 	load_config();
> I think this will require locking on odbc_tables. […]
Wrapped a write lock around load_config.



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

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Id85d81add33096f8282d212daf239f2fc845d783
Gerrit-Change-Number: 14698
Gerrit-PatchSet: 4
Gerrit-Owner: Dennis <dennis.buteyn at xorcom.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <default.enum at gmail.com>
Gerrit-Reviewer: Sean Bright <sean at seanbright.com>
Gerrit-CC: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Sun, 06 Nov 2022 12:12:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kevin Harwell <default.enum at gmail.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20221106/1fbfb20f/attachment.html>