Re: 7.0 - ifconfig create is not working as expected?

2008-03-30 Thread Eugene Grosbein
On Sat, Mar 29, 2008 at 03:43:44PM -0500, Brooks Davis wrote:

  I was using following command in FreeBSD 6.2:
  # ifconfig lo1 create inet 172.16.16.2 netmask 255.255.255.0
  In FreeBSD 7.0 I got an error:
  # ifconfig lo1 create inet 172.16.16.2 netmask 255.255.255.0
  ifconfig: inet: bad value
  But it is working splitted in to two commands:
  # ifconfig lo1 create
  # ifconfig lo1 inet 172.16.16.2 netmask 255.255.255.0
  Is this expected behavior or should I file a PR?
 This expected.  There's some argument it's wrong, but filing a PR is
 unlikely to cause it to change any time soon.

Why? The same with creating gif-tunnel, now I need to invoke ifconfig
twice, once for 'create' and once for other tunnel parameters,
whereas for RELENG_6 this works: 'ifconfig gif0 create tunnel 1.1.1.1 2.2.2.2' 

This breaks existing setups/scripts. This is POLA issue.
Why was it broken?

Eugene Grosbein
___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to [EMAIL PROTECTED]


Multiple netgraph threads

2008-03-30 Thread Alexander Motin

Hi.

I have implemented a patch (for the HEAD) making netgraph to use several 
own threads for event queues processing instead of using single swinet. 
It should significantly improve netgraph SMP scalability on complicated 
workloads that require queueing by implementation (PPTP/L2TP) or stack 
size limitations. It works perfectly on my UP system, showing results 
close to original or even a bit better. I have no real SMP test server 
to measure real scalability, but test on HTT CPU works fine, utilizing 
both virtual cores at the predictable level.


Reviews and feedbacks are welcome.

URL: http://people.freebsd.org/~mav/netgraph.threads.patch

--
Alexander Motin
___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: kern/122065: [gre] gre over ipsec not working

2008-03-30 Thread bz
Synopsis: [gre] gre over ipsec not working

State-Changed-From-To: open-feedback
State-Changed-By: bz
State-Changed-When: Sun Mar 30 09:46:04 UTC 2008
State-Changed-Why: 
I am exchanging mails with the submitter to narrow down the problem.


Responsible-Changed-From-To: freebsd-net-bz
Responsible-Changed-By: bz
Responsible-Changed-When: Sun Mar 30 09:46:04 UTC 2008
Responsible-Changed-Why: 
I am exchanging mails with the submitter to narrow down the problem.

http://www.freebsd.org/cgi/query-pr.cgi?pr=122065
___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: Multiple netgraph threads

2008-03-30 Thread Alexander Motin

Hans Petter Selasky wrote:
Have you thought about nodes that lock the same mutex must be run on the same 
thread else for example one thread will run while another will just waits for 
a mutex ?


Usually different nodes does not share data, keeping them inside 
node/hook private data, so I don't see problem here. It is possible that 
two messages will be queued to the same node at same time, but to fulfil 
serialization requirements each node queue processed only by one thread 
at a time, so there is no place for congestion.


You can achieve this by grouping nodes into a tree, and the node at the top of 
the tree decides on which thread the nodes in the tree should be run.


Netgraph graphs usually not linear and it is from hard to impossible to 
predict the way execution will go and the locks that may be congested. 
Including usually big number of nodes and usually small lock time, 
congestion probability IMHO looks insignificant comparing to additional 
logic overhead. If some node/hook needs big lock time it may be instead 
declared as FORCE_WRITER to enqueue congested messages instead of 
blocking them (such way now implemented all existing ppp 
compression/encryption nodes).


--
Alexander Motin
___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: Multiple netgraph threads

2008-03-30 Thread Hans Petter Selasky
Hi,

Have you thought about nodes that lock the same mutex must be run on the same 
thread else for example one thread will run while another will just waits for 
a mutex ?

You can achieve this by grouping nodes into a tree, and the node at the top of 
the tree decides on which thread the nodes in the tree should be run.

How does your patch handle this ?

Also see recent discussion about multithreaded callouts 
on [EMAIL PROTECTED]. Subject timeout/callout small step forward.

--HPS

