[asterisk-dev] [Code Review] 1598: chan_dahdi: create and destroy channels at run-time

[rmudgett]

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



/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/1598/#comment18068>

    All of the ast_verbose() calls in this function must be removed.  They are debug messages at best.  At worst they go to the wrong place and are unconditional.



/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/1598/#comment18077>

    This does not handle the "all" case where start=0, end=0.
    
    If the "all" case is changed to 1 and MAX_INT then you don't have to also worry about the existing pseudo channel.



/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/1598/#comment18084>

    Coding guidelines.  Declarations not allowed after the beginning of a block.



/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/1598/#comment18078>

    The "all" case is not handled here as well.
    
    Nothing needs to be done if the all case is 1-MAX_INT.



/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/1598/#comment18085>

    Remove this.



/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/1598/#comment18074>

    Remove tab after (.



/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/1598/#comment18083>

    This still needs to be done.  Otherwise already existing PRI spans will fail to get setup again.  Or we could have a file descriptor leak when the D channel is opened again.
    
    Solution: The current value of pri->pri.fds[i] could be used [if (-1)] to indicate that the D channel has not been setup.



/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/1598/#comment18070>

    Add curly braces.  (again)



/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/1598/#comment18086>

    Call sig_pri_init_pri() after destroying the D channel here.  At this point we know that the whole span is destroyed and we must setup to be able to recreate the span.



/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/1598/#comment18071>

    Use sscanf() instead of atoi().



/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/1598/#comment18069>

    Just return success here.
    
    When the destroy channel call returns, there are no channels in the range.  Whether there were any channels in the range to begin with doesn't really matter does it?



/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/1598/#comment18079>

    I think a note of caution is need here.
    
    You cannot add arbitrary channels in any order.  ISDN and SS7 require whole spans to be added at once.



/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/1598/#comment18082>

    It would probably be best if "all" passed in 1 and MAX_INT.  Then you don't have to worry about the already created pseudo channel mucking up the create channels request.



/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/1598/#comment18072>

    Use sscanf() instead of strtol().



/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/1598/#comment18081>

    This test could be:
    if (conf->wanted_channels_start && (real_channel < conf->wanted_channels_start || conf->wanted_channels_end < real_channel)) {
       continue;
    }
    
    If you have a start channel you will have an end channel.  Otherwise, you have no range and all channels need to be created.



/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/1598/#comment18080>

    Is this note still needed?



/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/1598/#comment18073>

    Curly braces.


- rmudgett


On July 17, 2013, 6:29 p.m., Tzafrir Cohen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1598/
> -----------------------------------------------------------
> 
> (Updated July 17, 2013, 6:29 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This code adds chan_dahdi the command 'dahdi create channels <range>' (where <range> is a single <n>-<m> or 'all') and updates 'dahdi destroy channel' with a similar 'dahdi destroy channels'.
> 
> This change is intended to provide a hook for a script running from udev once a span has been assigned ("registered") / unassigned ("unregistered") for its channels. The udev hook configures the span's channels with dahdi_cfg -S, and can then ask Asterisk to create ethe channels. See the scripts added to DAHDI-tools in 2.7.0.
> 
> This means that a separate DAHDI init script (running before the Asterisk one) would no longer be required. There is no longer a need to run a single command after all DAHDI devices are up.
> 
> If you use those new DAHDI settings you should set ignore_failed_channels in chan_dahdi.conf to avoid missing a channel failing the startup of chan_dahdi (with potentially other channels already available).
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_dahdi.c 394567 
> 
> Diff: https://reviewboard.asterisk.org/r/1598/diff/
> 
> 
> Testing
> -------
> 
> Works well with analog devices and mostly tested with ISDN devices.
> 
> 
> Thanks,
> 
> Tzafrir Cohen
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130718/20af4570/attachment-0001.htm>