[asterisk-dev] [Code Review] 2757: unbreak safe_asterisk.conf parsing (r394939 / ASTERISK-21965)

[wdoekes]

> On Aug. 13, 2013, 11:13 a.m., Tzafrir Cohen wrote:
> > Why do we reinvent the wheel and not just use /etc/asterisk/startup.d ? If the user wants to override values, the user should drop a file in /etc/asterisk/startup.d .

Yea. I had completely missed that it reads the (hardcoded) /etc/asterisk/startup.d until I was writing this review.

That sounds like the backwards compatible thing to do. Some people apparently already use that.

However, you can't leave it where it is. If tty9 (the default) is unavailable, you won't even get that far.

So, one would have to move it up to just after the default-setting (line 20). That breaks backwards compatibility. But that shouldn't be a disaster for those reading the CHANGES file.


- wdoekes


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


On Aug. 13, 2013, 8:10 a.m., wdoekes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2757/
> -----------------------------------------------------------
> 
> (Updated Aug. 13, 2013, 8:10 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> See patch.
> 
> The patched safe_asterisk is supposed to read values from safe_asterisk.conf, but it never did. It tried to grep a directory (non-recursively).
> 
> This fixes that search (and makes the code less ugly).
> 
> 
> But, there are a few other points that I'd like to address:
> 
> - Why did someone use "#"-comments in the configs/safe_asterisk.conf.sample? We use ";"-commands everywhere else? (And the first line isn't commented out?)
> 
> - I'd rather see a debian style "defaults" file read: if [ -f /etc/defaults/safe_asterisk ]; then source /etc/defaults/safe_asterisk; fi. Then the parsing gets delegated to the shell and we can make things dynamic if we want. But that might be too debian/ubuntu specific. What do other OS-es do?
> 
> - The file has vim:tabstop=4. That's wrong. If you want tabs of 4 spaces, use 4 spaces (sts=4:sw=4:(et)). It also has textwidth=80, but the authors didn't respect it. Let the line go, or use it.
> 
> - I see now that UMASK was checked down the file. Missed that one.
> 
> - NOTIFY default should be root. Not empty. The admin will have root aliased to a real e-mail address.
> 
> - I'd put DUMPDROP/chdir in /var/spool/asterisk (not /tmp). That's supposed to exist. And if you want, you can restrict access to the asterisk user. But that's open for debate. 
> 
> - If set TTY= is set, asterisk output is redirected to ttyN, but safe_asterisk echo's still go to stdout/stderr (but only if the terminal that run_asterisk() was started on is still open and reading from those pipes (the reason why we need to trap PIPE)). All those echo's should go or be redirected to $TTY. And run_asterisk & should get a >/dev/null 2>&1.
> 
> 
> But other than that I'm perfectly happy with it. The automatic restart with mailings about why, have been quite useful.
> 
> 
> Diffs
> -----
> 
>   /trunk/contrib/scripts/safe_asterisk 396558 
> 
> Diff: https://reviewboard.asterisk.org/r/2757/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> wdoekes
> 
>

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