Date: Mon, 29 Jan 2007 17:15:51 +0000 From: Florent Thoumie <flz@FreeBSD.org> To: Norikatsu Shigemura <nork@FreeBSD.org> Cc: freebsd-rc@FreeBSD.org, bug-followup@FreeBSD.org Subject: Re: conf/104884: Add support EtherChannel configuration to rc.conf Message-ID: <45BE2BC7.30406@FreeBSD.org> In-Reply-To: <200701291700.l0TH0gaR074811@freefall.freebsd.org> References: <200701291700.l0TH0gaR074811@freefall.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig3C1315AA345CB877ABCD4D34 Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Norikatsu Shigemura wrote: > The following reply was made to PR conf/104884; it has been noted by GN= ATS. >=20 > From: Norikatsu Shigemura <nork@FreeBSD.org> > To: Florent Thoumie <flz@FreeBSD.org> > Cc: Norikatsu Shigemura <nork@FreeBSD.org>, freebsd-bugs@FreeBSD.org, > FreeBSD-gnats-submit@FreeBSD.org, freebsd-rc@FreeBSD.org > Subject: Re: conf/104884: Add support EtherChannel configuration to rc.= conf > Date: Tue, 30 Jan 2007 01:53:17 +0900 >=20 > On Sun, 28 Jan 2007 19:37:03 +0000 > Florent Thoumie <flz@freebsd.org> wrote: > > >> http://www.freebsd.org/cgi/query-pr.cgi?pr=3D104884 > > >>> Category: conf > > >>> Responsible: freebsd-bugs > > >>> Synopsis: Add support EtherChannel configuration to rc.con= f > > >>> 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 > =20 > Thanks for your handling. > =20 > > about technical details, so I'll only focus on style. Here are my > > comments on the patch: > =20 > I think that the answer of all questions is 'BECAUSE netgraph's > specification.'. :-) =2E.. which I don't really know, so better not take everything I write as= face value :-) > > > Index: network.subr > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > > 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=3D`expr "${line}" : '.* name=3D"\([a-z]*[0-9]*\)" .*'` > > > + if [ -n "${t}" ]; then > > > + echo ${t} > > > + return > > > + fi > > > + done > > > +} > =20 > I implemented ng_mkpeer and ng_create_one as generic functions. > If anyone want to add other netgraph function, they can use > these functions. I wasn't commenting this one, seemed alright to me. > > > +ng_fec_create() { > > > + local req_iface iface bogus > > > + req_iface=3D"$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 functio= n to > > other scripts? > =20 > Ah! This code's meaning was changed from original code. Yes, > I think that this code should be removed. > =20 > > > + ngctl shutdown ${req_iface}: > /dev/null 2>&1 > > > + bogus=3D"" > > > + while true; do > > > + iface=3D`ng_create_one fec dummy fec` > > > + if [ -z "${iface}" ]; then > > > + exit 2 > > > + fi > > > + if [ "${iface}" =3D "${req_iface}" ]; then > > > + echo ${iface} > > > + break > > > + fi > > > + bogus=3D"${bogus} ${iface}" > > > + done > > > + for iface in ${bogus}; do > > > + ngctl shutdown ${iface}: > > > + done > >=20 > > 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 > =20 > 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. > =20 > > that iface is higher than req_iface (which would loop undefinitely)?= > =20 > Previously, I removed req_iface by ngctl shutdown. So not reache > infinity:-). Fair enough. > > > +# fec_up ifn > > > +# Configure Fast EtherChannel for interface $ifn. Returns 0 if FE= C > > > +# 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 w= ith > > the 'for' loop. If it's an empty list, then it just won't do anythin= g. > > Default has to be '' and not 'NO' (but it seems more sensible anyway= ). > =20 > I obtained gif_up code. I don't know why/where are problem in it. No problem, just seemed bad style to me :-) --=20 Florent Thoumie flz@FreeBSD.org FreeBSD Committer --------------enig3C1315AA345CB877ABCD4D34 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.5 (FreeBSD) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFFvivMMxEkbVFH3PQRCny3AJ4xgCVFGTFMEKVlG9xIxSpVZHbCywCeOhSh sod+QxcYxMjAkupUSK7ph/o= =5W2+ -----END PGP SIGNATURE----- --------------enig3C1315AA345CB877ABCD4D34--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?45BE2BC7.30406>