Date: Mon, 1 Jul 2013 00:10:32 +0200 From: Jilles Tjoelker <jilles@stack.nl> To: freebsd-rc@FreeBSD.org Subject: Re: Proposal: multi-instance and self-contained rc.d script Message-ID: <20130630221032.GB43309@stack.nl> In-Reply-To: <20130701.062953.1443190655468739608.hrs@allbsd.org> References: <20130701.062953.1443190655468739608.hrs@allbsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Jul 01, 2013 at 06:29:53AM +0900, Hiroki Sato wrote:
> I am working on rc.d script improvements in terms of the following
> two points. A prototype is attached. This still includes some rough
> edges but should be enough to understand the concepts. I would like
> your comments about them. (This is posted to -current@ and -rc@, but
> please reply to freebsd-rc@ only)
> 1. Multi-instance support
> [snip]
> A new variable "{name}_instances" defines instances. In this
> example, it is named_instances="cache1 cache2". All of the default
> values of $named_{instname}_foo are automatically set to the same as
> $named_foo.
> In the implementation, load_rc_config() reads variables for all
> instances and run_rc_command() runs each instance in order. When
> doing "rc.d/foo stop", run_rc_command() stops the instances in
> reverse order.
This looks OK in concept.
> In the patch, killing the processes without pid file does not work
> well yet. This can be improved.
I don't think it is possible to fix that properly. Killing processes by
name has a high risk of killing unrelated processes in any case.
> 2. Self-contained rc.d script
> rc.d scripts depend on /etc/default/rc.conf for the default
> configuration and rc.conf(5) manual page describes the knobs.
> However, it is difficult to understand which variable is related to
> which script. In addition, /etc/defaults/rc.conf is often out of
> sync with the rc.d scripts. So, my proposal is as follows:
> a) Define rc.conf variables and the default values in the rc.d
> script which uses them. "rc.d/foo rcvar" shows the variables
> and the default values.
This could be done.
> b) Make rc.d/foo always have rc.d/foo(8) manual page.
However, I don't like another set of manual pages.
> The attached patch includes an example of rc.d/routed. The primary
> difference is declaration part of rc.conf variables:
> set_rcvar enable NO
> set_rcvar program /sbin/routed
> set_rcvar flags -q
> These sets the default value of $routed_{enable,program,flags} at
> load_rc_config(). The reason why a simple ": ${routed_enable="NO"}"
> does not work is that it does not work with multi-instance support.
> This is backward-compatible with the current /etc/defaults/rc.conf.
> load_rc_config() sets these values first, and then reads
> /etc/defaults/rc.conf and /etc/rc.conf.d/$name.
> "rc.d/route rcvar" displays the current configuration and available
> variables briefly like the following:
> # routed: network RIP and router discovery routing daemon
> #
> routed_enable="NO" # (default: "NO")
> routed_program="/sbin/routed" # (default: "/sbin/routed")
> routed_flags="-q" # (default: "-q")
> When multi-instance is enabled in rc.conf like this:
> routed_enable="YES"
> routed_instances="hoge fuga"
> routed_hoge_desc="hogehoge"
> routed_fuga_enable="NO"
> routed_fuga_flags=""
> The results of rcvar will be the following:
> # routed: network RIP and router discovery routing daemon
> #
> routed_enable="YES" # (default: "NO")
> routed_program="/sbin/routed" # (default: "/sbin/routed")
> routed_flags="-q" # (default: "-q")
> # routed_hoge: network RIP and router discovery routing daemon: hogehoge
> #
> routed_hoge_enable="YES" # (default: "NO")
> routed_hoge_program="/sbin/routed" # (default: "/sbin/routed")
> routed_hoge_flags="-q" # (default: "-q")
> # routed_fuga: network RIP and router discovery routing daemon
> #
> routed_fuga_enable="NO" # (default: "NO")
> routed_fuga_program="/sbin/routed" # (default: "/sbin/routed")
> routed_fuga_flags="" # (default: "-q")
> We can remove or comment out all of lines in /etc/defaults/rc.conf,
> and mismatch between /etc/defaults/rc.conf and scripts does not
> occur. Running "rc.d/foo rcvar" can be used to generate
> /etc/defaults/rc.conf if needed.
This looks good.
> That's all. Both changes are fully backward compatible and I believe
> they improve flexibility and manageability of rc.d scripts.
> An example of rc.d/routed(8) manual page is also attached. If these
> changes are acceptable, I would like to split the current (lengthy)
> rc.conf(5) manual page into rc.d/foo(8).
> Index: etc/rc.subr
> ===================================================================
> --- etc/rc.subr (revision 252378)
> +++ etc/rc.subr (working copy)
> @@ -54,6 +54,21 @@ JID=`$PS -p $$ -o jid=`
> # functions
> # ---------
>
> +# set_rcvar [var] [defval]
> +# Define rc.conf variable.
> +#
> +set_rcvar()
> +{
> + case $# in
> + 0) echo $name
> + ;;
> + 1) eval rcvars=\"${rcvars# } $1\"
This eval is not needed.
> + ;;
> + 2) eval rcvars=\"${rcvars# } $1\"
This eval is not needed either.
> + eval ${rcvar%_enable}_${1}_defval=\"$2\"
To ensure $2 may contain any and all characters, use instead
eval ${rcvar%_enable}_${1}_defval=\$2
> + ;;
> + esac
> +}
> # set_rcvar_obsolete oldvar [newvar] [msg]
> # Define obsolete variable.
> # Global variable $rcvars_obsolete is used.
> @@ -570,6 +585,39 @@ check_startmsgs()
> #
> run_rc_command()
> {
> + local _act _instances _inst _name _desc _rcvar
> +
> + _act=$1
> + shift
> + eval _instances=\"DEFAULT \$${name}_instances\"
> + _name=$name
> + _desc=$desc
> + _rcvar=$rcvar
> +
> + # Use reverse order for stop.
> + case $_act in
> + *stop) _instances=$(reverse_list $_instances) ;;
This should use a version of reverse_list that writes to a variable
passed as an argument so it does not add a few dozen forks to the
shutdown sequence.
> + esac
> +
> + for _inst in $_instances; do
> + case $_inst in
> + DEFAULT)
> + name=$_name
> + desc=$_desc
> + rcvar=$_rcvar
> + ;;
> + *)
> + name=${_name}_${_inst}
> + eval desc=\"$_desc\${${_name}_${_inst}_desc+:\ }\$${_name}_${_inst}_desc\"
> + rcvar=${_rcvar%_enable}_${_inst}_enable
> + ;;
> + esac
> + _run_rc_command0 $_act "$@"
> + done
> +}
> +
> +_run_rc_command0()
> +{
> _return=0
> rc_arg=$1
> if [ -z "$name" ]; then
> @@ -823,47 +871,25 @@ $command $rc_flags $command_args"
> ;;
>
> rcvar)
> - echo -n "# $name"
> - if [ -n "$desc" ]; then
> - echo " : $desc"
> - else
> - echo ""
> - fi
> + echo "# $name${desc+: }${desc}"
> echo "#"
> # Get unique vars in $rcvar
> - for _v in $rcvar; do
> - case $v in
> - $_v\ *|\ *$_v|*\ $_v\ *) ;;
> - *) v="${v# } $_v" ;;
> - esac
> + v=
> + for _v in $(uniqlist ${rcvar%_enable} $rcvars $_rc_namevarlist); do
> + if [ "$_v" = "instances" ]; then
> + continue
> + fi
> + v="${v# } ${name}_$_v"
> done
>
> # Display variables.
> for _v in $v; do
> - if [ -z "$_v" ]; then
> + eval __v=\$$_v
> + eval _defval=\$${_v}_defval
> + if [ -z "$__v" -a "$__v" = "$_defval" ]; then
> continue
> fi
> -
> - eval _desc=\$${_v}_desc
> - eval _defval=\$${_v}_defval
> - _h="-"
> -
> - eval echo \"$_v=\\\"\$$_v\\\"\"
> - # decode multiple lines of _desc
> - while [ -n "$_desc" ]; do
> - case $_desc in
> - *^^*)
> - echo "# $_h ${_desc%%^^*}"
> - _desc=${_desc#*^^}
> - _h=" "
> - ;;
> - *)
> - echo "# $_h ${_desc}"
> - break
> - ;;
> - esac
> - done
> - echo "# (default: \"$_defval\")"
> + echo ${_v}=\"$__v\"${_defval:+\ # (default: \"$_defval\")}
> done
> echo ""
> ;;
> @@ -1004,11 +1030,55 @@ run_rc_script()
> }
>
> #
> +# uniqlist
> +# Return a list with duplicate words removed.
> +#
> +uniqlist()
> +{
> + local v _v
> +
> + v=
> + for _v in "$@"; do
> + case $v in
> + $_v\ *|\ *$_v|*\ $_v\ *) ;;
> + *) v="${v# } $_v" ;;
> + esac
> + done
> + echo $v
> +}
> +
This function should write its result to a variable whose name is
passed, so that it does not add forks to the boot sequence. Its local
variables should have uniqlist in their name so they do not clash with
the caller's destination variable.
> +#
> # load_rc_config name
> # Source in the configuration file for a given name.
> #
> load_rc_config()
> {
> + local _instances _inst _name _k _v
> +
> + _name=$1
> + _load_rc_config0 $_name
> +
> + eval _instances=\$${_name}_instances
> +
> + for _inst in $_instances; do
> + _load_rc_config0 ${_name}_${_inst}
> +
> + # Set default values by using $name.
> + for _k in $(uniqlist $rcvars $_rc_namevarlist); do
> + if [ "$_k" = "instances" ]; then
> + continue
> + fi
> + eval : \${${_name}_${_inst}_${_k}="\$${_name}_${_k}"}
> + eval : \${${_name}_${_inst}_${_k}_defval="\$${_name}_${_k}_defval"}
> +# eval echo DEBUG ${_name}_${_inst}_${_k}=\$${_name}_${_inst}_${_k}
> + done
> +# eval echo DEBUG ${rcvar%_enable}_${_inst}_enable=\$${rcvar%_enable}_${_inst}_enable
> + eval : \${${rcvar%_enable}_${_inst}_enable="\$${rcvar}"}
> + done
> +}
> +
> +_load_rc_config0()
> +{
> local _name _rcvar_val _var _defval _v _msg _new
> _name=$1
> if [ -z "$_name" ]; then
> @@ -1034,10 +1104,10 @@ load_rc_config()
> fi
>
> # Set defaults if defined.
> - for _var in $rcvar; do
> - eval _defval=\$${_var}_defval
> + for _var in $rcvars; do
> + eval _defval=\$${rcvar%_enable}_${_var}_defval
> if [ -n "$_defval" ]; then
> - eval : \${$_var:=\$${_var}_defval}
> + eval : \${${rcvar%_enable}_${_var}=\$${rcvar%_enable}_${_var}_defval}
> fi
> done
>
> @@ -1051,7 +1121,7 @@ load_rc_config()
> ;;
> *)
> if [ -z "$_new" ]; then
> - _msg="Ignored."
> + : ${_msg="Ignored."}
> else
> eval $_new=\"\$$_var\"
> if [ -z "$_msg" ]; then
> @@ -1736,7 +1806,7 @@ check_kern_features()
> # check_namevarlist var
> # Return "0" if ${name}_var is reserved in rc.subr.
>
> -_rc_namevarlist="program chroot chdir flags fib nice user group groups"
> +_rc_namevarlist="program chroot chdir flags fib nice user group groups instances"
> check_namevarlist()
> {
> local _v
On another note, this function is more complicated than necessary. A
case $1 in
program|chroot|chdir|flags|fib|nice|user|group|groups|instances)
return 0
esac
return 1
will do.
--
Jilles Tjoelker
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130630221032.GB43309>