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

Tue Aug 6 15:03:27 CDT 2013

> 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.
> 
> Tzafrir Cohen wrote:
>     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).
> 
> rmudgett wrote:
>     Isn't the point of the "all" in dahdi_create_channels() to fail if there are any existing channels?  This routine will not fail if the start,end is 0,0.  It will ignore all channels because: if (cur->channel >= 0 && cur->channel <= 0)

1,MAX_INT will fail if there is any existing channel. 0,0 will not fail and thus create any non-existing channel. This was useful for me while testing and I believe may be useful elsewhere. I renamed "all" to "new". Is that clear enough?

> 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().
> 
> Tzafrir Cohen wrote:
>     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) {
> 
> rmudgett wrote:
>     The %30d is because of a bug in libc that caused a security vulnerability.  Asterisk uses sscanf() throughout the code because bogus input is detectable where atoi() or strtol() would return 0.

See below regarding strtol(). Short version: Easy to assume here input is only valid if it is > 0.

> 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().
> 
> Tzafrir Cohen wrote:
>     What's wrong with strtol? Reading again, I think I prefer here atoi() as above.
> 
> rmudgett wrote:
>     atoi and strtol are being replaced by sscanf() in the codebase because of better error detection.

strtol allows check for non-number input just as sscanf and the code here uses that check. What the code here misses, though, is a test for the number being > 0. atoi() can't tell the difference between no/invalid number and 0, but we don't really care here. And it is simpler (and thus: more readable) than the two alternatives.

> 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.
> 
> Tzafrir Cohen wrote:
>     I'm not sure why it should. But if this needs to be done, it should probably be called from pri_destroy_dchan() itself.
> 
> rmudgett wrote:
>     This could be dropped.  We'll still be leaking memory anyway.  I was concerned about settings from the previous setup bleeding through to the new setup.  The setup_dahdi() code is likely assuming that is how the struct is setup when called.

Right. It is needed. Without it, prepare_pri won't reopen the D channel's DAHDI channel.

> 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.
> 
> Tzafrir Cohen wrote:
>     wanted_channel_start and wanted_channel_stop are defined to be independent. I don't want to assume anything else.
> 
> rmudgett wrote:
>     They are not used independently.

I don't really like it, but: OK.

- 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/20130806/4af26a13/attachment-0001.htm>