[Asterisk-code-review] chan_dahdi: Add DAHDIBusyOut (asterisk[master])

[N A]

Attention is currently required from: Mark Murawski.

N A has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/19264 )

Change subject: chan_dahdi: Add DAHDIBusyOut
......................................................................


Patch Set 8: Code-Review-1

(9 comments)

File channels/chan_dahdi.c:

https://gerrit.asterisk.org/c/asterisk/+/19264/comment/971db33d_11abaf9c 
PS6, Line 20362:                 usleep(1000);
> > I wonder if a thread to do this is the most efficient way. […]
It's chan_dahdi, there is no documentation, lol ;)

See this review for an example that came to mind: https://gerrit.asterisk.org/c/asterisk/+/19714/1/channels/chan_dahdi.c


File channels/chan_dahdi.c:

https://gerrit.asterisk.org/c/asterisk/+/19264/comment/21335495_b4e1d0d9 
PS7, Line 55: #include "asterisk.h"
Empty line after asterisk.h
Should it actually be <regex.h> or not? There isn't a regex.h in the Asterisk include directory, nor in the current directory AFAIK


https://gerrit.asterisk.org/c/asterisk/+/19264/comment/dc41c0bf_8c37ca85 
PS7, Line 16612:        AST_CLI_DEFINE(dahdi_set_busy, "Sets/resets Busyout mode on a channel"),
whitespace


https://gerrit.asterisk.org/c/asterisk/+/19264/comment/3c5f7365_26f7bdd5 
PS7, Line 17934:        ast_unregister_application(dahdi_busyout_app);
whitespace


https://gerrit.asterisk.org/c/asterisk/+/19264/comment/0581012a_910ab933 
PS7, Line 17965:        ast_manager_unregister("DAHDIBusyOut");
whitespace


https://gerrit.asterisk.org/c/asterisk/+/19264/comment/afde4bab_36d30a9a 
PS7, Line 20159:        ast_manager_register_xml("DAHDIBusyOut", 0, action_dahdibusyout);
whitespace


https://gerrit.asterisk.org/c/asterisk/+/19264/comment/d8fd1a73_c3b8b708 
PS7, Line 20304: 
Again, I would ask the same question as before. Say you enable this on 20 channels at once? Is it really efficient to have 20 threads up for channels that don't really exist?

I'm not positive, but I think this could be done a different way. At least, if not, I'd like it explained why not.

Suppose for simplicity we are setting busy out on idle channels. We set a busyout flag to 1.

There is a function in chan_dahdi.c that determines if a channel is available (sig_analog too I think). It returns NO for example if the DND flag is set. I think using DND as a model is useful here. I know that only applies to FXS lines, and this should also apply to FXO lines / trunks, etc.

This covers the subscriber scenario, since requesting a DAHDI channel with busy out will fail.

I'm positive this is not needed for FXS lines (FXO signalled).
For an FXO line that we want to appear busy to somebody trying to call us, the physical loop will need to be closed, so I can understand that much. However, I'm not sure if we really need threads tied up for each busy out. You could manually set the hook state in DAHDI and keep track of that. Since busy outs are all cancelled manually I believe, we don't even need to check for that elsewhere.

I realize this only covers analog, is this supposed to apply to non analog as well?

Overall, I think spawning threads like this should be avoided if possible.


https://gerrit.asterisk.org/c/asterisk/+/19264/comment/7ee431ac_dbbb369c 
PS7, Line 20423: 		return 0;
Should this return -1? Same above?


https://gerrit.asterisk.org/c/asterisk/+/19264/comment/3b2b2932_8fed730c 
PS7, Line 20536: 			result = ast_pthread_create_detached(&busyout_trying_threadid, NULL, dahdi_set_channel_busyout_on_thread, dahdi_chan);
Do not launch a thread to keep retrying. The channel will eventually clear. Set a flag on the DAHDI pvt that there's a queued busy out, check it when the line fully clears, and handle it then.



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Ib75c87c554bb18b8f65be73b4474474e7e1877b7
Gerrit-Change-Number: 19264
Gerrit-PatchSet: 8
Gerrit-Owner: Mark Murawski <markm at intellasoft.net>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: N A <asterisk at phreaknet.org>
Gerrit-Attention: Mark Murawski <markm at intellasoft.net>
Gerrit-Comment-Date: Sat, 14 Jan 2023 20:54:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: N A <asterisk at phreaknet.org>
Comment-In-Reply-To: Mark Murawski <markm at intellasoft.net>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20230114/84466283/attachment-0001.html>