[asterisk-dev] [Code Review] dial by name in chan_dahdi

[rmudgett at digium.com]

> On 2010-05-06 12:03:14, rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, lines 12526-12552
> > <https://reviewboard.asterisk.org/r/535/diff/6/?file=9992#file9992line12526>
> >
> >     There is some ambiguity introduced here.  The i<span>- is prefixed to the original dial string by call completion processing for ISDN channels.
> >     
> >     If the original dial string is:
> >     DAHDI/pool!19/5551212
> >     CC processing for an ISDN channel will prefix i<span>- onto the recall dial string:
> >     DAHDI/i1-pool!19/5551212
> >     Now the dial string is invalid!
> >     
> >     I do think it is a bad idea to specify channels on an ISDN span, but it is a possibility and should be taken into account.
> >     
> >     A solution would be to make the i<span>- prefix only valid on the group dialing format since the channel specific and new subdir format specify the DAHDI channel to use.
> >     The my_pri_make_cc_dialstring() function will need to be made smarter about when to prefix i<span>-.
> >     Add this logic to my_pri_make_cc_dialstring():
> >     If '!' is not in the group string and it begins with a digit then the prefix needs to be added.
> >     
> >     I had tried to make my_pri_make_cc_dialstring() as ignorant of the dialing formats as possible.  Unfortunately, that is no longer possible with the addition of this feature.
> 
> rmudgett wrote:
>     That logic should be:
>     If '!' is in the group string and it begins with a digit or 'i' then the prefix should not be added.
>     
>     Other similar logic could be used to detect that the prefix is not present and the dial string is a group dialing format.
> 
> Tzafrir Cohen wrote:
>     I don't see the point in using the combined (i-<subdir>!<channel>) syntax. The whole point of channel pools is not to rely on the arbitrary order of span registration.

The CC processing in my_pri_make_cc_dialstring() prefixes "i<span>-" to the dial string to ensure that the recall attempt goes out the same span.  It currently DOES NOT CARE about the dialing format used.  That is what I am saying needs to be changed.  I do think it is a bad idea to specify channels on an ISDN span, but it is a possibility and should be taken into account.

The acceptable dial string formats need to be changed from:
DAHDI/[i<span>-][<subdir>!]<channel#>[c|r<cadance#>|d][/extension[/options]]
DAHDI/[i<span>-](g|G|r|R)<group#(0-63)>[c|r<cadance#>|d][/extension[/options]]

to become:
DAHDI/[<subdir>!]<channel#>[c|r<cadance#>|d][/extension[/options]]
DAHDI/[i<span>-](g|G|r|R)<group#(0-63)>[c|r<cadance#>|d][/extension[/options]]

Also my original and corrected logic above for my_pri_make_cc_dialstring() was wrong.
Change:
<code>
	if (!args.group) {
		/* Append the ISDN span channel restriction to the dialstring. */
		snprintf(buf, buf_size, "%s/i%d-", args.tech, pvt->pri->span);
		return;
	}
-	if (args.group[0] == 'i') {
-		/* The ISDN span channel restriction is already in the dialstring. */
+	if (strchr(args.group, '!') || isdigit(args.group[0]) || args.group[0] == 'i') {
+		/* The ISDN span channel restriction is not needed or already in the dialstring. */
		ast_copy_string(buf, pvt->dialstring, buf_size);
		return;
	}
</code>


- rmudgett


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/535/#review1976
-----------------------------------------------------------


On 2010-05-06 03:38:59, Tzafrir Cohen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/535/
> -----------------------------------------------------------
> 
> (Updated 2010-05-06 03:38:59)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Currently a name of a channel in chan_dahdi must (with some exception of trunk-groups syntax) rely on DAHDI channel numbers. Sadly channel numbers depend on the specific order of registration. Changing that seems to be quite difficult.
> 
> Thus we try to use the original ope-by-device-file (rather than indert open through /dev/dahdi/channel), and allow files to reside in subdirectories of /dev/dahdi .
> 
> Specifically, the file name must still be a number (to allow using the same ranges syntax). However it should be preceded with the string '<dirname>!' .
> 
> For instance we have: /dev/dahdi/work/1 and /dev/dahdi/work/2 that are symlinks to /dev/dahdi/9 and /dev/dahdi/10 (DAHDI channels 9 and 10), I can refer to them in chan_dahdi.conf as:
> 
>   channel => work!1
>   channel => work!2
> 
> And likewise can dial through them using:
> 
>   Dial(DAHDI/work!1/123456)
> 
> See also the http://svn.asterisk.org/dahdi/linux/team/tzafrir/sysfs .
> 
> 
> Current status: 
> 
> * Tested to basically work. 
> 
> TODO:
> * Adapt better to Asterisk coding style.
> * Use '!' in subdir (in chan_dahdi.conf) as well?
> * Can the original path be exposed to the dialplan?
>   The equivalent of:
>   Set(DAHDI_CHAN=${CUT(-,${CHANNEL}))}
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_dahdi.c 260802 
>   /trunk/configs/chan_dahdi.conf.sample 260802 
> 
> Diff: https://reviewboard.asterisk.org/r/535/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Tzafrir
> 
>