From owner-freebsd-rc@FreeBSD.ORG Wed Jul 1 10:40:30 2009 Return-Path: Delivered-To: freebsd-rc@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 5BAAD1065672; Wed, 1 Jul 2009 10:40:30 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from mx1.stack.nl (relay02.stack.nl [131.155.140.104]) by mx1.freebsd.org (Postfix) with ESMTP id D56158FC13; Wed, 1 Jul 2009 10:40:29 +0000 (UTC) (envelope-from jilles@stack.nl) Received: by mx1.stack.nl (Postfix, from userid 65534) id 5299C358C2E; Wed, 1 Jul 2009 12:17:51 +0200 (CEST) X-Spam-DCC: : X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on hammer.stack.nl X-Spam-Level: * X-Spam-Status: No, score=1.5 required=5.0 tests=BAYES_00,DATE_IN_PAST_06_12, J_CHICKENPOX_12,J_CHICKENPOX_21,J_CHICKENPOX_24,J_CHICKENPOX_32, J_CHICKENPOX_34,NO_RELAYS autolearn=no version=3.2.5 X-Spam-Relay-Country: Received: from toad.stack.nl (toad.stack.nl [IPv6:2001:610:1108:5010::135]) by mx1.stack.nl (Postfix) with ESMTP id F270A35BD9E; Wed, 1 Jul 2009 09:06:37 +0200 (CEST) Received: by toad.stack.nl (Postfix, from userid 1677) id 4E7F574CAD; Wed, 1 Jul 2009 00:41:10 +0200 (CEST) Date: Wed, 1 Jul 2009 00:41:10 +0200 From: Jilles Tjoelker To: Hiroki Sato Message-ID: <20090630224110.GA33900@stack.nl> References: <20090628.194342.254155418.hrs@allbsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20090628.194342.254155418.hrs@allbsd.org> User-Agent: Mutt/1.5.18 (2008-05-17) Cc: freebsd-rc@FreeBSD.org Subject: Re: RFC: integrate network_ipv6 to netif and tidy up several rc.d scripts X-BeenThere: freebsd-rc@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Discussion related to /etc/rc.d design and implementation." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 01 Jul 2009 10:40:30 -0000 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