[asterisk-dev] [Code Review] Ensure line terminators for emails are consistent

[Russell Bryant]

> On 2010-03-23 08:51:04, Russell Bryant wrote:
> > /trunk/apps/app_voicemail.c, line 11799
> > <https://reviewboard.asterisk.org/r/544/diff/1/?file=8512#file8512line11799>
> >
> >     Is there a reason not to use ast_safe_system() instead of doing all of this sigchld handling manually?
> 
> Tilghman Lesher wrote:
>     A channel is not otherwise needed in this test function, and the ast_safe_system call requires the use of a channel structure.

Um ... ast_safe_system only takes a single argument, a const char *

Perhaps you're thinking of ast_safe_sleep()?


> On 2010-03-23 08:51:04, Russell Bryant wrote:
> > /trunk/apps/app_voicemail.c, lines 11970-11979
> > <https://reviewboard.asterisk.org/r/544/diff/1/?file=8512#file8512line11970>
> >
> >     Why aren't these cases test failures?
> 
> Tilghman Lesher wrote:
>     The reason they would fail are cases of memory conditions.  Therefore, they would detect a failure in the code when the true failure is in the environment.

It's still a failure, though.  It's certainly not obvious from this test code that the only way that fund_user() and find_or_create() could fail is due to a memory allocation failure.  I would still suggest treating these as failures.


> On 2010-03-23 08:51:04, Russell Bryant wrote:
> > /trunk/apps/app_voicemail.c, lines 11983-11985
> > <https://reviewboard.asterisk.org/r/544/diff/1/?file=8512#file8512line11983>
> >
> >     Should we force the test not to run if IMAP_STORAGE is on, then?
> 
> Tilghman Lesher wrote:
>     No, in the case of IMAP_STORAGE, the line terminators are different, and it is therefore important to ensure that a test runs equally well in that environment.

I agree.  The comment made it sounds like the test was expected not to work if IMAP_STORAGE was on, that's all.


- Russell


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


On 2010-03-08 19:03:35, Tilghman Lesher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/544/
> -----------------------------------------------------------
> 
> (Updated 2010-03-08 19:03:35)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Currently, for every backend other than IMAP, the line terminators for email notifications of voicemail are properly "\n", except within the attachments, which are "\r\n".  This confuses certain mailers.
> 
> 
> This addresses bug 16557.
>     https://issues.asterisk.org/view.php?id=16557
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_voicemail.c 251395 
> 
> Diff: https://reviewboard.asterisk.org/r/544/diff
> 
> 
> Testing
> -------
> 
> Unit test written and run to verify behavior.
> 
> 
> Thanks,
> 
> Tilghman
> 
>