Bug 63700

Oops, I anonymized the example in a bogus way, by having forgotten
to change the addresses in the output section as well.  Now I've
got relief of my paranoia and so I'm able to say, in order to prevent
confusion, that the rc.conf snippet should have read:

%%% rc.conf snippet %%%

### network_interfaces="lo0 fxp0"
cloned_interfaces="vlan99"
ifconfig_fxp0="inet 158.250.32.111 netmask 255.255.255.224"
ifconfig_vlan99="inet 169.254.1.1 netmask 255.255.0.0 vlan 99 vlandev fxp1"

%%% end %%%

However, that doesn't really matter for the issue raised in this PR.

-- 
Yar

Comment 2 Brooks Davis 2004-07-23 18:21:10 UTC

Responsible Changed
From-To: freebsd-bugs->brooks

I'll take this one, I've got some work to do on interface config and 
this is probably my bug.

I've got tired of the bug and knocked up a patch :-)
What do you think about it?  I'd like to commit it sooner than later
so that it can appear in 5.3.

-- 
Yar

Index: network.subr
===================================================================
RCS file: /home/ncvs/src/etc/network.subr,v
retrieving revision 1.155
diff -u -p -r1.155 network.subr
--- network.subr	27 Aug 2004 12:11:47 -0000	1.155
+++ network.subr	27 Aug 2004 13:47:13 -0000
@@ -258,7 +258,7 @@ ipx_down()
 #		nodhcp - all interfaces, excluding DHCP configured interfaces
 #		dhcp   - list only DHCP configured interfaces
 #	If no argument is specified all network interfaces are output.
-#	Note that the list always includes cloned interfaces.
+#	Note that the list will include cloned interfaces if applicable.
 #
 list_net_interfaces()
 {
@@ -269,12 +269,15 @@ list_net_interfaces()
 	case ${network_interfaces} in
 	[Aa][Uu][Tt][Oo])
 		_tmplist="`ifconfig -l`"
+		_pattern="`echo $_tmplist | tr ' ' '\n'`"
+		_tmpcloned="`echo ${cloned_interfaces} | tr ' ' '\n' | \
+			grep -Fvx \"${_pattern}\" | tr '\n' ' '`"
+		_tmplist="${_tmplist} ${_tmpcloned}"
 		;;
 	*)
-		_tmplist="${network_interfaces}"
+		_tmplist="${network_interfaces} ${cloned_interfaces}"
 		;;
 	esac
-	_tmplist="${_tmplist} ${cloned_interfaces}"
 
 	if [ -z "$type" ]; then
 		echo $_tmplist

On Fri, Aug 27, 2004 at 05:57:00PM +0400, Yar Tikhiy wrote:
> I've got tired of the bug and knocked up a patch :-)
> What do you think about it?  I'd like to commit it sooner than later
> so that it can appear in 5.3.

Unfortunatly, we can't use grep here because interface configuration
must take place before mountcritremote and mountcritremote may be where
we get /usr.  I think what we need is a list merge function.  I'll hack
one up shortly.

-- Brooks

On Fri, Aug 27, 2004 at 05:57:00PM +0400, Yar Tikhiy wrote:
> I've got tired of the bug and knocked up a patch :-)
> What do you think about it?  I'd like to commit it sooner than later
> so that it can appear in 5.3.

Here's an untested (I tested the dedup fuction, but not the rc scripts)
patch that should fix the problem.  It will be lame with really massive
numbers of interfaces since dedup is O(n^2), but that's unavoidable with
/bin/sh.

-- Brooks

==== //depot/user/brooks/cleanup/etc/network.subr#2 - /home/brooks/working/freebsd/p4/cleanup/etc/network.subr ====
@@ -274,7 +274,7 @@
 		_tmplist="${network_interfaces}"
 		;;
 	esac
-	_tmplist="${_tmplist} ${cloned_interfaces}"
+	_tmplist=`dedup ${_tmplist} ${cloned_interfaces}`
 
 	if [ -z "$type" ]; then
 		echo $_tmplist
==== //depot/user/brooks/cleanup/etc/rc.subr#10 - /home/brooks/working/freebsd/p4/cleanup/etc/rc.subr ====
@@ -1297,4 +1297,23 @@
 	/sbin/mdmfs $bpi -s $1 -M md $2
 }
 