On Sunday 30 March 2008, Alexander Motin wrote:
 Hi.

 I have implemented a patch (for the HEAD) making netgraph to use several
 own threads for event queues processing instead of using single swinet.
 It should significantly improve netgraph SMP scalability on complicated
 workloads that require queueing by implementation (PPTP/L2TP) or stack
 size limitations. It works perfectly on my UP system, showing results
 close to original or even a bit better. I have no real SMP test server
 to measure real scalability, but test on HTT CPU works fine, utilizing
 both virtual cores at the predictable level.

 Reviews and feedbacks are welcome.

 URL: http://people.freebsd.org/~mav/netgraph.threads.patch
___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: 7.0 - ifconfig create is not working as expected?

2008-03-30 Thread Bruce M. Simpson

Eugene Grosbein wrote:

On Sat, Mar 29, 2008 at 03:43:44PM -0500, Brooks Davis wrote:

  

I was using following command in FreeBSD 6.2:
# ifconfig lo1 create inet 172.16.16.2 netmask 255.255.255.0
In FreeBSD 7.0 I got an error:
# ifconfig lo1 create inet 172.16.16.2 netmask 255.255.255.0
ifconfig: inet: bad value
But it is working splitted in to two commands:
# ifconfig lo1 create
# ifconfig lo1 inet 172.16.16.2 netmask 255.255.255.0
Is this expected behavior or should I file a PR?
  

This expected.  There's some argument it's wrong, but filing a PR is
unlikely to cause it to change any time soon.



Why? The same with creating gif-tunnel, now I need to invoke ifconfig
twice, once for 'create' and once for other tunnel parameters,
whereas for RELENG_6 this works: 'ifconfig gif0 create tunnel 1.1.1.1 2.2.2.2' 


This breaks existing setups/scripts. This is POLA issue.
Why was it broken?
  


I don't know why or how this has happened, however, given the complexity 
of the command line grammar which ifconfig is expected to parse, our 
choices are limited, unless someone(tm) is willing to come along and 
implement a full parser in ifconfig.


I investigated this some years ago and frankly didn't get anywhere, one 
of the constraints was that Sam wanted to modularize the ifconfig code, 
with a view to future dynamic loading -- as such, this places 
restrictions on the kind of parser which can be used.


There is valid argument that we should not do this, as ifconfig is a 
tool which sits in the base system, and should be kept simple and 
therefore small.


On the other hand, there's also the argument that as ifconfig's syntax 
has grown considerably over the years, that we should go ahead and add a 
parser anyway.


In the absence of a full-blown parser, I'm comfortable with ifconfig 
cloner create being a separate operation, which preferably throws an 
error if other commands are included with it, and understand why these 
limitations apply.


cheers
BMS
___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: Multiple netgraph threads

2008-03-30 Thread Robert Watson

On Sun, 30 Mar 2008, Alexander Motin wrote:

I have implemented a patch (for the HEAD) making netgraph to use several own 
threads for event queues processing instead of using single swinet. It 
should significantly improve netgraph SMP scalability on complicated 
workloads that require queueing by implementation (PPTP/L2TP) or stack size 
limitations. It works perfectly on my UP system, showing results close to 
original or even a bit better. I have no real SMP test server to measure 
real scalability, but test on HTT CPU works fine, utilizing both virtual 
cores at the predictable level. Reviews and feedbacks are welcome. URL: 
http://people.freebsd.org/~mav/netgraph.threads.patch


FYI, you might be interested in some similar work I've been doing in the 
rwatson_netisr branch in Perforce, which:


(1) Adds per-CPU netisr threads
(2) Introduces inpcb affinity, assigned using a hash on the tuple (similar to
RSS) to load balance
(3) Moves to rwlock use on inpcb and pcbinfo, used extensively in UDP and
somewhat in TCP

My initial leaning would be that we would like to avoid adding too many more 
threads that will do per-packet work, as that leads to excessive context 
switching.  I have similar worries regarding ithreads, and I suspect (hope?) 
we will have an active discussion of this at the BSDCan developer summit.


BTW, I'd be careful with the term should and SMP -- often times scalability 
to multiple cores involves not just introducing the opportunity for 
parallelism, but also significantly refining or changing the data model to 
allow that parallelism to be efficiently used.  Right now, despite loopback 
performance being a bottleneck with a single netisr thread, we're not seeing a 
performance improvement for database workloads over loopback with multiple 
netisr threads.  We're diagnosing this still -- initially it appeared to be 
tcbinfo lock contention (not surprising), but moving to read locking on 
tcbinfo didn't appear to help (except in reducing contention leading to more 
idle time rather than more progress).  The current theory is that something 
about the approach isn't working well with the scheduler but we need to 
analyze the scheduler traces further.  My recommendation would be that you do 
a fairly thorough performance evaluation before committing.


