Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update and add doc comments to the FreeBSD rc script #49

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Update and add doc comments to the FreeBSD rc script #49

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb11c8a
Update and add doc comments to the FreeBSD rc script
Oct 19, 2016
c3cdf43
Add quotes to $1 on run_rc_command
Oct 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 37 additions & 17 deletions scripts/rtpproxy.in.freebsd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,48 @@
#!/bin/sh

# Add the following lines to /etc/rc.conf to enable RTPproxy:
#
# rtpproxy_enable="YES"

# PROVIDE: rtpproxy
# REQUIRE: DAEMON
# BEFORE: ser openser

prefix=%%PREFIX%%

. %%RC_SUBR%%
# BEFORE: kamailio opensips
#
# Add the following lines to /etc/rc.conf to enable RTPproxy:
#
# rtpproxy_enable (bool): Set to NO by default
# Set it to YES to enable rtpproxy
# rtpproxy_laddr (string): Set listen address
# Default is "0.0.0.0"
# rtpproxy_ctrl_socket (string): Set control socket location
# Default is "/var/run/rtpproxy.sock"
# rtpproxy_paddr (string): Set advertised address
# Default is "0.0.0.0"
# rtpproxy_usr (string): Set user to run rtpproxy
# Default is "rtpproxy"
# rtpproxy_grp (string): Set group to run rtpproxy
# Default is "rtpproxy"
# rtpproxy_args (string): Set additional command line arguments
# Default is ""

. /etc/rc.subr

name=rtpproxy
rcvar=`set_rcvar`

command="${prefix}/bin/rtpproxy"
pidfile="/var/run/rtpproxy.pid"
desc="rtpproxy daemon startup script"
rcvar=rtpproxy_enable

load_rc_config ${name}

rtpproxy_enable=${rtpproxy_enable:-"NO"}
rtpproxy_laddr=${rtpproxy_laddr:-"0.0.0.0"}
prefix=/usr/local
command=${prefix}/bin/rtpproxy
pidfile=/var/run/rtpproxy.pid


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra blank line.

: ${rtpproxy_enable:-"NO"}
: ${rtpproxy_laddr:-"0.0.0.0"}
: ${rtpproxy_ctrl_socket:-"unix:/var/run/rtpproxy.sock"}
: ${rtpproxy_paddr:-"0.0.0.0"}
: ${rtpproxy_usr:-"rtpproxy"}
: ${rtpproxy_grp:-"rtpproxy"}
: ${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} \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

-s ${rtpproxy_ctrl_socket} -d INFO -p /var/run/rtpproxy.pid ${rtpproxy_args}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/var/run/rtpproxy.pid -> "${pidfile}"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, will change.


run_rc_command "${1}"
run_rc_command "$1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is superfluous. "${1}" is the same as "$1"

Copy link
Contributor Author

@jevonearth jevonearth Oct 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

..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 $1 is somehow better than ${1}, which it is not. Most people just use the former because it's shorter, I don't think there is any official style guide that prefers one over another esp in vendor-supplied scripts.