Re: [PATCH 2.6.12-rc2] aoe [1/6]: improve allowed interfaces configuration

Re: [PATCH 2.6.12-rc2] aoe [1/6]: improve allowed interfaces configuration

[Bodo Eggert <harvested.in.lkml@posting.7eggert.dyndns.org>]

Ed L Cashin <ecashin@coraid.com> wrote:

> +++ b/Documentation/aoe/aoe.txt       2005-04-20 11:42:20.000000000 -0400

> +  When the aoe driver is a module, use

Is there any reason for this inconsistent behaviour?

> +  /sys/module/aoe/parameters/aoe_iflist instead of
                                ^^^

Why does the module name need to be part of the attribute?
That's redundant. That's redundant.

> +  There is a boot option for the built-in aoe driver and a
> +  corresponding module parameter, aoe_iflist.  Without this option,
> +  all network interfaces may be used for ATA over Ethernet.  Here is a
> +  usage example for the module parameter.
> +
> +    modprobe aoe_iflist="eth1 eth3"
               ^
             "aoe"
-- 
Top 100 things you don't want the sysadmin to say:
63. Oracle will be down until 8pm, but you can come back in and finish your
    work when it comes up tonight.

Re: [PATCH 2.6.12-rc2] aoe [1/6]: improve allowed interfaces configuration

[Ed L Cashin]

"Bodo Eggert <harvested.in.lkml@posting.7eggert.dyndns.org>" <7eggert@gmx.de> writes:

> Ed L Cashin <ecashin@coraid.com> wrote:
>
>> +++ b/Documentation/aoe/aoe.txt       2005-04-20 11:42:20.000000000 -0400
>
>> +  When the aoe driver is a module, use
>
> Is there any reason for this inconsistent behaviour?

Yes, the /sys/module/aoe area is only present when the aoe driver is a
module.  It would be nicer if there were a sysfs area where I could
put this file regardless of whether the driver is a module or built
into the kernel.  

I could probably create one, but I got the file in
/sys/module/aoe/parameters for free when I used module_param_string.

>> +  /sys/module/aoe/parameters/aoe_iflist instead of
>                                 ^^^
>
> Why does the module name need to be part of the attribute?
> That's redundant. That's redundant.

Yes.  That's true.  Redundancy isn't always bad, though, and using the
"aoe_" prefix lets the kernel parameter for the built-in aoe driver be
the same as the parameter for the modular driver.

-- 
  Ed L Cashin <ecashin@coraid.com>

Re: [PATCH 2.6.12-rc2] aoe [1/6]: improve allowed interfaces configuration

[Greg KH]

On Thu, Apr 21, 2005 at 09:36:17AM -0400, Ed L Cashin wrote:
> "Bodo Eggert <harvested.in.lkml@posting.7eggert.dyndns.org>" <7eggert@gmx.de> writes:
> 
> > Ed L Cashin <ecashin@coraid.com> wrote:
> >
> >> +++ b/Documentation/aoe/aoe.txt       2005-04-20 11:42:20.000000000 -0400
> >
> >> +  When the aoe driver is a module, use
> >
> > Is there any reason for this inconsistent behaviour?
> 
> Yes, the /sys/module/aoe area is only present when the aoe driver is a
> module.

Not true, have you looked in /sys/module lately?  :)

> It would be nicer if there were a sysfs area where I could
> put this file regardless of whether the driver is a module or built
> into the kernel.  

That's the place for it.  It will be there if the driver is built as a
module or into the kernel.

thanks,

greg k-h

Re: [PATCH 2.6.12-rc2] aoe [1/6]: improve allowed interfaces configuration

[Ed L Cashin]

[-- Attachment #1: Type: text/plain, Size: 1202 bytes --]

Greg KH <greg@kroah.com> writes:

> On Thu, Apr 21, 2005 at 09:36:17AM -0400, Ed L Cashin wrote:
>> "Bodo Eggert <harvested.in.lkml@posting.7eggert.dyndns.org>" <7eggert@gmx.de> writes:
>> 
>> > Ed L Cashin <ecashin@coraid.com> wrote:
>> >
>> >> +++ b/Documentation/aoe/aoe.txt       2005-04-20 11:42:20.000000000 -0400
>> >
>> >> +  When the aoe driver is a module, use
>> >
>> > Is there any reason for this inconsistent behaviour?
>> 
>> Yes, the /sys/module/aoe area is only present when the aoe driver is a
>> module.
>
> Not true, have you looked in /sys/module lately?  :)
>
>> It would be nicer if there were a sysfs area where I could
>> put this file regardless of whether the driver is a module or built
>> into the kernel.  
>
> That's the place for it.  It will be there if the driver is built as a
> module or into the kernel.