Robert N M Watson
Computer Laboratory
University of Cambridge
___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: 7.0 - ifconfig create is not working as expected?

2008-03-30 Thread Eugene Grosbein
 Why? The same with creating gif-tunnel, now I need to invoke ifconfig
 twice, once for 'create' and once for other tunnel parameters,
 whereas for RELENG_6 this works: 'ifconfig gif0 create tunnel 1.1.1.1 
 2.2.2.2' 
 This breaks existing setups/scripts. This is POLA issue.
 Why was it broken?
 
 I don't know why or how this has happened, however, given the complexity 
 of the command line grammar which ifconfig is expected to parse, our 
 choices are limited, unless someone(tm) is willing to come along and 
 implement a full parser in ifconfig.

It worked. Now it does not work. Someone(tm) made the change.
We have the CVS. Isn't there such thing as responsibility for changes?
A dichotomy will show us who did the change :-)

Eugene Grosbein
___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: 7.0 - ifconfig create is not working as expected?

2008-03-30 Thread Remko Lodder


On Mar 30, 2008, at 12:45 PM, Eugene Grosbein wrote:
Why? The same with creating gif-tunnel, now I need to invoke  
ifconfig

twice, once for 'create' and once for other tunnel parameters,
whereas for RELENG_6 this works: 'ifconfig gif0 create tunnel  
1.1.1.1

2.2.2.2'
This breaks existing setups/scripts. This is POLA issue.
Why was it broken?


I don't know why or how this has happened, however, given the  
complexity

of the command line grammar which ifconfig is expected to parse, our
choices are limited, unless someone(tm) is willing to come along and
implement a full parser in ifconfig.


It worked. Now it does not work. Someone(tm) made the change.
We have the CVS. Isn't there such thing as responsibility for changes?
A dichotomy will show us who did the change :-)

Eugene Grosbein


Given that the idea is that we dont expect to get to this anytime  
soon, we welcome the person who does the analysis for us so that we  
might be able to fix this quicker (if possible with all the changes  
involved).


Thanks,
remko

--
/\   Best regards,| [EMAIL PROTECTED]
\ /   Remko Lodder  | [EMAIL PROTECTED]
Xhttp://www.evilcoder.org/  |
/ \   ASCII Ribbon Campaign | Against HTML Mail and News

___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: Multiple netgraph threads

2008-03-30 Thread Alexander Motin

Robert Watson wrote:
 FYI, you might be interested in some similar work I've been doing
 in the rwatson_netisr branch in Perforce, which:
 Adds per-CPU netisr threads

Thanks. Netgraph from the beginning uses concept of direct function 
calls, when level of parallelism limited by data source. In that point 
multiple netisr threads will give benefits.


My initial leaning would be that we would like to avoid adding too many 
more threads that will do per-packet work, as that leads to excessive 
context switching.


Netgraph uses queueing only as last resort, when direct call is not 
possible due to locking or stack limitations. For example, while working 
with kernel sockets (*upcall)() I have got many issues which make 
impossible to freely use received data without queueing as upcall() 
caller holds some locks leading to unpredicted LORs in socket/TCP/UDP code.
In case of such forced queueing, node becomes an independent data source 
which can be pinned to and processed by whatever specialized thread or 
netisr, when it will be able to do it more effectively.


--
Alexander Motin
___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: 7.0 - ifconfig create is not working as expected?

2008-03-30 Thread Eugene Grosbein
On Sun, Mar 30, 2008 at 01:14:36PM +0200, Remko Lodder wrote:

 It worked. Now it does not work. Someone(tm) made the change.
 We have the CVS. Isn't there such thing as responsibility for changes?
 A dichotomy will show us who did the change :-)
 
 Given that the idea is that we dont expect to get to this anytime  
 soon, we welcome the person who does the analysis for us so that we  
 might be able to fix this quicker (if possible with all the changes  
 involved).

The revision 1.120 of src/sbin/ifconfig broke this:
cvs update -D '2006/07/09 06:00:00' gives us ifconfig binary
what works and cvs update -D '2006/07/10 00:00:00' gives one
that fails. The commit deals with interface cloning.

Now I'm looking for the fix. This is POLA issue and such regressions
should not be tolerated, there are already too many of them.

Eugene Grosbein
___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: 7.0 - ifconfig create is not working as expected?

