[asterisk-dev] [Code Review]: Add reload support to res_fax

[Kevin Fleming]

> On Feb. 3, 2012, 11:41 a.m., Paul Belanger wrote:
> > should't this be for trunk since this is a new feature?
> 
> Mark Michelson wrote:
>     From the linked issue, Leif left this comment:
>     
>     "It's true that res_fax.so does not have 'reload' support. Per kpfleming and tlesher, if the module has configuration options, it is expected behaviour that we would support reloads, so to close this issue we need to add reload support.
>     
>     For now, the work around is to unload and load the module to cause changes to happen."
>     
>     It sounds as though this was discussed previously and determined to be a bug, rather than a feature request.
> 
> Paul Belanger wrote:
>     Understood, but adding a new CLI command within a release branch seems like a new feature.  Like the original issue says, a workaround is to load / unload the module.
>     
>     *shurgs*
>     
>     It would be nice to define things like this in a wiki page, and consult it.

I don't understand the confusion here; 'unload/load' of a module is a difficult operation to arrange on a busy system where FAX functionality is in use. If 'unload/load' was an acceptable alternative, it would be acceptable across the board, and we wouldn't put 'reload' support into any module. 

As has already been commented here, if a  module has configuration of its own, then it must support 'reload', unless doing so is impractical or problematic. Not doing so is a bug. Also, to be pedantic, this patch does not 'add a new CLI command'. It makes 'module reload' work for res_fax.


- Kevin


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


On Feb. 3, 2012, 11:26 a.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1713/
> -----------------------------------------------------------
> 
> (Updated Feb. 3, 2012, 11:26 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Since res_fax has configurable options, it needs reload support. This patch adds reload support to res_fax.
> 
> 
> This addresses bug ASTERISK-16712.
>     https://issues.asterisk.org/jira/browse/ASTERISK-16712
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/res/res_fax.c 353917 
> 
> Diff: https://reviewboard.asterisk.org/r/1713/diff
> 
> 
> Testing
> -------
> 
> Confirmed that "module reload res_fax.so" is operational and properly loads new configuration options.
> 
> 
> Thanks,
> 
> Mark
> 
>

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