Fwd: Commit approval requested

[Jilles Tjoelker]

On Fri, Jul 01, 2011 at 02:16:10PM -0500, Josh Paezel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1

> This patch is in production at an organization which uses both single
> and multiple pflog devices on a range of devices.  (eg: it doesn't break
> current configurations)

> It allows multiple pflog devices as well as multiple ftp-proxy instances.

> The patch was submitted as a PR conf/158181

> I've applied the patch to a HEAD svn co and regenerated the patch with
> svn diff from that.

Comments are inline. Note that I have not tested the patch nor any
proposed changes.

> Proposed commit message: (obviously the reviewed by depends :)

> The ftp-proxy and pflogd daemons both allow for multiple instances to be
> run simultaneously. However, the rc scripts that control the startup of
> these daemons have no concept of multiple instances.

> ftp-proxy requires a seperate instance for each "type" of proxying that
> you would like to perform. For example, two instances are required to
> allow operation through NAT for a group of internal servers connecting
> out, and an external server connecting inbound using active-mode FTP.

> pf allows logging to different target pflog interfaces, for more
> granular logging of traffic. This requires multiple pflogd instances,
> one to read each pflog interface.

> Contributed by: Thomas Johnson <tom at claimlynx.com>
> Approved by: kib (mentor)
> Reviewed by: freebsd-rc at freebsd.org
> Sponsored by: Claimlynx, Inc.
> MFC: 1 week
> PR: conf/158181

> Index: share/man/man5/rc.conf.5
> ===================================================================
> --- share/man/man5/rc.conf.5	(revision 223711)
> +++ share/man/man5/rc.conf.5	(working copy)
> @@ -880,6 +880,33 @@
>  This variable contains additional flags passed to the
>  .Xr pflogd 8
>  program.
> +.It Va pflog_instances
> +.Pq Vt str
> +If logging to more than one 
> +.Xr pflog 4
> +interface is desired, 
> +.Va pflog_instances
> +is set to the list of
> +.Xr pflogd 8
> +instances that should be started at system boot time. If 
> +.Va pflog_instances
> +is set, for each whitespace-seperated

Spelling: whitespace-separated

> +.Ar element
> +in the list,
> +.Ao Ar element Ac Ns Va _dev
> +and
> +.Ao Ar element Ac Ns Va _logfile
> +elements are assumed to exist.
> +.Ao Ar element Ac Ns Va _dev
> +must contain the
> +.Xr pflog 4
> +interface to be watched by the named
> +.Xr pflogd 8
> +instance.
> +.Ao Ar element Ac Ns Va _logfile
> +must contain the name of the logfile that will be used by the
> +.Xr pflogd 8
> +instance.
>  .It Va ftpproxy_enable
>  .Pq Vt bool
>  Set to
> @@ -898,6 +925,19 @@
>  This variable contains additional flags passed to the
>  .Xr ftp-proxy 8
>  program.
> +.It Va ftpproxy_instances
> +.Pq Vt str
> +Empty by default. If multiple instances of
> +.Xr ftp-proxy 8
> +are desired at boot time, 
> +.Va ftpproxy_instances
> +should contain a whitespace-seperated list of instance names. For each
> +.Ar element
> +in the list, a variable named
> +.Ao Ar element Ac Ns Va _flags
> +should be defined, containing the command-line flags to be passed to the
> +.Xr ftp-proxy 8
> +instance.
>  .It Va pfsync_enable
>  .Pq Vt bool
>  Set to
> Index: etc/rc.d/pflog
> ===================================================================
> --- etc/rc.d/pflog	(revision 223711)
> +++ etc/rc.d/pflog	(working copy)
> @@ -24,25 +24,41 @@
>  {
>  	load_kld pflog || return 1
>  
> -	# set pflog0 interface to up state
> -	if ! ifconfig pflog0 up; then
> -		warn 'could not bring up pflog0.'
> +	# set pflog_dev interface to up state
> +	if ! ifconfig $pflog_dev up; then
> +		warn "could not bring up $pflog_dev."
>  		return 1
>  	fi
>  
>  	# prepare the command line for pflogd
> -	rc_flags="-f $pflog_logfile $rc_flags"
> +	rc_flags="-f $pflog_logfile -i $pflog_dev $rc_flags"
>  
>  	# report we're ready to run pflogd
>  	return 0
>  }
>  
> +pflog_poststart() {
> +	# Allow child pflogd to settle
> +	sleep 0.10
> +	# More elegant(?) method for getting a unique pid
> +	if [ -f /var/run/pflogd.pid ]; then
> +		mv /var/run/pflogd.pid $pidfile
> +	else
> +		warn "/var/run/pflogd.pid does not exist. Too fast."
> +	fi
> +}

This renaming is rather ugly and should be replaced by a pidfile option
in pflogd(8). However, the hack could be acceptable.

The sleep also points at a sadly common error in daemonizing. IMHO the
parent process should not exit until the pidfile is written, and ideally
not until all initialization that can commonly fail is done. This saves
a lot of grief when debugging problems. Note that this means that
daemon(3) cannot be used.

