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

Wed Nov 30 13:03:20 CST 2011

> On Nov. 28, 2011, 8:09 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, lines 1371-1372
> > <https://reviewboard.asterisk.org/r/1598/diff/1/?file=21849#file21849line1371>
> >
> >     Doxygen description for new members.

Done

> On Nov. 28, 2011, 8:09 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, lines 11329-11330
> > <https://reviewboard.asterisk.org/r/1598/diff/1/?file=21849#file21849line11329>
> >
> >     Update comment and doxegnify.

Done

> On Nov. 28, 2011, 8:09 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, lines 11345-11348
> > <https://reviewboard.asterisk.org/r/1598/diff/1/?file=21849#file21849line11345>
> >
> >     Curly braces.

Done

> On Nov. 28, 2011, 8:09 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, line 11368
> > <https://reviewboard.asterisk.org/r/1598/diff/1/?file=21849#file21849line11368>
> >
> >     You did not need to move this prototype.

Fails build otherwise.

> On Nov. 28, 2011, 8:09 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, lines 11371-11372
> > <https://reviewboard.asterisk.org/r/1598/diff/1/?file=21849#file21849line11371>
> >
> >     Doxygen function description.

Done.

> On Nov. 28, 2011, 8:09 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, line 11389
> > <https://reviewboard.asterisk.org/r/1598/diff/1/?file=21849#file21849line11389>
> >
> >     Adding channels to an existing ISDN span is going to have subtle problems.  This check is not adequate to prevent it.
> >     
> >     Also adding SS7 channels to existing linksets is likely to have other problems due to the weird way SS7 configures channels.

The typical use case I have in mind is to add complete (DAHDI) spans at a time (once they are assigned). However Asterisk has no generic notion of DAHDI spans. I wonder if there would be a better interface.

> On Nov. 28, 2011, 8:09 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, line 15090
> > <https://reviewboard.asterisk.org/r/1598/diff/1/?file=21849#file21849line15090>
> >
> >     Might as well fix the spacing with the parentheses.

Done

> On Nov. 28, 2011, 8:09 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, line 15072
> > <https://reviewboard.asterisk.org/r/1598/diff/1/?file=21849#file21849line15072>
> >
> >     if (sscanf() == 1)

Replaced it there. Now using two separate arguments.

> On Nov. 28, 2011, 8:09 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, line 15111
> > <https://reviewboard.asterisk.org/r/1598/diff/1/?file=21849#file21849line15111>
> >
> >     if (sscanf() == 1)

Ditto.

> On Nov. 28, 2011, 8:09 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, line 15129
> > <https://reviewboard.asterisk.org/r/1598/diff/1/?file=21849#file21849line15129>
> >
> >     Spacing with parentheses.

Ditto.

> On Nov. 28, 2011, 8:09 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, lines 17013-17016
> > <https://reviewboard.asterisk.org/r/1598/diff/1/?file=21849#file21849line17013>
> >
> >     Odd line breaks.
> >     Curly braces needed.

Hopefully better now.

> On Nov. 28, 2011, 8:09 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, lines 18354-18360
> > <https://reviewboard.asterisk.org/r/1598/diff/1/?file=21849#file21849line18354>
> >
> >     Memory leak in conf when goto path taken.
> >     Need to call ast_cc_config_params_destroy().
> >     
> >     Why not just check for the pseudo channel before entering this if block?

Right. No need to create it more than once. Might as well save this information in a global (static) variable.

> On Nov. 28, 2011, 8:09 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, line 14220
> > <https://reviewboard.asterisk.org/r/1598/diff/1/?file=21849#file21849line14220>
> >
> >     Comma spacing.

Done

> On Nov. 28, 2011, 8:09 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, lines 11428-11430
> > <https://reviewboard.asterisk.org/r/1598/diff/1/?file=21849#file21849line11428>
> >
> >     This must be done for all exit points in the function to not have a memory leak.  This is why setup_dahdi_int() was extracted from setup_dahdi().

Kernel-style goto?

> On Nov. 28, 2011, 8:09 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, lines 11395-11398
> > <https://reviewboard.asterisk.org/r/1598/diff/1/?file=21849#file21849line11395>
> >
> >     Curly braces.

Done

- Tzafrir

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

On Nov. 24, 2011, 12:19 p.m., Tzafrir Cohen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1598/
> -----------------------------------------------------------
> 
> (Updated Nov. 24, 2011, 12:19 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This code adds chan_dahdi the command 'dahdi create channels <range>' (where <range> is a single <n>-<m>) and updates 'dahdi destroy channel' with a similar 'dahdi destroy channels'.
> 
> 'dahdi create channels' acts as a filter for process_dahdi() and such in DAHDI: it re-parses the whole configuration, but only attempts to create channels in the range.
> 
> Right not this patch disables the initial probing at module load time (workaround: run 'dahdi create channels 1-10000' at startup through cli.conf). This is for debugging, mainly and should be removed later on.
> 
> This changeg 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: https://gitorious.org/~tzafrir/asterisk-tools/tzafrirs-dahdi-tools/commits/pinned-spans
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_dahdi.c 346290 
> 
> Diff: https://reviewboard.asterisk.org/r/1598/diff
> 
> 
> Testing
> -------
> 
> Works well with analog devices. Seems to work with ISDN ones as well. Not fully complete, though.
> 
> 
> Thanks,
> 
> Tzafrir
> 
>

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