[Asterisk-code-review] pbx: Ensure we don't operate on dialplan while dialplan is reloading, ... (asterisk[18])

[Joshua Colp]

Attention is currently required from: Sean Bright, Kevin Harwell, Mark Murawski.
Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/17798 )

Change subject: pbx: Ensure we don't operate on dialplan while dialplan is reloading, which causes deadlocks
......................................................................


Patch Set 5: Code-Review-1

(5 comments)

File main/pbx.c:

https://gerrit.asterisk.org/c/asterisk/+/17798/comment/16f88cf4_b9ccace8 
PS5, Line 7305:   int ret;
Indentation


https://gerrit.asterisk.org/c/asterisk/+/17798/comment/e49d1305_4bea0ccc 
PS5, Line 7307: 	if (ast_mutex_trylock(&conlock)) {
Have you audited all of the callers of this to see how they react? What user experience/etc occurs?

Have you tested this through all use cases, for example does this get used when pattern matching hints are done? Does that impact/cause locking issues? I ask because instead of locking on a per-context basis, it's now locking the known universe.


https://gerrit.asterisk.org/c/asterisk/+/17798/comment/43a27d9c_c9c4102f 
PS5, Line 7312: 		application, data, datad, registrar, registrar_file, registrar_line, 1);
Conlock is already held, should the last argument be 0 since the locking doesn't need to occur?


https://gerrit.asterisk.org/c/asterisk/+/17798/comment/91ac3a06_b3f40b37 
PS5, Line 7316:   return ret;
Indentation


File pbx/pbx_config.c:

https://gerrit.asterisk.org/c/asterisk/+/17798/comment/f90fbe16_a1b33c95 
PS5, Line 2098: 	/*
This will cause PBX execution to halt during the entire I/O and processing operation for extensions.conf, which is a substantial change to behavior and can have real world implications besides fixing this issue.

Why do you have to lock this here? The ast_merge_contexts_and_delete function locks.



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

Gerrit-Project: asterisk
Gerrit-Branch: 18
Gerrit-Change-Id: I435db97fc56f18b6f5c25a3e400603817b07aa49
Gerrit-Change-Number: 17798
Gerrit-PatchSet: 5
Gerrit-Owner: Mark Murawski <markm at intellasoft.net>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Sean Bright <sean at seanbright.com>
Gerrit-CC: Kevin Harwell <kharwell at digium.com>
Gerrit-Attention: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Kevin Harwell <kharwell at digium.com>
Gerrit-Attention: Mark Murawski <markm at intellasoft.net>
Gerrit-Comment-Date: Tue, 18 Jan 2022 10:37:47 +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/20220118/02e31353/attachment.html>