2008-03-30 Thread Eugene Grosbein
On Sun, Mar 30, 2008 at 07:39:23PM +0800, Eugene Grosbein wrote:

 The revision 1.120 of src/sbin/ifconfig broke this:
 cvs update -D '2006/07/09 06:00:00' gives us ifconfig binary
 what works and cvs update -D '2006/07/10 00:00:00' gives one
 that fails. The commit deals with interface cloning.

Read: ... of src/sbin/ifconfig.c

Eugene Grosbein
___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: 7.0 - ifconfig create is not working as expected?

2008-03-30 Thread Eugene Grosbein
On Sun, Mar 30, 2008 at 01:14:36PM +0200, Remko Lodder wrote:

 It worked. Now it does not work. Someone(tm) made the change.
 We have the CVS. Isn't there such thing as responsibility for changes?
 A dichotomy will show us who did the change :-)
 
 Given that the idea is that we dont expect to get to this anytime  
 soon, we welcome the person who does the analysis for us so that we  
 might be able to fix this quicker (if possible with all the changes  
 involved).

Let's take a look to ifconfig(8) manual page:

SYNOPSIS
ifconfig [-L] [-k] [-m] [-n] interface [create] [address_family] [address
  [dest_address]] [parameters]

It states that create word may be used with address family,
adresses and other paramerets in line. Now there is design problem brought
into ifconfig code with sam's commit 2006/07/09. Current code
defers device cloning using callback facility because vlan creation
procedure needs to gather vlan tag and parent device parameners before.

However, it means that create inet or create tunnel clauses
now became incorrect. So this change broke things and should be corrected.
Basically, it broke all setups for sake of vlan processing.
It seems that vlans should be processed as special case leaving
all other cases in peace.

Eugene Grosbein
___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: Multiple netgraph threads

2008-03-30 Thread Robert Watson

On Sun, 30 Mar 2008, Alexander Motin wrote:

My initial leaning would be that we would like to avoid adding too many 
more threads that will do per-packet work, as that leads to excessive 
context switching.


Netgraph uses queueing only as last resort, when direct call is not possible 
due to locking or stack limitations. For example, while working with kernel 
sockets (*upcall)() I have got many issues which make impossible to freely 
use received data without queueing as upcall() caller holds some locks 
leading to unpredicted LORs in socket/TCP/UDP code. In case of such forced 
queueing, node becomes an independent data source which can be pinned to and 
processed by whatever specialized thread or netisr, when it will be able to 
do it more effectively.


I guess my caution is that it does not necessarily follow from a design that 
allows for explicit parallelism that the implementation will use it well, and 
that any time context switchs are necessarily introduced, cost goes up.  The 
move to direct dispatch from the ithread, despite reducing opportunities for 
parallelism, significantly increased performance for many local workloads. 
If we have a netisr thread, an ithread, and a netgraph thread, the potential 
context switch overhead is significant, even if we are doing a good job at 
batching work transfer between them.  Often times, the way this behaves in 
practice is quite dependent on scheduling, and right now we have known 
defficiencies in this area, so give it a try on an SMP box and see what 
happens.  Since you're a FreeBSD committer, you can sign up to use the netperf 
cluster, which might not be a bad idea if you don't have local access to a 
good SMP test setup.


Robert N M Watson
Computer Laboratory
University of Cambridge
___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: 7.0 - ifconfig create is not working as expected?

2008-03-30 Thread Eugene Grosbein
On Sun, Mar 30, 2008 at 01:14:36PM +0200, Remko Lodder wrote:

 Given that the idea is that we dont expect to get to this anytime  
 soon, we welcome the person who does the analysis for us so that we  
 might be able to fix this quicker (if possible with all the changes  
 involved).

Here is a patch for RELENG_7. I ask Miroslav Lachman to test it.
Apply:

cd /usr/src/sbin/ifconfig
patch  /path/to/patchfile
make

Test:

./ifconfig lo1 create inet 5.5.5.5 netmask 255.255.255.0

Or full-blown syntax:

./ifconfig gif0 create inet 6.6.6.6 7.7.7.7 tunnel 1.1.1.1 2.2.2.2 \
netmask 255.255.255.255 mtu 1500 link2

Index: ifclone.c
===
RCS file: /home/ncvs/src/sbin/ifconfig/ifclone.c,v
retrieving revision 1.3
diff -u -r1.3 ifclone.c
--- ifclone.c   12 Aug 2006 18:07:17 -  1.3
+++ ifclone.c   30 Mar 2008 14:19:08 -
@@ -131,7 +131,9 @@
 static
 DECL_CMD_FUNC(clone_create, arg, d)
 {
-   callback_register(ifclonecreate, NULL);
+   if (strstr(name, vlan) == name)
+   callback_register(ifclonecreate, NULL);
+   else ifclonecreate(s, NULL);
 }
 
 static
