Re: 7.0 - ifconfig create is not working as expected?
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
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
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
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
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?
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
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?
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?
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
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?
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?
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?
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
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?
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?
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?
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?
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?
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]
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;
+