Wow!  Well, that's very convenient for driver writers, so I'm pleased,
and I can update the docs.  It surprises me, though, to find out that
/sys/module is for things other than modules.

The correction below follows and depends on patch 1 of the six.


fix docs: built-in driver can use files in /sys/module

Signed-off-by: Ed L. Cashin <ecashin@coraid.com>


[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 1128 bytes --]

diff -urNp a-exp/linux/Documentation/aoe/aoe.txt b-exp/linux/Documentation/aoe/aoe.txt
--- a-exp/linux/Documentation/aoe/aoe.txt	2005-04-21 11:25:48.000000000 -0400
+++ b-exp/linux/Documentation/aoe/aoe.txt	2005-04-21 11:25:49.000000000 -0400
@@ -102,12 +102,11 @@ USING SYSFS
       e4.8            eth1              up
       e4.9            eth1              up
 
-  When the aoe driver is a module, use
-  /sys/module/aoe/parameters/aoe_iflist instead of
-  /dev/etherd/interfaces to limit AoE traffic to the network
-  interfaces in the given whitespace-separated list.  Unlike the old
-  character device, the sysfs entry can be read from as well as
-  written to.
+  Use /sys/module/aoe/parameters/aoe_iflist (or better, the driver
+  option discussed below) instead of /dev/etherd/interfaces to limit
+  AoE traffic to the network interfaces in the given
+  whitespace-separated list.  Unlike the old character device, the
+  sysfs entry can be read from as well as written to.
 
   It's helpful to trigger discovery after setting the list of allowed
   interfaces.  The aoetools package provides an aoe-discover script

[-- Attachment #3: Type: text/plain, Size: 41 bytes --]



-- 
  Ed L Cashin <ecashin@coraid.com>

Re: [PATCH 2.6.12-rc2] aoe [1/6]: improve allowed interfaces configuration

[Greg KH]

On Thu, Apr 21, 2005 at 11:30:06AM -0400, Ed L Cashin wrote:
> Greg KH <greg@kroah.com> writes:
> 
> > On Thu, Apr 21, 2005 at 09:36:17AM -0400, Ed L Cashin wrote:
> >> "Bodo Eggert <harvested.in.lkml@posting.7eggert.dyndns.org>" <7eggert@gmx.de> writes:
> >> 
> >> > Ed L Cashin <ecashin@coraid.com> wrote:
> >> >
> >> >> +++ b/Documentation/aoe/aoe.txt       2005-04-20 11:42:20.000000000 -0400
> >> >
> >> >> +  When the aoe driver is a module, use
> >> >
> >> > Is there any reason for this inconsistent behaviour?
> >> 
> >> Yes, the /sys/module/aoe area is only present when the aoe driver is a
> >> module.
> >
> > Not true, have you looked in /sys/module lately?  :)
> >
> >> It would be nicer if there were a sysfs area where I could
> >> put this file regardless of whether the driver is a module or built
> >> into the kernel.  
> >
> > That's the place for it.  It will be there if the driver is built as a
> > module or into the kernel.
> 
> Wow!  Well, that's very convenient for driver writers, so I'm pleased,
> and I can update the docs.  It surprises me, though, to find out that
> /sys/module is for things other than modules.

It's not for things other than modules, it's filling a real need that
you yourself just pointed out.  Namely, we need to be able to have
access to module paramaters in a consistant place, no matter if the
driver is built into the kernel or not.

Man, you try to be nice to people...  :)

thanks,

greg k-h

/sys/module (was Re: [PATCH 2.6.12-rc2] aoe [1/6]: improve allowed interfaces configuration)

[Ed L Cashin]

Greg KH <greg@kroah.com> writes:

...
> It's not for things other than modules, it's filling a real need that
> you yourself just pointed out.  Namely, we need to be able to have
> access to module paramaters in a consistant place, no matter if the
> driver is built into the kernel or not.
>
> Man, you try to be nice to people...  :)

