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

[Mark Michelson]

> On Feb. 4, 2012, 1:58 p.m., Tilghman Lesher wrote:
> > /branches/1.8/res/res_fax.c, lines 2724-2726
> > <https://reviewboard.asterisk.org/r/1713/diff/2/?file=23881#file23881line2724>
> >
> >     Although you're not changing how this works, I think you're actually doing it wrong here.  If there's an issue with an option in the middle of the configuration file, then you set previous-settings + all-options-so-far.  I think the correct behavior should be:  if you encounter a nonrecoverable error in the configuration file, then you should error out and set NO options at all.  This then preserves existing options, instead of implementing a partial configuration.
> >     
> >     In other words, move the end: label down by one line.

I'm glad to hear someone else feels this way. Doing things this way would actually allow for simplifying config setting in other ways as well. I'll put up a new patch.


- Mark


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


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/20120208/2f58fe9d/attachment.htm>