Index: ifconfig.c
===
RCS file: /home/ncvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.134
diff -u -r1.134 ifconfig.c
--- ifconfig.c  4 Oct 2007 09:45:41 -   1.134
+++ ifconfig.c  30 Mar 2008 14:22:00 -
@@ -247,7 +247,12 @@
if (iflen = sizeof(name))
errx(1, %s: cloning name too long,
ifname);
-   ifconfig(argc, argv, NULL);
+   if (argc  1) {
+   afp = af_getbyname(argv[1]);
+   if (afp != NULL)
+   argv[1] = NULL;
+   }
+   ifconfig(argc, argv, afp);
exit(0);
}
errx(1, interface %s does not exist, ifname);
@@ -451,6 +456,9 @@
while (argc  0) {
const struct cmd *p;
 
+   if(*argv == NULL)
+   goto next;
+   
p = cmd_lookup(*argv);
if (p == NULL) {
/*
@@ -479,6 +487,7 @@
} else
p-c_u.c_func(*argv, p-c_parameter, s, afp);
}
+   next:
argc--, argv++;
}
 
___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: 7.0 - ifconfig create is not working as expected?

2008-03-30 Thread Sam Leffler

Eugene Grosbein wrote:

On Sun, Mar 30, 2008 at 01:14:36PM +0200, Remko Lodder wrote:

  
Given that the idea is that we dont expect to get to this anytime  
soon, we welcome the person who does the analysis for us so that we  
might be able to fix this quicker (if possible with all the changes  
involved).



Here is a patch for RELENG_7. I ask Miroslav Lachman to test it.
Apply:

cd /usr/src/sbin/ifconfig
patch  /path/to/patchfile
make

Test:

./ifconfig lo1 create inet 5.5.5.5 netmask 255.255.255.0

Or full-blown syntax:

./ifconfig gif0 create inet 6.6.6.6 7.7.7.7 tunnel 1.1.1.1 2.2.2.2 \
netmask 255.255.255.255 mtu 1500 link2

Index: ifclone.c
===
RCS file: /home/ncvs/src/sbin/ifconfig/ifclone.c,v
retrieving revision 1.3
diff -u -r1.3 ifclone.c
--- ifclone.c   12 Aug 2006 18:07:17 -  1.3
+++ ifclone.c   30 Mar 2008 14:19:08 -
@@ -131,7 +131,9 @@
 static
 DECL_CMD_FUNC(clone_create, arg, d)
 {
-   callback_register(ifclonecreate, NULL);
+   if (strstr(name, vlan) == name)
+   callback_register(ifclonecreate, NULL);
+   else ifclonecreate(s, NULL);
  


This breaks other cloning operations (e.g. wlan vaps that are about to 
show up in HEAD).  In general it is wrong to embed knowledge about one 
type of cloning op in the common clone code.  If you want to add the 
notion of cloning operations that should be done immediately vs. ones 
that should be deferred then do it generically; not by hacks like this.  
Understand however that now !vlan clone operations behave differently 
than vlans and many people will be utterly confused by the inconsistency.



 }
 
 static

Index: ifconfig.c
===
RCS file: /home/ncvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.134
diff -u -r1.134 ifconfig.c
--- ifconfig.c  4 Oct 2007 09:45:41 -   1.134
+++ ifconfig.c  30 Mar 2008 14:22:00 -
@@ -247,7 +247,12 @@
if (iflen = sizeof(name))
errx(1, %s: cloning name too long,
ifname);
-   ifconfig(argc, argv, NULL);
+   if (argc  1) {
+   afp = af_getbyname(argv[1]);
+   if (afp != NULL)
+   argv[1] = NULL;
+   }
+   ifconfig(argc, argv, afp);
exit(0);
}
errx(1, interface %s does not exist, ifname);
@@ -451,6 +456,9 @@
while (argc  0) {
const struct cmd *p;
 
+		if(*argv == NULL)

+   goto next;
+		
 		p = cmd_lookup(*argv);

if (p == NULL) {
/*
@@ -479,6 +487,7 @@
} else
p-c_u.c_func(*argv, p-c_parameter, s, afp);
}
+   next:
argc--, argv++;
}
  


Aside from not maintaining prevailing style and breaking cloning of 
other devices you seem to understand the issue.  How to handle it is 
however unclear.  I considered making 2 passes over the arguments to 
collect those required for a clone operation but never got to it.  That 
still seems like the correct approach.


   Sam

___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: 7.0 - ifconfig create is not working as expected?

2008-03-30 Thread Sam Leffler

Sam Leffler wrote:

Eugene Grosbein wrote:

On Sun, Mar 30, 2008 at 01:14:36PM +0200, Remko Lodder wrote:

 
Given that the idea is that we dont expect to get to this anytime  
soon, we welcome the person who does the analysis for us so that we  
might be able to fix this quicker (if possible with all the changes  
involved).



Here is a patch for RELENG_7. I ask Miroslav Lachman to test it.
Apply:

cd /usr/src/sbin/ifconfig
patch  /path/to/patchfile
make

Test:

./ifconfig lo1 create inet 5.5.5.5 netmask 255.255.255.0

Or full-blown syntax:

./ifconfig gif0 create inet 6.6.6.6 7.7.7.7 tunnel 1.1.1.1 2.2.2.2 \
netmask 255.255.255.255 mtu 1500 link2

Index: ifclone.c
===
RCS file: /home/ncvs/src/sbin/ifconfig/ifclone.c,v
retrieving revision 1.3
diff -u -r1.3 ifclone.c
--- ifclone.c12 Aug 2006 18:07:17 -1.3
+++ ifclone.c30 Mar 2008 14:19:08 -
@@ -131,7 +131,9 @@
 static
 DECL_CMD_FUNC(clone_create, arg, d)
 {
-callback_register(ifclonecreate, NULL);
+if (strstr(name, vlan) == name)
+callback_register(ifclonecreate, NULL);
+else ifclonecreate(s, NULL);
  


This breaks other cloning operations (e.g. wlan vaps that are about to 
show up in HEAD).  In general it is wrong to embed knowledge about one 
type of cloning op in the common clone code.  If you want to add the 
notion of cloning operations that should be done immediately vs. ones 
that should be deferred then do it generically; not by hacks like 
this.  Understand however that now !vlan clone operations behave 
differently than vlans and many people will be utterly confused by the 
inconsistency.



 }
 
 static

Index: ifconfig.c
===
RCS file: /home/ncvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.134
diff -u -r1.134 ifconfig.c
--- ifconfig.c4 Oct 2007 09:45:41 -1.134
+++ ifconfig.c30 Mar 2008 14:22:00 -
@@ -247,7 +247,12 @@
 if (iflen = sizeof(name))
 errx(1, %s: cloning name too long,
 ifname);
-ifconfig(argc, argv, NULL);
+if (argc  1) {
+afp = af_getbyname(argv[1]);
+if (afp != NULL)
+argv[1] = NULL;
+}
+ifconfig(argc, argv, afp);
 exit(0);
 }
 errx(1, interface %s does not exist, ifname);
@@ -451,6 +456,9 @@
 while (argc  0) {
 const struct cmd *p;
 
+if(*argv == NULL)

+goto next;
+ p = cmd_lookup(*argv);
 if (p == NULL) {
 /*
@@ -479,6 +487,7 @@
 } else
 p-c_u.c_func(*argv, p-c_parameter, s, afp);
 }
+next:
 argc--, argv++;
 }
  


Aside from not maintaining prevailing style and breaking cloning of 
other devices you seem to understand the issue.  How to handle it is 
however unclear.  I considered making 2 passes over the arguments to 
collect those required for a clone operation but never got to it.  
That still seems like the correct approach.


It might be simpler to just do 1 pass over the args and push the clone 
callback on the first non-clone parameter.  Right now however there's no 
way to tell what is clone-related and what is not.


   Sam

___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: 7.0 - ifconfig create is not working as expected?

2008-03-30 Thread Sam Leffler

Sam Leffler wrote:

Sam Leffler wrote:

Eugene Grosbein wrote:

On Sun, Mar 30, 2008 at 01:14:36PM +0200, Remko Lodder wrote:

 
Given that the idea is that we dont expect to get to this anytime  
soon, we welcome the person who does the analysis for us so that 
we  might be able to fix this quicker (if possible with all the 
changes  involved).



Here is a patch for RELENG_7. I ask Miroslav Lachman to test it.
Apply:

cd /usr/src/sbin/ifconfig
patch  /path/to/patchfile
make

Test:

./ifconfig lo1 create inet 5.5.5.5 netmask 255.255.255.0

Or full-blown syntax:

./ifconfig gif0 create inet 6.6.6.6 7.7.7.7 tunnel 1.1.1.1 2.2.2.2 \
netmask 255.255.255.255 mtu 1500 link2

Index: ifclone.c
===
RCS file: /home/ncvs/src/sbin/ifconfig/ifclone.c,v
retrieving revision 1.3
diff -u -r1.3 ifclone.c
--- ifclone.c12 Aug 2006 18:07:17 -1.3
+++ ifclone.c30 Mar 2008 14:19:08 -
@@ -131,7 +131,9 @@
 static
 DECL_CMD_FUNC(clone_create, arg, d)
 {
-callback_register(ifclonecreate, NULL);
+if (strstr(name, vlan) == name)
+callback_register(ifclonecreate, NULL);
+else ifclonecreate(s, NULL);
  


This breaks other cloning operations (e.g. wlan vaps that are about 
to show up in HEAD).  In general it is wrong to embed knowledge about 
one type of cloning op in the common clone code.  If you want to add 
the notion of cloning operations that should be done immediately vs. 
ones that should be deferred then do it generically; not by hacks 
like this.  Understand however that now !vlan clone operations behave 
differently than vlans and many people will be utterly confused by 
the inconsistency.



 }
 
 static

Index: ifconfig.c
===
RCS file: /home/ncvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.134
diff -u -r1.134 ifconfig.c
--- ifconfig.c4 Oct 2007 09:45:41 -1.134
+++ ifconfig.c30 Mar 2008 14:22:00 -
@@ -247,7 +247,12 @@
 if (iflen = sizeof(name))
 errx(1, %s: cloning name too long,
 ifname);
-ifconfig(argc, argv, NULL);
+if (argc  1) {
+afp = af_getbyname(argv[1]);
+if (afp != NULL)
+argv[1] = NULL;
+}
+ifconfig(argc, argv, afp);
 exit(0);
 }
 errx(1, interface %s does not exist, ifname);
@@ -451,6 +456,9 @@
 while (argc  0) {
 const struct cmd *p;
 
+if(*argv == NULL)

+goto next;
+ p = cmd_lookup(*argv);
 if (p == NULL) {
 /*
@@ -479,6 +487,7 @@
 } else
 p-c_u.c_func(*argv, p-c_parameter, s, afp);
 }
+next:
 argc--, argv++;
 }
  


Aside from not maintaining prevailing style and breaking cloning of 
other devices you seem to understand the issue.  How to handle it is 
however unclear.  I considered making 2 passes over the arguments to 
collect those required for a clone operation but never got to it.  
That still seems like the correct approach.


It might be simpler to just do 1 pass over the args and push the clone 
callback on the first non-clone parameter.  Right now however there's 
no way to tell what is clone-related and what is not.


I think the attached patch against HEAD DTRT.  Should work on RELENG_7 too.

   Sam

Index: ifconfig.c
===
RCS file: /usr/ncvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.135
diff -u -r1.135 ifconfig.c
--- ifconfig.c  10 Dec 2007 02:31:00 -  1.135
+++ ifconfig.c  30 Mar 2008 19:29:39 -
@@ -93,7 +93,8 @@
 intsupmedia = 0;
 intprintkeys = 0;  /* Print keying material for interfaces. */
 
-static int ifconfig(int argc, char *const *argv, const struct afswtch *afp);
+static int ifconfig(int argc, char *const *argv, int iscreate,
+   const struct afswtch *afp);
 static void status(const struct afswtch *afp, const struct sockaddr_dl *sdl,
struct ifaddrs *ifa);
 static void tunnel_status(int s);
@@ -247,7 +248,7 @@
if (iflen = sizeof(name))
errx(1, %s: cloning name too long,
ifname);
-   ifconfig(argc, argv, NULL);
+   ifconfig(argc, argv, 1, NULL);
exit(0);
}
errx(1, interface %s does not exist, ifname);
@@ -305,7 +306,7 @@
}
 