+# Remove duplicates from a list without using tools in /usr
+dedup()
+{
+	local newlist=""
+	for candidate in $*; do 
+		dup=0
+		for item in ${newlist}; do
+			if [ ${candidate} = ${item} ]; then
+				dup=1
+				break
+			fi
+		done
+		if [ ${dup} -eq 0 ]; then
+			newlist="${newlist} ${candidate}"
+		fi
+	done
+	echo ${newlist}
+}
+
 fi

On Fri, Aug 27, 2004 at 12:15:14PM -0700, Brooks Davis wrote:
> On Fri, Aug 27, 2004 at 05:57:00PM +0400, Yar Tikhiy wrote:
> > I've got tired of the bug and knocked up a patch :-)
> > What do you think about it?  I'd like to commit it sooner than later
> > so that it can appear in 5.3.
> 
> Here's an untested (I tested the dedup fuction, but not the rc scripts)
> patch that should fix the problem.  It will be lame with really massive
> numbers of interfaces since dedup is O(n^2), but that's unavoidable with
> /bin/sh.

First of all, thank you for pointing me out that I shouldn't have used
tools from /usr there.  I missed that.

I tested your dedup, and it appeared rather slow :-(  I.e. it took 6
sec to dedup 500 unique words (which is the worst case) on a 300MHz
machine, and it took 26 sec to dedup 1000 words.  AFAIK people are
running huge numbers of vlan or similar interfaces on rather slow-cpu
routers, and list_net_interfaces is called more than once from rc
scripts...

What do you think about another approach shown below?  It is really
fast: 500 words - 0.3 sec; 1000 words - 0.7 sec; 5000 words - 19 sec
(in the latter case it hits some limit inside sh like var name hash
quality, I guess.)  My version of dedup has a limitation: it
will accept only words that are valid sh identifier substrings.
However, for interface names it's not an issue as long as rc.conf
uses ifconfig_${ifname} variables.

BTW, with all the problems we face when trying to dedup a list using
/bin only, wouldn't it better just to use `ifconfig -l` as the list
of all interfaces in the system if network_interfaces=auto?  After all,
cloned interfaces should have been created earlier.  I cannot imagine
rc code that will accept a list of interfaces, _some_ of which do not
exist yet, and do a right thing...

-- 
Yar

--- rc.subr.orig	Fri Aug 20 02:26:00 2004
+++ rc.subr	Sat Aug 28 02:07:59 2004
@@ -1297,4 +1297,18 @@
 	/sbin/mdmfs $bpi -s $1 -M md $2
 }
 
+# Remove duplicates from a list without using tools in /usr
+dedup()
+{
+	local newlist=""
+	for candidate in $*; do 
+		eval local seen=\$seen_${candidate}
+		if [ -z "${seen}" ]; then
+			newlist="${newlist} ${candidate}"
+			eval local seen_${candidate}=1
+		fi
+	done
+	echo ${newlist}
+}
+
 fi