> +
>  pflog_poststop()
>  {
> -	if ! ifconfig pflog0 down; then
> -		warn 'could not bring down pflog0.'
> +	if ! ifconfig $pflog_dev down; then
> +		warn "could not bring down $pflog_dev."
>  		return 1
>  	fi
> +
> +	if [ "$pflog_instances" ] && [ -n "$pflog_instances" ]; then

These two checks are equivalent. The convention in rc.d scripts is
  [ -n "$pflog_instances" ]

This occurs a few more times.

> +		rm $pidfile
> +	fi
> +
>  	return 0
>  }
>  
> @@ -53,4 +69,33 @@
>  }
>  
>  load_rc_config $name
> -run_rc_command "$1"
> +
> +# Check if spawning multiple pflogd
> +echo "Starting pflogd: $pflog_instances"
> +if [ "$pflog_instances" ] && [ -n "$pflog_instances" ]; then
> +	start_postcmd="pflog_poststart"
> +	# Interate through requested instances.
> +	for i in $pflog_instances; do
> +		# Set required variables
> +		eval pflog_dev=\$pflog_${i}_dev
> +		eval pflog_logfile=\$pflog_${i}_logfile
> +		eval pflog_flags=\$pflog_${i}_flags
> +		# Check that required vars have non-zero length, warn if not.
> +		if [ -z $pflog_dev ]; then

Quotes around $pflog_dev would be safer.

> +			warn "pflog_dev not set"
> +			continue
> +		fi
> +		if [ -z $pflog_logfile ]; then

Quotes.

> +			warn "pflog_logfile not set"
> +			continue
> +		fi
> +		# pflogd sets a pidfile, but the name is hardcoded. Concoct a
> +		# unique pidfile name.
> +		pidfile="/var/run/pflogd.$i.pid"
> +		run_rc_command "$1"
> +	done
> +else
> +	# Typical case, spawn single instance only.
> +	pflog_dev=${pflog_dev:-"pflog0"}
> +	run_rc_command "$1"
> +fi

Using run_rc_command "$1" multiple times also happens in
etc/rc.d/sendmail so I guess it is ok.

> Index: etc/rc.d/ftp-proxy
> ===================================================================
> --- etc/rc.d/ftp-proxy	(revision 223711)
> +++ etc/rc.d/ftp-proxy	(working copy)
> @@ -14,4 +14,62 @@
>  command="/usr/sbin/ftp-proxy"
>  
>  load_rc_config $name
> -run_rc_command "$1"
> +
> +#
> +# manage_pid argument
> +#	Create or remove a pidfile manually, for daemons that can't be bothered
> +#	to do it themselves. Takes one argument, which is the argument provided
> +#	to the rc script. The pidfile will be named /var/run/<$name>.pid,
> +#	unless $pidfile is defined.
> +#
> +#	The method used to determine the pid is rather hacky; grep ps output to
> +#	find '$procname|$command', then grep for ${name}_flags. If at all
> +#	possible, use another method if at all possible, to avoid that dirty-
> +#	code feeling.
> +#
> +manage_pid() {

This function should be broken into a ftp_proxy_poststart and
ftp_proxy_poststop and enabled via $start_postcmd and $stop_postcmd
respectively. This is cleaner and ensures the pidfile is not messed with
if something fails.

> +	local search_string ps_pid
> +	case $1 in
> +		*start)
> +			cmd_string=`basename ${procname:-${command}}`

Consider
  cmd_string=${procname:-${command}}
  cmd_string=${cmd_string##*/}
like rc.subr does, saving a fork.

> +			eval flag_string=\"\$${name}_flags\"
> +			# Determine the pid.
> +			ps_pid=`ps ax -o pid= -o command= | grep $cmd_string | grep -e "$flag_string" | grep -v grep | awk '{ print $1 }'`

Would it be possible to replace this long pipeline with pgrep, like
  ps_pid=$(pgrep -f "$cmd_string.*$flag_string")
to simplify and reduce forks?

If not, move the functionality of the three greps into the awk.

> +			# Write the pidfile depending on $pidfile status.
> +			echo $ps_pid > ${pidfile:-"/var/run/$name.pid"}
> +	       		;;
> +		stop)
> +	       		rm $pidfile
> +	       		;;
> +	esac
> +}
> +
> +# Allow ftp-proxy to start up in two different ways. The typical behavior
> +# is to start up one instance of ftp-proxy by setting ftpproxy_enable and
> +# ftpproxy_flags. The alternate behavior allows multiple instances of ftp-
> +# proxy to be started, allowing different types of proxy behavior. To use the
> +# new behavior, a list of instances must be defined, and a list of flags for
> +# each instance. For example, if we want to start two instances of ftp-proxy,
> +# foo and bar, we would set the following vars.
> +#	ftpproxy_enable="YES"
> +#	ftpproxy_instances="foo bar"
> +#	ftpproxy_foo="<arguments for foo>"
> +#	ftpproxy_bar="<arguments for bar>"
> +#
> +# Starting more than one ftp-proxy?
> +if [ "$ftpproxy_instances" ] && [ -n "${ftpproxy_instances}" ]; then

Redundant test, see above.

> +	# Iterate through instance list.
> +	for i in $ftpproxy_instances; do
> +		#eval ftpproxy_${i}_flags=\$ftpproxy_${i}
> +		#eval name=ftpproxy_${i}

These comments do not belong here, I suppose?

> +		# Set flags for this instance.
> +		eval ftpproxy_flags=\$ftpproxy_${i}
> +		# Define a unique pid file name.
> +		pidfile="/var/run/ftp-proxy.$i.pid"
> +		run_rc_command "$1"
> +		manage_pid $1
> +	done
> +else
> +	# Traditional single-instance behavior
> +	run_rc_command "$1"
> +fi

-- 
Jilles Tjoelker