Date: Wed, 1 Jul 2009 00:41:10 +0200 From: Jilles Tjoelker <jilles@stack.nl> To: Hiroki Sato <hrs@FreeBSD.org> Cc: freebsd-rc@FreeBSD.org Subject: Re: RFC: integrate network_ipv6 to netif and tidy up several rc.d scripts Message-ID: <20090630224110.GA33900@stack.nl> In-Reply-To: <20090628.194342.254155418.hrs@allbsd.org> References: <20090628.194342.254155418.hrs@allbsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Jun 28, 2009 at 07:43:42PM +0900, Hiroki Sato wrote:
> I would like your review on the attached patch. Changes are the
> following:
>
> 1. Integrate IPv6 interface configuration to rc.d/netif. Also, IPv6
> routing and options are handled rc.d/routing and rc.d/netoptions
> now. If no INET6, IPv6 configuration is safely ignored.
>
> 2. rc.conf variable change.
>
> ipv6_enable -> (removed)
> ipv6_ifconfig_IF -> ifconfig_ipv6_IF
> ipv6_ifconfig_IF_aliasN -> ifconfig_IF_aliasN (same as IPv4)
>
> The old variables still valid, but display a warning.
>
> 3. rc.d/routed and rc.d/route6d now accept standard rc.d variables
> like $routed_enable. The old $router_enable, $ipv6_router_enable
> and so on are still valid, but display a warning.
>
> 4. Clean up rc.d/netoptions to adjust it to the rc.d framework. No
> functional change but IPv6 specific options are added.
>
> 5. Remove rc.d/auto_linklocal and rc.d/network_ipv6. No longer
> needed.
>
> 6. Fix rc.d/defaultroute to suppress an extra blank line.
>
> 7. rc.conf(5) update. The default value of $ipv6_network_interfaces
> is changed from "auto" to "none".
>
> Basically these changes should be backward compatible except for
> $ipv6_enable and $ipv6_network_interfaces. Note that a part of these
> changes depend on another patch I posted on -net@ recently (ifconfig
> ND6 flags and so on), so simply applying the diff to the current
> system does not work.
> Any comments (or objections) are welcome.
Some comments about the shell scripting, inline.
> Index: etc/network.subr
> ===================================================================
> --- etc/network.subr (revision 195123)
> +++ etc/network.subr (working copy)
> [...]
> + # inet6 specific
> + if afexists ipv6; then
> + if ipv6if $1; then
> + if checkyesno ipv6_gateway_enable ]; then
What's this ']'?
> [...]
> -# _ifconfig_getargs if
> +# _ifconfig_getargs if [af]
> # Echos the arguments for the supplied interface to stdout.
> # returns 1 if empty. In general, ifconfig_getargs should be used
> # outside this file.
> _ifconfig_getargs()
> {
> _ifn=$1
> + case $2 in
> + "") _af= ;;
> + *) _af=_$2 ;;
> + esac
> +
This can be done more simply: _af=${2:+_$2}
> if [ -z "$_ifn" ]; then
> return 1
> fi
>
> - get_if_var $_ifn ifconfig_IF "$ifconfig_DEFAULT"
> + get_if_var $_ifn ifconfig_IF$_af "$ifconfig_DEFAULT"
> }
> [...]
> +# afexists af
> +# Returns 0 if the address family is enabled in the kernel
> +# 1 otherwise.
> +afexists()
> +{
> + _af=$1
> +
> + case ${_af} in
> + inet|ipv4|ip|ip4)
> + if ${SYSCTL_N} net.inet > /dev/null; then
> + return 0
> + else
> + return 1
> + fi
> + ;;
> + inet6|ipv6|ip6)
> + if ${SYSCTL_N} net.inet6 > /dev/null; then
> + return 0
> + else
> + return 1
> + fi
> + ;;
> + esac
> +}
Here and elsewhere, consider using 'local' (even though it's not POSIX,
it is already used and rather useful) or not copying the parameter into
a variable at all. Otherwise strange bugs may occur due to variables
being corrupted by seemingly innocuous function calls.
The redirection should be > /dev/null 2>&1 to avoid an error message if
the address family is not enabled.
There should be a default case which possibly prints an error message
and returns 1.
> # ipv6if if
> # Returns 0 if the interface should be configured for IPv6 and
> # 1 otherwise.
> ipv6if()
> {
> - if ! checkyesno ipv6_enable; then
> + _if=$1
> +
> + if ! afexists ipv6; then
> return 1
> fi
> +
> + # lo0 is always IPv6-enabled
> + case $_if in
> + lo[0-9]*)
> + return 0
> + ;;
> + esac
> +
> case "${ipv6_network_interfaces}" in
> [Aa][Uu][Tt][Oo])
> return 0
> @@ -292,14 +367,61 @@
> return 1
> ;;
> esac
> - for v6if in ${ipv6_network_interfaces}; do
> - if [ "${v6if}" = "${1}" ]; then
> + for i in ${ipv6_network_interfaces}; do
> + if [ "$i" = "$_if" ]; then
Unnecessary change which might cause trouble because i is not local.
> return 0
> fi
> done
> return 1
> }
> [...]
> +
> +# ifalias_ipv4_up if
> +# Helper function for ifalias_up(). Handles IPv4.
> +#
> +ifalias_ipv4_up()
> +{
> + _ret=1
> +
> alias=0
> while : ; do
> ifconfig_args=`get_if_var $1 ifconfig_IF_alias${alias}`
> - if [ -n "${ifconfig_args}" ]; then
> + case "${ifconfig_args}" in
> + inet\ *)
> ifconfig $1 ${ifconfig_args} alias
> alias=$((${alias} + 1))
> _ret=0
> - else
> + ;;
> + *)
> break
> - fi
> + ;;
> + esac
> done
> return $_ret
> }
It looks like this will stop processing the aliases as soon as it finds
an inet6 one. ifalias_ipv6_up, ifalias_ipv4_down and ifalias_ipv6_down
seem similarly affected.
> -#ifalias_down if
> +# ifalias_ipv6_up if
> +# Helper function for ifalias_up(). Handles IPv6.
> +#
> +ifalias_ipv6_up()
> +{
> + _ret=1
> +
> + alias=0
> + while : ; do
> + ifconfig_args=`get_if_var $1 ifconfig_IF_alias${alias}`
> + case "${ifconfig_args}" in
> + inet6\ *)
> + ifconfig $1 ${ifconfig_args} alias
> + alias=$((${alias} + 1))
> + _ret=0
> + ;;
> + *)
> + break
> + ;;
> + esac
> + done
> +
> + # backward compatibility: ipv6_ifconfig_IF_aliasN.
> + alias=0
> + while : ; do
> + ifconfig_args=`get_if_var $1 ipv6_ifconfig_IF_alias${alias}`
> + case "${ifconfig_args}" in
> + "")
> + break
> + ;;
> + *)
> + ifconfig $1 inet6 ${ifconfig_args} alias
> + alias=$((${alias} + 1))
> + warn "\$ipv6_ifconfig_$1_alias${alias} is obsolete."
> + " Use ifconfig_$1_alias${alias} instead."
> + _ret=0
> + ;;
> + esac
> + done
> + return $_ret
> +}
The warning message is wrong in the sense that ifconfig_$1_alias${alias}
will not work if there are also IPv4 aliases. You could count the number
of IPv4 aliases and add that in, but it may be more appropriate to print
a single warning message.
> [...]
> +# ipv6_prefix_hostid_addr_up if
> +# add IPv6 prefix + hostid addr to the interface $if
> +ipv6_prefix_hostid_addr_up()
> +{
> + _if=$1
> + prefix=`get_if_var ${_if} ipv6_prefix_IF`
> +
> + if [ -n "${prefix}" ]; then
> + laddr=`network6_getladdr ${_if}`
> + hostid=`expr "${laddr}" : 'fe80::\(.*\)%\(.*\)'`
Faster:
hostid=${laddr#fe80::}
hostid=${hostid%\%*}
> + for j in ${prefix}; do
> + address=$j\:${hostid}
> + ifconfig ${_if} inet6 ${address} prefixlen 64 alias
> +
> + # if I am a router, add subnet router
> + # anycast address (RFC 2373).
> + if checkyesno ipv6_gateway_enable; then
> + ifconfig ${_if} inet6 $j:: prefixlen 64 \
> + alias anycast
> + fi
> + done
> + fi
> +}
> [...]
> @@ -708,6 +1066,7 @@
>
> # Get a list of ALL the interfaces and make lo0 first if it's there.
> #
> + _tmplist=
> case ${network_interfaces} in
> [Aa][Uu][Tt][Oo])
> _prefix=''
Looks like a possible bugfix. Because _tmplist is overwritten in the *
case, it may be more appropriate to put this assignment under the auto
case.
> @@ -737,26 +1096,49 @@
>
> # Separate out dhcp and non-dhcp interfaces
> #
> - _aprefix=
> - _bprefix=
> - for _if in ${_tmplist} ; do
> - if dhcpif $_if; then
> - _dhcplist="${_dhcplist}${_aprefix}${_if}"
> - [ -z "$_aprefix" ] && _aprefix=' '
> - elif [ -n "`_ifconfig_getargs $_if`" ]; then
> - _nodhcplist="${_nodhcplist}${_bprefix}${_if}"
> - [ -z "$_bprefix" ] && _bprefix=' '
> - fi
> - done
> -
> + _list=
> + _prefix=
> case "$type" in
> nodhcp)
> - echo $_nodhcplist
> + for _if in ${_tmplist} ; do
> + if ! dhcpif $_if && \
> + [ -n "`_ifconfig_getargs $_if`" ]; then
> + _list="${_list}${_prefix}${_if}"
> + [ -z "$_prefix" ] && _prefix=' '
> + fi
> + done
> + echo $_list
The _prefix variable is unnecessary complication. Just
_list="${_list} ${_if}" will do. Word splitting in echo $_list will drop
the initial space. If word splitting weren't acceptable,
echo "${_list# }" would remove it as well; this could simplify the auto
case above.
> [...]
> Index: etc/rc.d/addswap
> ===================================================================
> --- etc/rc.d/addswap (revision 195133)
> +++ etc/rc.d/addswap (working copy)
> @@ -7,7 +7,6 @@
> # PROVIDE: addswap
> # REQUIRE: FILESYSTEMS
> -# BEFORE: sysctl
> # KEYWORD: nojail
> [...]
> Index: etc/rc.d/sysctl
> ===================================================================
> --- etc/rc.d/sysctl (revision 195133)
> +++ etc/rc.d/sysctl (working copy)
> @@ -5,7 +5,7 @@
>
> # PROVIDE: sysctl
> # REQUIRE: root
> -# BEFORE: DAEMON
> +# BEFORE: FILESYSTEMS
> . /etc/rc.subr
I think these two changes need separate consideration.
> [...]
> Index: etc/rc.d/defaultroute
> ===================================================================
> --- etc/rc.d/defaultroute (revision 195133)
> +++ etc/rc.d/defaultroute (working copy)
> [...]
> delay=`expr $delay - 1`
delay=$((delay - 1))
> [...]
> Index: etc/rc.d/rtadvd
> ===================================================================
> --- etc/rc.d/rtadvd (revision 195133)
> +++ etc/rc.d/rtadvd (working copy)
> @@ -40,10 +40,25 @@
> # get a list of interfaces and enable it on them
> #
> case ${rtadvd_interfaces} in
> - '')
> + [Aa][Uu][Tt][Oo]|'')
> for i in `ifconfig -l` ; do
> case $i in
> - lo0|gif[0-9]*|stf[0-9]*|faith[0-9]*|lp[0-9]*|sl[0-9]*|tun[0-9]*)
> + lo0|\
> + stf[0-9]*|\
> + faith[0-9]*|\
> + lp[0-9]*|\
> + sl[0-9]*|\
> + pflog[0-9]*|\
> + pfsync[0-9]*|\
> + an[0-9]*|\
> + ath[0-9]*|\
> + ipw[0-9]*|\
> + iwi[0-9]*|\
> + iwn[0-9]*|\
> + ral[0-9]*|\
> + wi[0-9]*|\
> + wl[0-9]*|\
> + wpi[0-9]*)
> continue
> ;;
> *)
Hmm, any reason you're removing gif[0-9]* here?
> Index: etc/rc.d/routing
> ===================================================================
> --- etc/rc.d/routing (revision 195133)
> +++ etc/rc.d/routing (working copy)
> @@ -21,17 +21,75 @@
>
> routing_start()
> {
> - static_start
> - options_start
> + static_start $*
> + options_start $*
> }
Nitpick: use "$@" to preserve the parameters exactly. $* performs word
splitting and filename generation on each parameter. (This does not
really matter because rc.subr currently breaks it and the called
functions don't care.)
--
Jilles Tjoelker
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090630224110.GA33900>