From owner-freebsd-bugs@FreeBSD.ORG Mon Jan 29 16:53:19 2007 Return-Path: X-Original-To: freebsd-bugs@FreeBSD.org Delivered-To: freebsd-bugs@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 2634016A400; Mon, 29 Jan 2007 16:53:19 +0000 (UTC) (envelope-from nork@FreeBSD.org) Received: from sakura.ninth-nine.com (sakura.ninth-nine.com [219.127.74.120]) by mx1.freebsd.org (Postfix) with ESMTP id A46E413C4A6; Mon, 29 Jan 2007 16:53:18 +0000 (UTC) (envelope-from nork@FreeBSD.org) Received: from nadesico.ninth-nine.com (nadesico.ninth-nine.com [219.127.74.122]) by sakura.ninth-nine.com (8.13.8/8.13.8/NinthNine) with SMTP id l0TGrH5p042100; Tue, 30 Jan 2007 01:53:17 +0900 (JST) (envelope-from nork@FreeBSD.org) Date: Tue, 30 Jan 2007 01:53:17 +0900 From: Norikatsu Shigemura To: Florent Thoumie Message-Id: <20070130015317.6c052297.nork@FreeBSD.org> In-Reply-To: <45BCFB5F.2040509@FreeBSD.org> References: <20061029010934.5afef73e.nork@FreeBSD.org> <200610281610.k9SGAIVb051055@freefall.freebsd.org> <20070129000459.b2dba4e0.nork@FreeBSD.org> <45BCFB5F.2040509@FreeBSD.org> X-Mailer: Sylpheed 2.3.0rc (GTK+ 2.10.9; i386-portbld-freebsd6.2) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-2.0.2 (sakura.ninth-nine.com [219.127.74.121]); Tue, 30 Jan 2007 01:53:17 +0900 (JST) Cc: freebsd-bugs@FreeBSD.org, FreeBSD-gnats-submit@FreeBSD.org, freebsd-rc@FreeBSD.org, Norikatsu Shigemura Subject: Re: conf/104884: Add support EtherChannel configuration to rc.conf X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 29 Jan 2007 16:53:19 -0000 On Sun, 28 Jan 2007 19:37:03 +0000 Florent Thoumie 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 < > +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.