conf/104884: Add support EtherChannel configuration to rc.conf

[Norikatsu Shigemura]

The following reply was made to PR conf/104884; it has been noted by GNATS.

From: Norikatsu Shigemura <nork at FreeBSD.org>
To: Florent Thoumie <flz at FreeBSD.org>
Cc: Norikatsu Shigemura <nork at FreeBSD.org>, freebsd-bugs at FreeBSD.org,
        FreeBSD-gnats-submit at FreeBSD.org, freebsd-rc at FreeBSD.org
Subject: Re: conf/104884: Add support EtherChannel configuration to rc.conf
Date: Tue, 30 Jan 2007 01:53:17 +0900

 On Sun, 28 Jan 2007 19:37:03 +0000
 Florent Thoumie <flz at freebsd.org> wrote:
 > >> http://www.freebsd.org/cgi/query-pr.cgi?pr=104884
 > >>> Category:       conf
 > >>> Responsible:    freebsd-bugs
 > >>> Synopsis:       Add support EtherChannel configuration to rc.conf
 > >>> Arrival-Date:   Sat Oct 28 16:10:18 GMT 2006
 > > 	I chased HEAD.  Please see following patch.
 > > 	Anyone, please handle this PR?
 > > 	And I'll make a patch for 6-stable.
 > I'm sorry, I meant to answer but forgot about it. I don't know much
 
 	Thanks for your handling.
 
 > about technical details, so I'll only focus on style. Here are my
 > comments on the patch:
 
 	I think that the answer of all questions is 'BECAUSE netgraph's
 	specification.'. :-)
 
 > > Index: network.subr
 > > ===================================================================
 > > RCS file: /home/ncvs/src/etc/network.subr,v
 > > retrieving revision 1.176
 > > diff -u -r1.176 network.subr
 > > --- network.subr	29 Oct 2006 13:29:49 -0000	1.176
 > > +++ network.subr	28 Jan 2007 14:52:36 -0000
 > > @@ -907,3 +907,78 @@
 > >  		esac
 > >  	done
 > >  }
 > > +
 > > +ng_mkpeer() {
 > > +	ngctl -f - 2> /dev/null <<EOF
 > > +mkpeer $*
 > > +msg dummy nodeinfo
 > > +EOF
 > > +}
 > > +
 > > +ng_create_one() {
 > > +	ng_mkpeer $* | while read line; do
 > > +		t=`expr "${line}" : '.* name="\([a-z]*[0-9]*\)" .*'`
 > > +		if [ -n "${t}" ]; then
 > > +			echo ${t}
 > > +			return
 > > +		fi
 > > +	done
 > > +}
 
 	I implemented ng_mkpeer and ng_create_one as generic functions.
 	If anyone want to add other netgraph function, they can use
 	these functions.
 
 > > +ng_fec_create() {
 > > +	local req_iface iface bogus
 > > +	req_iface="$1"
 > > +	if [ -z "${req_iface}" ]; then
 > Why are you testing this? It's only called in fec_up() and can't be
 > called with a empty argument. Or do you want to "export" the function to
 > other scripts?
 
 	Ah! This code's meaning was changed from original code.  Yes,
 	I think that this code should be removed.
 
 > > +	ngctl shutdown ${req_iface}: > /dev/null 2>&1
 > > +	bogus=""
 > > +	while true; do
 > > +		iface=`ng_create_one fec dummy fec`
 > > +		if [ -z "${iface}" ]; then
 > > +			exit 2
 > > +		fi
 > > +		if [ "${iface}" = "${req_iface}" ]; then
 > > +			echo ${iface}
 > > +			break
 > > +		fi
 > > +		bogus="${bogus} ${iface}"
 > > +	done
 > > +	for iface in ${bogus}; do
 > > +		ngctl shutdown ${iface}:
 > > +	done
 > 
 > These loops are a bit confusing. If I understand correctly, you're
 > creating interfaces until they reach the right number and then you
 > delete all the ones which have been created unnecessarily? Could it be
 
 	Your understanding is right:-).  But we cannot control unit number
 	in 'ngctl mkpeer', because 'Find the first free unit number for a
 	new interface' strategy in sys/netgraph/ng_fec.c#ng_fec_get_unit.
 
 > that iface is higher than req_iface (which would loop undefinitely)?
 
 	Previously, I removed req_iface by ngctl shutdown.  So not reache
 	infinity:-).
 
 > > +# fec_up ifn
 > > +# Configure Fast EtherChannel for interface $ifn. Returns 0 if FEC
 > > +# arguments were found and configured; returns 1 otherwise.
 > > +fec_up() {
 > > +	case ${fec_interfaces} in
 > > +	[Nn][Oo] | '')
 > > +		;;
 > What's the point of this? The 'case' seems useless to me. Just got with
 > the 'for' loop. If it's an empty list, then it just won't do anything.
 > Default has to be '' and not 'NO' (but it seems more sensible anyway).
 
 	I obtained gif_up code.  I don't know why/where are problem in it.
 
 > > --- defaults/rc.conf	20 Jan 2007 04:24:19 -0000	1.303
 > > +++ defaults/rc.conf	28 Jan 2007 14:52:36 -0000
 > > @@ -183,6 +183,10 @@
 > >  				# Choose correct tunnel addrs.
 > >  #gifconfig_gif0="10.1.1.1 10.1.2.1"	# Examples typically for a router.
 > >  #gifconfig_gif1="10.1.1.2 10.1.2.2"	# Examples typically for a router.
 > > +fec_interfaces="NO"		# List of Fast EtherChannels (or "NO")
 > Set to '' instead of 'NO' as explained above.
 
 	same as gif_interfaces, too.