It wasn't a complaint --- like I said, I'm pleased!  I just wanted to
serve as a datum: one guy was surprised.

I wanted to put the interfaces list configuration into sysfs, but I
didn't really know how or where to put it, so I procrastinated.  Then
I created the module parameter and was pleased to see it show up in
sysfs.  I had read about module parameters before, but I had forgotten
about that feature, or maybe it's newer than the docs I read.

Thanks for working so hard to make sysfs useful.

-- 
  Ed L Cashin <ecashin@coraid.com>

Re: [PATCH 2.6.12-rc2] aoe [1/6]: improve allowed interfaces configuration

[Randy.Dunlap]

On Thu, 21 Apr 2005 11:30:06 -0400 Ed L Cashin wrote:

| Greg KH <greg@kroah.com> writes:
| 
| > On Thu, Apr 21, 2005 at 09:36:17AM -0400, Ed L Cashin wrote:
| >> "Bodo Eggert <harvested.in.lkml@posting.7eggert.dyndns.org>" <7eggert@gmx.de> writes:
| >> 
| >> > Ed L Cashin <ecashin@coraid.com> wrote:
| >> >
| >> >> +++ b/Documentation/aoe/aoe.txt       2005-04-20 11:42:20.000000000 -0400
| >> >
| >> >> +  When the aoe driver is a module, use
| >> >
| >> > Is there any reason for this inconsistent behaviour?
| >> 
| >> Yes, the /sys/module/aoe area is only present when the aoe driver is a
| >> module.
| >
| > Not true, have you looked in /sys/module lately?  :)
| >
| >> It would be nicer if there were a sysfs area where I could
| >> put this file regardless of whether the driver is a module or built
| >> into the kernel.  
| >
| > That's the place for it.  It will be there if the driver is built as a
| > module or into the kernel.
| 
| Wow!  Well, that's very convenient for driver writers, so I'm pleased,
| and I can update the docs.  It surprises me, though, to find out that
| /sys/module is for things other than modules.

Just depends on your definition of a module.

AOE (or just about any device driver) can be considered logically
as a module.  You seem to be equating module with "loadable module"
vs. a builtin module.  The good news is that /sys/module works
for loadable or builtin modules.

---
~Randy

Re: [PATCH 2.6.12-rc2] aoe [1/6]: improve allowed interfaces configuration

[Domen Puncer]

On 21/04/05 09:36 -0400, Ed L Cashin wrote:
> "Bodo Eggert <harvested.in.lkml@posting.7eggert.dyndns.org>" <7eggert@gmx.de> writes:
> 
> > Ed L Cashin <ecashin@coraid.com> wrote:
> >
...
> >> +  /sys/module/aoe/parameters/aoe_iflist instead of
> >                                 ^^^
> >
> > Why does the module name need to be part of the attribute?
> > That's redundant. That's redundant.
> 
> Yes.  That's true.  Redundancy isn't always bad, though, and using the
> "aoe_" prefix lets the kernel parameter for the built-in aoe driver be
> the same as the parameter for the modular driver.

The __setup() stuff is redundancy too, as module parameters already
work as boot parameters (ie. aoe.iflist).


	Domen

[PATCH 2.6.12-rc2] aoe [1/6]: improve allowed interfaces configuration

[Ed L Cashin]

improve allowed interfaces configuration

Signed-off-by: Ed L. Cashin <ecashin@coraid.com>

diff -uprN a/Documentation/aoe/aoe.txt b/Documentation/aoe/aoe.txt
--- a/Documentation/aoe/aoe.txt	2005-04-20 11:40:55.000000000 -0400
+++ b/Documentation/aoe/aoe.txt	2005-04-20 11:42:20.000000000 -0400
@@ -33,6 +33,9 @@ USING DEVICE NODES
   "cat /dev/etherd/err" blocks, waiting for error diagnostic output,
   like any retransmitted packets.
 
+  The /dev/etherd/interfaces special file is obsoleted by the
+  aoe_iflist boot option and module option (and its sysfs entry
+  described in the next section).
   "echo eth2 eth4 > /dev/etherd/interfaces" tells the aoe driver to
   limit ATA over Ethernet traffic to eth2 and eth4.  AoE traffic from
   untrusted networks should be ignored as a matter of security.
@@ -89,3 +92,24 @@ USING SYSFS
       e4.7            eth1              up
       e4.8            eth1              up
       e4.9            eth1              up
