<p> Attention is currently required from: Mark Murawski. </p>
<p>Patch set 8:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/19264">View Change</a></p><p>9 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">File channels/chan_dahdi.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/19264/comment/971db33d_11abaf9c">Patch Set #6, Line 20362:</a> <code style="font-family:monospace,monospace"> usleep(1000);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">> I wonder if a thread to do this is the most efficient way. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">It's chan_dahdi, there is no documentation, lol ;)</p><p style="white-space: pre-wrap; word-wrap: break-word;">See this review for an example that came to mind: https://gerrit.asterisk.org/c/asterisk/+/19714/1/channels/chan_dahdi.c</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File channels/chan_dahdi.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/19264/comment/21335495_b4e1d0d9">Patch Set #7, Line 55:</a> <code style="font-family:monospace,monospace">#include "asterisk.h"</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Empty line after asterisk.h<br>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</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/19264/comment/dc41c0bf_8c37ca85">Patch Set #7, Line 16612:</a> <code style="font-family:monospace,monospace"> AST_CLI_DEFINE(dahdi_set_busy, "Sets/resets Busyout mode on a channel"),</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">whitespace</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/19264/comment/3c5f7365_26f7bdd5">Patch Set #7, Line 17934:</a> <code style="font-family:monospace,monospace"> ast_unregister_application(dahdi_busyout_app);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">whitespace</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/19264/comment/0581012a_910ab933">Patch Set #7, Line 17965:</a> <code style="font-family:monospace,monospace"> ast_manager_unregister("DAHDIBusyOut");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">whitespace</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/19264/comment/afde4bab_36d30a9a">Patch Set #7, Line 20159:</a> <code style="font-family:monospace,monospace"> ast_manager_register_xml("DAHDIBusyOut", 0, action_dahdibusyout);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">whitespace</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/19264/comment/d8fd1a73_c3b8b708">Patch Set #7, Line 20304:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Suppose for simplicity we are setting busy out on idle channels. We set a busyout flag to 1.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">This covers the subscriber scenario, since requesting a DAHDI channel with busy out will fail.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm positive this is not needed for FXS lines (FXO signalled).<br>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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I realize this only covers analog, is this supposed to apply to non analog as well?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Overall, I think spawning threads like this should be avoided if possible.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/19264/comment/7ee431ac_dbbb369c">Patch Set #7, Line 20423:</a> <code style="font-family:monospace,monospace"> return 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should this return -1? Same above?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/19264/comment/3b2b2932_8fed730c">Patch Set #7, Line 20536:</a> <code style="font-family:monospace,monospace"> result = ast_pthread_create_detached(&busyout_trying_threadid, NULL, dahdi_set_channel_busyout_on_thread, dahdi_chan);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/19264">change 19264</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/19264"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Ib75c87c554bb18b8f65be73b4474474e7e1877b7 </div>
<div style="display:none"> Gerrit-Change-Number: 19264 </div>
<div style="display:none"> Gerrit-PatchSet: 8 </div>
<div style="display:none"> Gerrit-Owner: Mark Murawski <markm@intellasoft.net> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: N A <asterisk@phreaknet.org> </div>
<div style="display:none"> Gerrit-Attention: Mark Murawski <markm@intellasoft.net> </div>
<div style="display:none"> Gerrit-Comment-Date: Sat, 14 Jan 2023 20:54:56 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Comment-In-Reply-To: N A <asterisk@phreaknet.org> </div>
<div style="display:none"> Comment-In-Reply-To: Mark Murawski <markm@intellasoft.net> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>