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

Tue Jun 1 13:19:03 CDT 2010

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


/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/535/#comment4496>

    This sentence should probably end with a '.' instead. :)


/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/535/#comment4506>

    Change:
     * DAHDI/[i<span>-]<channel#>[c|r<cadance#>|d][/extension[/options]]
     * DAHDI/[i<span>-](g|G|r|R)<group#(0-63)>[c|r<cadance#>|d][/extension[/options]]
    To:
     * DAHDI/[i<span>-](g|G|r|R)<group#(0-63)>[c|r<cadance#>|d][/extension[/options]]
    
    Essentially delete the first line.


/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/535/#comment4507>

    Change:
     * DAHDI/i<span>-<channel#>[c|r<cadance#>|d][/extension[/options]]
     * DAHDI/i<span>-(g|G|r|R)<group#(0-63)>[c|r<cadance#>|d][/extension[/options]]
    To:
     * DAHDI/i<span>-(g|G|r|R)<group#(0-63)>[c|r<cadance#>|d][/extension[/options]]
    
    Essentially, delete the first line.


/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/535/#comment4510>

    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/i4-pool!19/5551212
    Now the dial string is invalid!
    
    
    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.
    
    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>
    

/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/535/#comment4502>

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

/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/535/#comment4497>

    Line did not need to be split.


/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/535/#comment4498>

    Line did not need to be split.


/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/535/#comment4499>

    Line did not need to be split.


/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/535/#comment4490>

    Add curly braces and spaces after for and if.


/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/535/#comment4500>

    Use !ast_strlen_zero(subdir) here instead.


/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/535/#comment4491>

    Space after if.


/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/535/#comment4501>

    The spacing change here was unnecessary.


/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/535/#comment4492>

    Since this line is being changed, the red spot also needs to be fixed.


/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/535/#comment4495>

    Add curly braces and a space after the if.


/trunk/configs/chan_dahdi.conf.sample
<https://reviewboard.asterisk.org/r/535/#comment4493>

    Red spot


/trunk/configs/chan_dahdi.conf.sample
<https://reviewboard.asterisk.org/r/535/#comment4494>

    Maybe add a note that this option only makes sense if all of the channels are declared using the relative dahdi path.


- rmudgett


On 2010-05-30 19:41:58, Tzafrir Cohen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/535/
> -----------------------------------------------------------
> 
> (Updated 2010-05-30 19:41:58)
> 
> 
> 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 open-by-device-file (rather than indirect open through /dev/dahdi/channel), and allow files to reside in sub-directories 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/dir/1 and /dev/dahdi/work/dir/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!dir!1
>   channel => work!dir!2
> 
> Or:
>   channel => work!dir!1-2
> 
> And likewise can dial through them using:
> 
>   Dial(DAHDI/work!dir!1/123456)
> 
> Now that we have a naming scheme that does not depend on the specific load order of DAHDI devices, it should be safe to allow a configuration to continue even if some channels have failed. For this we allow setting
> 
>   ignore_failed_channels = true
> 
> in chan_dahdi.conf (it only makes sense to set it in the beginning of the [channels] section). This tells chan_dahdi not to fail even if a channel has failed to configure.
> 
> See also:
> http://svn.asterisk.org/dahdi/linux/team/tzafrir/sysfs
> http://svn.asterisk.org/dahdi/tools/team/tzafrir/sysfs
> https://reviewboard.asterisk.org/r/677
> 
> Current status: 
> 
> * Tested to basically work. 
> 
> TODO:
> * Adapt better to Asterisk coding style.
> * Can the original path be exposed to the dialplan?
>   The equivalent of:
>   Set(DAHDI_CHAN=${CUT(-,${CHANNEL}))}
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_dahdi.c 266565 
>   /trunk/configs/chan_dahdi.conf.sample 266565 
> 
> Diff: https://reviewboard.asterisk.org/r/535/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Tzafrir
> 
>