if (argc  0)
-   ifconfig(argc, argv, afp);
+   ifconfig(argc, argv, 0, afp);
else
status(afp, sdl, ifa);
}
@@ -433,17 +434,19 @@
 

Re: 7.0 - ifconfig create is not working as expected?

2008-03-30 Thread Sam Leffler

Sam Leffler wrote:
I think the attached patch against HEAD DTRT.  Should work on RELENG_7 
too.


Never mind; this has some issues.  I'll post another patch after I do 
some real testing.


   Sam

___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: 7.0 - ifconfig create is not working as expected? [patch 2]

2008-03-30 Thread Sam Leffler

Sam Leffler wrote:

Sam Leffler wrote:
I think the attached patch against HEAD DTRT.  Should work on 
RELENG_7 too.


Never mind; this has some issues.  I'll post another patch after I do 
some real testing.




Try this.  It does as a I suggested--on seeing the first cmd line arg 
that is not marked as a cloning parameter the callback is done and state 
is updated.  Still too hackish for my taste but to handle this and other 
similar stuff properly requires a significant overhaul (there have 
been too many hands in the code doing just enough to get their feature 
working).


   Sam

Index: ifclone.c
===
RCS file: /usr/ncvs/src/sbin/ifconfig/ifclone.c,v
retrieving revision 1.3
diff -u -r1.3 ifclone.c
--- ifclone.c   12 Aug 2006 18:07:17 -  1.3
+++ ifclone.c   30 Mar 2008 19:58:35 -
@@ -143,9 +143,9 @@
 }
 
 static struct cmd clone_cmds[] = {
-   DEF_CMD(create,   0,  clone_create),
+   DEF_CLONE_CMD(create, 0,  clone_create),
DEF_CMD(destroy,  0,  clone_destroy),
-   DEF_CMD(plumb,0,  clone_create),
+   DEF_CLONE_CMD(plumb,  0,  clone_create),
DEF_CMD(unplumb,  0,  clone_destroy),
 };
 