On Sat, Aug 28, 2004 at 02:39:30AM +0400, Yar Tikhiy wrote:
> On Fri, Aug 27, 2004 at 12:15:14PM -0700, Brooks Davis wrote:
> > On Fri, Aug 27, 2004 at 05:57:00PM +0400, Yar Tikhiy wrote:
> > > I've got tired of the bug and knocked up a patch :-)
> > > What do you think about it?  I'd like to commit it sooner than later
> > > so that it can appear in 5.3.
> > 
> > Here's an untested (I tested the dedup fuction, but not the rc scripts)
> > patch that should fix the problem.  It will be lame with really massive
> > numbers of interfaces since dedup is O(n^2), but that's unavoidable with
> > /bin/sh.
> 
> First of all, thank you for pointing me out that I shouldn't have used
> tools from /usr there.  I missed that.
> 
> I tested your dedup, and it appeared rather slow :-(  I.e. it took 6
> sec to dedup 500 unique words (which is the worst case) on a 300MHz
> machine, and it took 26 sec to dedup 1000 words.  AFAIK people are
> running huge numbers of vlan or similar interfaces on rather slow-cpu
> routers, and list_net_interfaces is called more than once from rc
> scripts...

That's the best you can do in the general case unless you want to
implement an efficent sort alorithm in sh which will take you from
O(n^2) to O(n log n). :-(

> What do you think about another approach shown below?  It is really
> fast: 500 words - 0.3 sec; 1000 words - 0.7 sec; 5000 words - 19 sec
> (in the latter case it hits some limit inside sh like var name hash
> quality, I guess.)  My version of dedup has a limitation: it
> will accept only words that are valid sh identifier substrings.
> However, for interface names it's not an issue as long as rc.conf
> uses ifconfig_${ifname} variables.

That would be OK in this case.  If only we has sort this would be easy
since we could sort the list and then remove the duplicates.

> BTW, with all the problems we face when trying to dedup a list using
> /bin only, wouldn't it better just to use `ifconfig -l` as the list
> of all interfaces in the system if network_interfaces=auto?  After all,
> cloned interfaces should have been created earlier.  I cannot imagine
> rc code that will accept a list of interfaces, _some_ of which do not
> exist yet, and do a right thing...

I wondered about that my self.  I can't think of a good reason not to
ignore cloned_interfaces in the auto case.

-- Brooks

On Fri, Aug 27, 2004 at 04:01:59PM -0700, Brooks Davis wrote:
> 
> > BTW, with all the problems we face when trying to dedup a list using
> > /bin only, wouldn't it better just to use `ifconfig -l` as the list
> > of all interfaces in the system if network_interfaces=auto?  After all,
> > cloned interfaces should have been created earlier.  I cannot imagine
> > rc code that will accept a list of interfaces, _some_ of which do not
> > exist yet, and do a right thing...
> 
> I wondered about that my self.  I can't think of a good reason not to
> ignore cloned_interfaces in the auto case.

Then let's ignore it :-)  The auto case is special enough for
either way to have its complications, and ignoring cloned_interfaces
is at least simple and clear, unlike deduping the resulting list.
Shall I commit the below patch?

-- 
Yar

Index: network.subr
===================================================================
RCS file: /home/ncvs/src/etc/network.subr,v
retrieving revision 1.155
diff -u -p -r1.155 network.subr
--- network.subr	27 Aug 2004 12:11:47 -0000	1.155
+++ network.subr	28 Aug 2004 00:16:56 -0000
@@ -258,7 +258,9 @@ ipx_down()
 #		nodhcp - all interfaces, excluding DHCP configured interfaces
 #		dhcp   - list only DHCP configured interfaces
 #	If no argument is specified all network interfaces are output.
-#	Note that the list always includes cloned interfaces.
+#	Note that the list will include cloned interfaces if applicable.
+#	Cloned interfaces must already exist to have a chance to appear
+#	in the list if ${network_interfaces} is set to `auto'.
 #
 list_net_interfaces()
 {
@@ -271,10 +273,9 @@ list_net_interfaces()
 		_tmplist="`ifconfig -l`"
 		;;
 	*)
-		_tmplist="${network_interfaces}"
+		_tmplist="${network_interfaces} ${cloned_interfaces}"
 		;;
 	esac
-	_tmplist="${_tmplist} ${cloned_interfaces}"
 
 	if [ -z "$type" ]; then
 		echo $_tmplist

On Sat, Aug 28, 2004 at 04:28:21AM +0400, Yar Tikhiy wrote:
> On Fri, Aug 27, 2004 at 04:01:59PM -0700, Brooks Davis wrote:
> > 
> > > BTW, with all the problems we face when trying to dedup a list using
> > > /bin only, wouldn't it better just to use `ifconfig -l` as the list
> > > of all interfaces in the system if network_interfaces=auto?  After all,
> > > cloned interfaces should have been created earlier.  I cannot imagine
> > > rc code that will accept a list of interfaces, _some_ of which do not
> > > exist yet, and do a right thing...
> > 
> > I wondered about that my self.  I can't think of a good reason not to
> > ignore cloned_interfaces in the auto case.
> 
> Then let's ignore it :-)  The auto case is special enough for
> either way to have its complications, and ignoring cloned_interfaces
> is at least simple and clear, unlike deduping the resulting list.
> Shall I commit the below patch?

Sounds good to me.  Please go ahead.

-- Brooks

Comment 10 Yar Tikhiy 2004-08-28 09:01:23 UTC

State Changed
From-To: open->patched

Fixed in CURRENT. 

Comment 11 Yar Tikhiy 2004-08-28 09:01:23 UTC

Responsible Changed
From-To: brooks->yar

Thank you Brooks, it was a pleasure to work arm in arm with you 
on this problem!  Now I'll lead it to the end of its life cycle.

Comment 12 Yar Tikhiy 2004-09-28 03:11:12 UTC

State Changed
From-To: patched->closed

Merged to RELENG_5.