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

[Tzafrir Cohen]

> On July 18, 2013, 6:43 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, line 11917
> > <https://reviewboard.asterisk.org/r/1598/diff/6/?file=41561#file41561line11917>
> >
> >     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.

This is a loop that is supposed to fail the call in advance if there is any existing channel in the range. Using the range 1-MAX_INT would fail this call if there is any existing channel. The special values 0 and 0 are checked for in build_channels (the values of wanted_channels_start and wanted_channels_end).


> On July 18, 2013, 6:43 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, lines 15759-15767
> > <https://reviewboard.asterisk.org/r/1598/diff/6/?file=41561#file41561line15759>
> >
> >     Use sscanf() instead of atoi().

I prefer atoi() to sscanf in case the input will clearly not be 0. The replacement code with sscanf is less readable and IMHO slightly more error prone:

res = sscanf(a->argv[3], "%30d", &start); // why 30?
if (res != 1 || start < 1) {


> On July 18, 2013, 6:43 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, lines 17723-17727
> > <https://reviewboard.asterisk.org/r/1598/diff/6/?file=41561#file41561line17723>
> >
> >     Is this note still needed?

Not needed.


> On July 18, 2013, 6:43 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, line 15805
> > <https://reviewboard.asterisk.org/r/1598/diff/6/?file=41561#file41561line15805>
> >
> >     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.

The pseudo channel will not be re-created as has_pseudo can not be reset to 0.


> On July 18, 2013, 6:43 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, line 15289
> > <https://reviewboard.asterisk.org/r/1598/diff/6/?file=41561#file41561line15289>
> >
> >     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.

I'm not sure why it should. But if this needs to be done, it should probably be called from pri_destroy_dchan() itself.


> On July 18, 2013, 6:43 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, lines 15782-15783
> > <https://reviewboard.asterisk.org/r/1598/diff/6/?file=41561#file41561line15782>
> >
> >     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?

Right. dahdi_destroy_channel_range() can't fail right now as destroy_channel() pretends not to never fail.


> On July 18, 2013, 6:43 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, line 15796
> > <https://reviewboard.asterisk.org/r/1598/diff/6/?file=41561#file41561line15796>
> >
> >     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.

Added. Note that Users have been able to add partial spans before (using chan_dahdi.conf).


> On July 18, 2013, 6:43 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, lines 15808-15815
> > <https://reviewboard.asterisk.org/r/1598/diff/6/?file=41561#file41561line15808>
> >
> >     Use sscanf() instead of strtol().

What's wrong with strtol? Reading again, I think I prefer here atoi() as above.


> On July 18, 2013, 6:43 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, lines 17719-17722
> > <https://reviewboard.asterisk.org/r/1598/diff/6/?file=41561#file41561line17719>
> >
> >     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.

wanted_channel_start and wanted_channel_stop are defined to be independent. I don't want to assume anything else.


- Tzafrir


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


On July 22, 2013, 5:28 p.m., Tzafrir Cohen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1598/
> -----------------------------------------------------------
> 
> (Updated July 22, 2013, 5:28 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/20130722/5c0b8a5c/attachment-0001.htm>