Update and add doc comments to the FreeBSD rc script#49
Update and add doc comments to the FreeBSD rc script#49jevonearth wants to merge 2 commits intosippy:masterfrom
Conversation
| -s ${rtpproxy_ctrl_socket} -d INFO -p /var/run/rtpproxy.pid ${rtpproxy_args}" | ||
|
|
||
| run_rc_command "${1}" | ||
| run_rc_command "$1" |
There was a problem hiding this comment.
This change is superfluous. "${1}" is the same as "$1"
There was a problem hiding this comment.
I made this change to make it consistent with what the FreeBSD project ships in official rc scripts, and what is the apparent convention among most ports. It's not a functional change, but it makes the rc script consistent with the observed FreeBSD convention.
There was a problem hiding this comment.
Well, arguably it's FreeBSD script that is inconsistent, so EWONTFIX. I prefer using ${} for all variables and not make exception for ${N}. I'd suggest to email port maintainer instead once we clean everything else.
There was a problem hiding this comment.
..by which I mean that once we've merged all functional changes I don't think there should be much resistance from maintainer for dropping out rc.d script from ports and replacing it with one supplied with the package regardless ${1} vs. $1. Making the proposed change in our repository would kinda imply that
| command=${prefix}/bin/rtpproxy | ||
| pidfile=/var/run/rtpproxy.pid | ||
|
|
||
|
|
| : ${rtpproxy_args:-""} | ||
|
|
||
| command_args="-l ${rtpproxy_laddr} -p /var/run/rtpproxy.pid" | ||
| command_args="-u ${rtpproxy_usr}:${rtpproxy_grp} -A ${rtpproxy_paddr} -F -l ${rtpproxy_laddr} \ |
There was a problem hiding this comment.
I am not sure if `-A 0.0.0.0' would do any good. Address after -A is taken as a string verbatim and injected into responses without any further checks. Have you tested with all default values? I'd suggest something like the following:
${rtpproxy_paddr:-""}
if [ ! -z "${rtpproxy_paddr}" ]
then
command_args="${command_args} -A ${rtpproxy_paddr}"
fi
I also pretty sure that running rtpproxy with "-l 0.0.0.0" is not the same as running rtpproxy without "-l" altogether, so that the same logic might be needed for that one too.
There was a problem hiding this comment.
I'll have to investigate. I did some basic testing, but not enough. This approach was actually taken from the rc patch file shipped with the public net/rtpproxy port.
Thanks!
|
|
||
| command_args="-l ${rtpproxy_laddr} -p /var/run/rtpproxy.pid" | ||
| command_args="-u ${rtpproxy_usr}:${rtpproxy_grp} -A ${rtpproxy_paddr} -F -l ${rtpproxy_laddr} \ | ||
| -s ${rtpproxy_ctrl_socket} -d INFO -p /var/run/rtpproxy.pid ${rtpproxy_args}" |
There was a problem hiding this comment.
/var/run/rtpproxy.pid -> "${pidfile}"
There was a problem hiding this comment.
Great, will change.
|
P.S. I'd also suggest you to check our startup script in the softswicth to see what other options we are setting up and make provisions for those. So at some point we can possibly drop that and replace it with a stock one. Also good to have those documented and readily available for anyone to consider. Some of those options are: -n set b2bua notification socket for disconnect-on-timeout notifications |
sobomax
force-pushed
the
master
branch
6 times, most recently
from
55700ad to
2131bec
Compare
|
I think this can be closed since the FreeBSD rc.d script has been updated. |
3 participants
These changes should remove the need for the FreeBSD port net/rtpproxy to patch the rc script. Adds comment documentation for each rc parameter. No parameters have been renamed, so existing deployments of the FreeBSD port will not break.