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

[Kevin Harwell]

Attention is currently required from: Sean Bright, Dennis.
Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/14698 )

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


Patch Set 3: Code-Review-1

(8 comments)

Commit Message:

https://gerrit.asterisk.org/c/asterisk/+/14698/comment/5b40876e_744c9802 
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.sample file describing how these parameters at least now work (post your patch that is).


File cel/cel_odbc.c:

https://gerrit.asterisk.org/c/asterisk/+/14698/comment/4c8f751f_bfb670f9 
PS3, Line 581: ast_strdupa
Stack allocation like this should really never be done in a loop as it increases the potential of blowing the stack.

I do realize this was part of the pre-patched code. As a check is done prior and if it can be guaranteed to only be done a limited number of times then this _might_ be okay.

However, I think since modifying the code anyways, and given that there could be 'N' aliases I lean toward having this handled in a different way.


https://gerrit.asterisk.org/c/asterisk/+/14698/comment/03089e58_6eac69f0 
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 in a loop also needs removal/changing.


https://gerrit.asterisk.org/c/asterisk/+/14698/comment/d5b1b797_5d1b1339 
PS3, Line 596: 			char *celname = ast_strip(ast_strdupa(var->name + 6));
Another stack allocation case to consider changing.


https://gerrit.asterisk.org/c/asterisk/+/14698/comment/b62e3f69_fde67944 
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. Could you explain more/add a comment about what it does and why it's needed?


https://gerrit.asterisk.org/c/asterisk/+/14698/comment/c05c9db3_9fec4378 
PS3, Line 767: SQLBindParameter
Is the return value worth checking for success/failure?

Same for any other instances of this call below.


https://gerrit.asterisk.org/c/asterisk/+/14698/comment/059e7889_585a9e41 
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 reintroduce something like this bug:

See https://issues.asterisk.org/jira/browse/ASTERISK-30096


https://gerrit.asterisk.org/c/asterisk/+/14698/comment/e8c1ca53_48874af9 
PS3, Line 993: 	destroy_tables_safely();
             : 	load_config();
I think this will require locking on odbc_tables.

While the tables are destroyed safely, it does not appear they are loaded safely. For instance what would happen if during a reload an attempt is made to write to odbc_tables while load_config was only partly done?



-- 
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: 3
Gerrit-Owner: Dennis <dennis.buteyn at xorcom.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Sean Bright <sean at seanbright.com>
Gerrit-CC: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Dennis <dennis.buteyn at xorcom.com>
Gerrit-Comment-Date: Fri, 15 Jul 2022 18:34:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220715/9819ad49/attachment.html>