Index: ifconfig.c
===
RCS file: /usr/ncvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.135
diff -u -r1.135 ifconfig.c
--- ifconfig.c  10 Dec 2007 02:31:00 -  1.135
+++ ifconfig.c  30 Mar 2008 19:43:34 -
@@ -93,7 +93,8 @@
 intsupmedia = 0;
 intprintkeys = 0;  /* Print keying material for interfaces. */
 
-static int ifconfig(int argc, char *const *argv, const struct afswtch *afp);
+static int ifconfig(int argc, char *const *argv, int iscreate,
+   const struct afswtch *afp);
 static void status(const struct afswtch *afp, const struct sockaddr_dl *sdl,
struct ifaddrs *ifa);
 static void tunnel_status(int s);
@@ -247,7 +248,7 @@
if (iflen = sizeof(name))
errx(1, %s: cloning name too long,
ifname);
-   ifconfig(argc, argv, NULL);
+   ifconfig(argc, argv, 1, NULL);
exit(0);
}
errx(1, interface %s does not exist, ifname);
@@ -305,7 +306,7 @@
}
 
if (argc  0)
-   ifconfig(argc, argv, afp);
+   ifconfig(argc, argv, 0, afp);
else
status(afp, sdl, ifa);
}
@@ -433,17 +434,19 @@
DEF_CMD(ifdstaddr, 0, setifdstaddr);
 
 static int