+
+  When the aoe driver is a module, use
+  /sys/module/aoe/parameters/aoe_iflist instead of
+  /dev/etherd/interfaces to limit AoE traffic to the network
+  interfaces in the given whitespace-separated list.  Unlike the old
+  character device, the sysfs entry can be read from as well as
+  written to.
+
+  It's helpful to trigger discovery after setting the list of allowed
+  interfaces.  If your distro provides an aoe-discover script, you can
+  use that.  Otherwise, you can directly use the /dev/etherd/discover
+  file described above.
+
+DRIVER OPTIONS
+
+  There is a boot option for the built-in aoe driver and a
+  corresponding module parameter, aoe_iflist.  Without this option,
+  all network interfaces may be used for ATA over Ethernet.  Here is a
+  usage example for the module parameter.
+
+    modprobe aoe_iflist="eth1 eth3"
diff -uprN a/drivers/block/aoe/aoenet.c b/drivers/block/aoe/aoenet.c
--- a/drivers/block/aoe/aoenet.c	2005-04-20 11:41:18.000000000 -0400
+++ b/drivers/block/aoe/aoenet.c	2005-04-20 11:42:20.000000000 -0400
@@ -7,6 +7,7 @@
 #include <linux/hdreg.h>
 #include <linux/blkdev.h>
 #include <linux/netdevice.h>
+#include <linux/moduleparam.h>
 #include "aoe.h"
 
 #define NECODES 5
@@ -26,6 +27,19 @@ enum {
 };
 
 static char aoe_iflist[IFLISTSZ];
+module_param_string(aoe_iflist, aoe_iflist, IFLISTSZ, 0600);
+MODULE_PARM_DESC(aoe_iflist, " aoe_iflist=\"dev1 [dev2 ...]\n");
+
+#ifndef MODULE
+static int __init aoe_iflist_setup(char *str)
+{
+	strncpy(aoe_iflist, str, IFLISTSZ);
+	aoe_iflist[IFLISTSZ - 1] = '\0';
+	return 1;
+}
+
+__setup("aoe_iflist=", aoe_iflist_setup);
+#endif
 
 int
 is_aoe_netif(struct net_device *ifp)
@@ -36,7 +50,8 @@ is_aoe_netif(struct net_device *ifp)
 	if (aoe_iflist[0] == '\0')
 		return 1;
 