-ifconfig(int argc, char *const *argv, const struct afswtch *afp)
+ifconfig(int argc, char *const *argv, int iscreate, const struct afswtch *afp)
 {
+   const struct afswtch *nafp;
struct callback *cb;
int s;
 
+   strncpy(ifr.ifr_name, name, sizeof ifr.ifr_name);
+top:
if (afp == NULL)
afp = af_getbyname(inet);
ifr.ifr_addr.sa_family =
afp-af_af == AF_LINK || afp-af_af == AF_UNSPEC ?
AF_INET : afp-af_af;
-   strncpy(ifr.ifr_name, name, sizeof ifr.ifr_name);
 
if ((s = socket(ifr.ifr_addr.sa_family, SOCK_DGRAM, 0))  0)
err(1, socket(family %u,SOCK_DGRAM, ifr.ifr_addr.sa_family);
@@ -460,6 +463,33 @@
p = (setaddr ? setifdstaddr_cmd : setifaddr_cmd);
}
if (p-c_u.c_func || p-c_u.c_func2) {
+   if (iscreate  !p-c_iscloneop) { 
+   /*
+* Push the clone create callback so the new
+* device is created and can be used for any
+* remaining arguments.
+*/
+   cb = callbacks;
+   if (cb == NULL)
+   errx(1, internal error, no callback);
+   callbacks = cb-cb_next;
+   cb-cb_func(s, cb-cb_arg);
+   iscreate = 0;
+   /*
+* Handle any address family spec that
+* immediately follows and potentially
+* recreate the socket.
+*/
+   nafp = af_getbyname(*argv);
+   if (nafp != NULL) {
+   argc--, argv++;
+   if (nafp != afp) {
+   close(s);
+   afp = nafp;
+