-	for (p = aoe_iflist; *p; p = q + strspn(q, WHITESPACE)) {
+	p = aoe_iflist + strspn(aoe_iflist, WHITESPACE);
+	for (; *p; p = q + strspn(q, WHITESPACE)) {
 		q = p + strcspn(p, WHITESPACE);
 		if (q != p)
 			len = q - p;


-- 
  Ed L. Cashin <ecashin@coraid.com>

Re: [PATCH 2.6.12-rc2] aoe [1/6]: improve allowed interfaces configuration

[Randy.Dunlap]

On Wed, 20 Apr 2005 13:02:12 -0400 Ed L Cashin wrote:

Just a nit/typo:

| +    modprobe aoe_iflist="eth1 eth3"

|  static char aoe_iflist[IFLISTSZ];
| +module_param_string(aoe_iflist, aoe_iflist, IFLISTSZ, 0600);
| +MODULE_PARM_DESC(aoe_iflist, " aoe_iflist=\"dev1 [dev2 ...]\n");

No leading space (" aoe_iflist=") and put a trailing \" in it:

  +MODULE_PARM_DESC(aoe_iflist, "aoe_iflist=\"dev1 [dev2 ...]\"\n");


---
~Randy

Re: [PATCH 2.6.12-rc2] aoe [1/6]: improve allowed interfaces configuration

[Ed L Cashin]

[-- Attachment #1: Type: text/plain, Size: 602 bytes --]

"Randy.Dunlap" <rddunlap@osdl.org> writes:

> On Wed, 20 Apr 2005 13:02:12 -0400 Ed L Cashin wrote:
>
> Just a nit/typo:
>
> | +    modprobe aoe_iflist="eth1 eth3"
>
> |  static char aoe_iflist[IFLISTSZ];
> | +module_param_string(aoe_iflist, aoe_iflist, IFLISTSZ, 0600);
> | +MODULE_PARM_DESC(aoe_iflist, " aoe_iflist=\"dev1 [dev2 ...]\n");
>
> No leading space (" aoe_iflist=") and put a trailing \" in it:
>
>   +MODULE_PARM_DESC(aoe_iflist, "aoe_iflist=\"dev1 [dev2 ...]\"\n");

Thanks for catching that.


improve allowed interfaces configuration

Signed-off-by: Ed L. Cashin <ecashin@coraid.com>


[-- Attachment #2: patch-124.rediff --]
[-- Type: text/plain, Size: 2878 bytes --]

diff -uprN a/Documentation/aoe/aoe.txt b/Documentation/aoe/aoe.txt
--- a/Documentation/aoe/aoe.txt	2005-04-20 11:40:55.000000000 -0400
+++ b/Documentation/aoe/aoe.txt	2005-04-20 11:42:20.000000000 -0400
@@ -33,6 +33,9 @@ USING DEVICE NODES
   "cat /dev/etherd/err" blocks, waiting for error diagnostic output,
   like any retransmitted packets.
 
+  The /dev/etherd/interfaces special file is obsoleted by the
+  aoe_iflist boot option and module option (and its sysfs entry
+  described in the next section).
   "echo eth2 eth4 > /dev/etherd/interfaces" tells the aoe driver to
   limit ATA over Ethernet traffic to eth2 and eth4.  AoE traffic from
   untrusted networks should be ignored as a matter of security.
@@ -89,3 +92,24 @@ USING SYSFS
       e4.7            eth1              up
       e4.8            eth1              up
       e4.9            eth1              up
+
+  When the aoe driver is a module, use
+  /sys/module/aoe/parameters/aoe_iflist instead of
+  /dev/etherd/interfaces to limit AoE traffic to the network
+  interfaces in the given whitespace-separated list.  Unlike the old
+  character device, the sysfs entry can be read from as well as
+  written to.
+
+  It's helpful to trigger discovery after setting the list of allowed
+  interfaces.  If your distro provides an aoe-discover script, you can
+  use that.  Otherwise, you can directly use the /dev/etherd/discover
+  file described above.
+
+DRIVER OPTIONS
+
+  There is a boot option for the built-in aoe driver and a
+  corresponding module parameter, aoe_iflist.  Without this option,
+  all network interfaces may be used for ATA over Ethernet.  Here is a
+  usage example for the module parameter.
+
+    modprobe aoe_iflist="eth1 eth3"
diff -uprN a/drivers/block/aoe/aoenet.c b/drivers/block/aoe/aoenet.c
--- a/drivers/block/aoe/aoenet.c	2005-04-20 11:41:18.000000000 -0400
+++ b/drivers/block/aoe/aoenet.c	2005-04-20 11:42:20.000000000 -0400
@@ -7,6 +7,7 @@
 #include <linux/hdreg.h>
 #include <linux/blkdev.h>
 #include <linux/netdevice.h>
+#include <linux/moduleparam.h>
 #include "aoe.h"
 
 #define NECODES 5
@@ -26,6 +27,19 @@ enum {
 };
 
 static char aoe_iflist[IFLISTSZ];
+module_param_string(aoe_iflist, aoe_iflist, IFLISTSZ, 0600);
+MODULE_PARM_DESC(aoe_iflist, "aoe_iflist=\"dev1 [dev2 ...]\"\n");
+
+#ifndef MODULE
+static int __init aoe_iflist_setup(char *str)
+{
+	strncpy(aoe_iflist, str, IFLISTSZ);
+	aoe_iflist[IFLISTSZ - 1] = '\0';
+	return 1;
+}
+
+__setup("aoe_iflist=", aoe_iflist_setup);
+#endif
 
 int
 is_aoe_netif(struct net_device *ifp)
@@ -36,7 +50,8 @@ is_aoe_netif(struct net_device *ifp)
 	if (aoe_iflist[0] == '\0')
 		return 1;
 
-	for (p = aoe_iflist; *p; p = q + strspn(q, WHITESPACE)) {
+	p = aoe_iflist + strspn(aoe_iflist, WHITESPACE);
+	for (; *p; p = q + strspn(q, WHITESPACE)) {
 		q = p + strcspn(p, WHITESPACE);
 		if (q != p)
 			len = q - p;

[-- Attachment #3: Type: text/plain, Size: 41 bytes --]



-- 
  Ed L Cashin <ecashin@coraid.com>