Re: [PATCH v2 -mm 1/9] netconsole: Cleanups, codingstyle, prettyfication

Re: [PATCH v2 -mm 1/9] netconsole: Cleanups, codingstyle, prettyfication

[Matt Mackall]

On Tue, Jul 10, 2007 at 02:49:25PM +0530, Satyam Sharma wrote:
> From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> 
> [1/9] netconsole: Cleanups, codingstyle, prettyfication
> 
> (1) Remove unwanted headers.
> (2) Mark __init and __exit as appropriate.
> (3) Various trivial codingstyle and prettification stuff.

I don't much like what you've done in (3) because I personally think
tabs should never appear anywhere but at the beginning of a line.
Which is why we consistently ask for people not to mix codingstyle
bits with substantive changes.

But what the hell..

Acked-by: Matt Mackall <mpm@selenic.com>

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH v2 -mm 2/9] netconsole: Remove bogus check

[Matt Mackall]

On Tue, Jul 10, 2007 at 02:49:30PM +0530, Satyam Sharma wrote:
> From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> 
> [2/9] netconsole: Remove bogus check
> 
> The (!np.dev) check in write_msg() is bogus (always false), because:
> np.dev is set by netpoll_setup(), which is called by the target init
> code in init_netconsole() _before_ register_console() => write_msg() cannot
> be triggered unless netpoll_setup() returns with success. And that will not
> happen if netpoll_setup() failed to set np.dev. Also np.dev cannot go from
> under us while netconsole is loaded. This is because netpoll_setup() grabs
> a reference for us on that dev. So let's remove the pointless check.
> 
> Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>

Acked-by: Matt Mackall <mpm@selenic.com>

-- 
Mathematics is the supreme nostalgia of our time.

[RFC][PATCH v2 -mm 0/9] netconsole: Multiple targets and dynamic reconfigurability

[Satyam Sharma]

[0/9] netconsole: Multiple targets and dynamic reconfigurability

This patchset is a rework of the original idea and patches posted by
Keiichi Kii and Takayoshi Kochi at: http://lkml.org/lkml/2007/6/13/72

This is v2 of the patchset, the previous version is available at:
http://lkml.org/lkml/2007/7/4/107

This is diffed against 2.6.22-rc6-mm1 + the earlier netpoll fixlet and the
configfs cleanup patches (those are already merged in -mm AFAIK, and hence
not reproduced here).

[1/9] netconsole: Cleanups, codingstyle, prettyfication
[2/9] netconsole: Remove bogus check
[3/9] netconsole: Simplify boot/module option setup logic
[4/9] netconsole: Add some useful tips to documentation
[5/9] netconsole: Introduce netconsole_target
[6/9] netconsole: Introduce netconsole_netdev_notifier
[7/9] netconsole: Use netif_running() in write_msg()
[8/9] netconsole: Support multiple logging targets
[9/9] netconsole: Support dynamic reconfiguration using configfs

Changes since v1:
=================

* Split / merge various patches in the patchset as suggested in reviews.

* Get rid of redundant "id" and "dev_status" fields.

* Use netif_running() instead of IFF_UP.

* Explain the boot/module option setup simplification and the netdev
  notifier patches in more detail (justify why they're good / required).

* Get rid of possible trailing newline from echo(1) in store_dev_name().

* Fix a panic bug -- now, when no boot/module option is specified, we
  skip only the part that creates and inserts param string-created
  targets. Rest of the initialization (notifier, configfs subsystem)
  is done as usual.

* Get rid of lock-skipping special-casing where possible.

* Use config_item_name() instead of "empty_item".

* Introduce netpoll_print_options() in netpoll and use it.

* Style cleanups (reduce #ifdefs, indentation, whitespace fixes, etc).

* Miscellaneous fixes in documentation (as suggested in reviews).

About this patchset:
====================

What?

Support multiple remote logging targets in netconsole. Also, ability to
dynamically add or remove targets or modify parameters (IP, port, remote
MAC address) of targets at runtime, from userspace, using configfs.

Why?

>From Keiichi Kii's original post:

[...] current netconsole is not flexible. For example, if you want to change
ip address for logging agent, in the case of built-in netconsole, you can't
change config except for changing boot parameter and rebooting your system,
or in the case of module netconsole, you need to remove it and reload
with different parameters. [...] and we have been losing serial console
port with PCs and Servers.

(... and especially laptops, I would add.)

Kindly review and comment.

Satyam

[PATCH v2 -mm 1/9] netconsole: Cleanups, codingstyle, prettyfication

[Satyam Sharma]

From: Satyam Sharma <ssatyam@cse.iitk.ac.in>

[1/9] netconsole: Cleanups, codingstyle, prettyfication

(1) Remove unwanted headers.
(2) Mark __init and __exit as appropriate.
(3) Various trivial codingstyle and prettification stuff.

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>

---

 drivers/net/netconsole.c |   55 +++++++++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 28 deletions(-)

---

diff -ruNp a/drivers/net/netconsole.c b/drivers/net/netconsole.c
--- a/drivers/net/netconsole.c	2007-04-26 08:38:32.000000000 +0530
+++ b/drivers/net/netconsole.c	2007-07-03 20:27:17.000000000 +0530
@@ -35,35 +35,32 @@
  ****************************************************************/
 
 #include <linux/mm.h>
-#include <linux/tty.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/console.h>
-#include <linux/tty_driver.h>
 #include <linux/moduleparam.h>
 #include <linux/string.h>
-#include <linux/sysrq.h>
-#include <linux/smp.h>
 #include <linux/netpoll.h>
 
 MODULE_AUTHOR("Maintainer: Matt Mackall <mpm@selenic.com>");
 MODULE_DESCRIPTION("Console driver for network interfaces");
 MODULE_LICENSE("GPL");
 
-static char config[256];
-module_param_string(netconsole, config, 256, 0);
+#define MAX_PARAM_LENGTH	256
+#define MAX_PRINT_CHUNK		1000
+
+static char config[MAX_PARAM_LENGTH];
+module_param_string(netconsole, config, MAX_PARAM_LENGTH, 0);
 MODULE_PARM_DESC(netconsole, " netconsole=[src-port]@[src-ip]/[dev],[tgt-port]@<tgt-ip>/[tgt-macaddr]\n");
 
 static struct netpoll np = {
-	.name = "netconsole",
-	.dev_name = "eth0",
-	.local_port = 6665,
-	.remote_port = 6666,
-	.remote_mac = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff},
+	.name		= "netconsole",
+	.dev_name	= "eth0",
+	.local_port	= 6665,
+	.remote_port	= 6666,
+	.remote_mac	= {0xff, 0xff, 0xff, 0xff, 0xff, 0xff},
 };
-static int configured = 0;
-
-#define MAX_PRINT_CHUNK 1000
+static int configured;
 
 static void write_msg(struct console *con, const char *msg, unsigned int len)
 {
@@ -75,7 +72,7 @@ static void write_msg(struct console *co
 
 	local_irq_save(flags);
 
-	for(left = len; left; ) {
+	for (left = len; left;) {
 		frag = min(left, MAX_PRINT_CHUNK);
 		netpoll_send_udp(&np, msg, frag);
 		msg += frag;
@@ -86,12 +83,12 @@ static void write_msg(struct console *co
 }
 
 static struct console netconsole = {
-	.name = "netcon",
-	.flags = CON_ENABLED | CON_PRINTBUFFER,
-	.write = write_msg
+	.name	= "netcon",
+	.flags	= CON_ENABLED | CON_PRINTBUFFER,
+	.write	= write_msg,
 };
 
-static int option_setup(char *opt)
+static int __init option_setup(char *opt)
 {
 	configured = !netpoll_parse_options(&np, opt);
 	return 1;
@@ -99,28 +96,30 @@ static int option_setup(char *opt)
 
 __setup("netconsole=", option_setup);
 
-static int init_netconsole(void)
+static int __init init_netconsole(void)
 {
-	int err;
+	int err = 0;
 
-	if(strlen(config))
+	if (strnlen(config, MAX_PARAM_LENGTH))
 		option_setup(config);
 
-	if(!configured) {
-		printk("netconsole: not configured, aborting\n");
-		return 0;
+	if (!configured) {
+		printk(KERN_INFO "netconsole: not configured, aborting\n");
+		goto out;
 	}
 
 	err = netpoll_setup(&np);
 	if (err)
-		return err;
+		goto out;
 
 	register_console(&netconsole);
 	printk(KERN_INFO "netconsole: network logging started\n");
-	return 0;
+
+out:
+	return err;
 }
 
-static void cleanup_netconsole(void)
+static void __exit cleanup_netconsole(void)
 {
 	unregister_console(&netconsole);
 	netpoll_cleanup(&np);

[PATCH v2 -mm 2/9] netconsole: Remove bogus check

[Satyam Sharma]

From: Satyam Sharma <ssatyam@cse.iitk.ac.in>

[2/9] netconsole: Remove bogus check

The (!np.dev) check in write_msg() is bogus (always false), because:
np.dev is set by netpoll_setup(), which is called by the target init
code in init_netconsole() _before_ register_console() => write_msg() cannot
be triggered unless netpoll_setup() returns with success. And that will not
happen if netpoll_setup() failed to set np.dev. Also np.dev cannot go from
under us while netconsole is loaded. This is because netpoll_setup() grabs
a reference for us on that dev. So let's remove the pointless check.

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>

---

 drivers/net/netconsole.c |    3 ---
 1 file changed, 3 deletions(-)

---

diff -ruNp a/drivers/net/netconsole.c b/drivers/net/netconsole.c
--- a/drivers/net/netconsole.c	2007-07-10 05:53:42.000000000 +0530
+++ b/drivers/net/netconsole.c	2007-07-10 05:55:00.000000000 +0530
@@ -67,9 +67,6 @@ static void write_msg(struct console *co
 	int frag, left;
 	unsigned long flags;
 
-	if (!np.dev)
-		return;
-
 	local_irq_save(flags);
 
 	for (left = len; left;) {

[PATCH v2 -mm 3/9] netconsole: Simplify boot/module option setup logic

[Satyam Sharma]

From: Satyam Sharma <ssatyam@cse.iitk.ac.in>

[3/9] netconsole: Simplify boot/module option setup logic

Presently, for built-in netconsole:

__setup(..., option_setup) ensures that the option_setup() function is
called at boot-time from obsolete_checksetup() with the string matching
"netconsole=" passed to it from the kernel's command line. We call the
netpoll_parse_options() from in there, and populate the netpoll struct
with the passed values. Then, when init_netconsole() is called during the
initcall phase, strlen(config) fails, thus skipping option_setup().

For modular netconsole:

module_param_string() ensures that the string corresponding to the
"netconsole" module parameter passed from the modprobe command line is
copied into the "config" static variable. This time, when the module is
being initialized, strlen(config) is true and so option_setup() is called
on "config" from init_netconsole() and the input string is parsed and the
netpoll struct populated.

Hence, quite different things happen in the copying and parsing of the
passed netpoll parameters for the modular / built-in cases. This patch
makes both of them similar by doing exactly the equivalent of a
module_param_string() in option_setup() also -- just copying the param
string passed from the kernel command line into the "config" static
variable. So, init_netconsole() parses (using netpoll_parse_options())
it in both the cases, and makes the logic somewhat simpler.

Now, option_setup() is only ever called / used for the built-in case,
so we put it inside a #ifndef MODULE, otherwise gcc will complain about
option_setup() being "defined but not used".

Also, the "configured" variable is redundant with this patch and so removed.

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>

---

 drivers/net/netconsole.c |   27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

---

diff -ruNp a/drivers/net/netconsole.c b/drivers/net/netconsole.c
--- a/drivers/net/netconsole.c	2007-07-10 06:00:15.000000000 +0530
+++ b/drivers/net/netconsole.c	2007-07-10 06:28:19.000000000 +0530
@@ -53,6 +53,15 @@ static char config[MAX_PARAM_LENGTH];
 module_param_string(netconsole, config, MAX_PARAM_LENGTH, 0);
 MODULE_PARM_DESC(netconsole, " netconsole=[src-port]@[src-ip]/[dev],[tgt-port]@<tgt-ip>/[tgt-macaddr]\n");
 
+#ifndef	MODULE
+static int __init option_setup(char *opt)
+{
+	strlcpy(config, opt, MAX_PARAM_LENGTH);
+	return 1;
+}
+__setup("netconsole=", option_setup);
+#endif	/* MODULE */
+
 static struct netpoll np = {
 	.name		= "netconsole",
 	.dev_name	= "eth0",
@@ -60,7 +69,6 @@ static struct netpoll np = {
 	.remote_port	= 6666,
 	.remote_mac	= {0xff, 0xff, 0xff, 0xff, 0xff, 0xff},
 };
-static int configured;
 
 static void write_msg(struct console *con, const char *msg, unsigned int len)
 {
@@ -85,26 +93,19 @@ static struct console netconsole = {
 	.write	= write_msg,
 };
 
-static int __init option_setup(char *opt)
-{
-	configured = !netpoll_parse_options(&np, opt);
-	return 1;
-}
-
-__setup("netconsole=", option_setup);
-
 static int __init init_netconsole(void)
 {
 	int err = 0;
 
-	if (strnlen(config, MAX_PARAM_LENGTH))
-		option_setup(config);
-
-	if (!configured) {
+	if (!strnlen(config, MAX_PARAM_LENGTH)) {
 		printk(KERN_INFO "netconsole: not configured, aborting\n");
 		goto out;
 	}
 
+	err = netpoll_parse_options(&np, config);
+	if (err)
+		goto out;
+
 	err = netpoll_setup(&np);
 	if (err)
 		goto out;

[PATCH v2 -mm 4/9] netconsole: Add some useful tips to documentation

[Satyam Sharma]

From: Satyam Sharma <ssatyam@cse.iitk.ac.in>

[4/9] netconsole: Add some useful tips to documentation

Add some useful general-purpose tips.

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>

---

 Documentation/networking/netconsole.txt |   13 +++++++++++++
 1 file changed, 13 insertions(+)

---

diff -ruNp a/Documentation/networking/netconsole.txt b/Documentation/networking/netconsole.txt
--- a/Documentation/networking/netconsole.txt	2007-04-26 08:38:32.000000000 +0530
+++ b/Documentation/networking/netconsole.txt	2007-07-10 07:31:12.000000000 +0530
@@ -44,6 +44,19 @@ WARNING: the default target ethernet set
 ethernet address to send packets, which can cause increased load on
 other systems on the same ethernet segment.
 
+TIP: some LAN switches may be configured to suppress ethernet broadcasts
+so it is advised to explicitly specify the remote agents' MAC addresses
+from the config parameters passed to netconsole.
+
+TIP: to find out the MAC address of, say, 10.0.0.2, you may try using:
+
+ ping -c 1 10.0.0.2 ; /sbin/arp -n | grep 10.0.0.2
+
+TIP: in case the remote logging agent is on a separate LAN subnet than
+the sender, it is suggested to try specifying the MAC address of the
+default gateway (you may use /sbin/route -n to find it out) as the
+remote MAC address instead.
+
 NOTE: the network device (eth1 in the above case) can run any kind
 of other network traffic, netconsole is not intrusive. Netconsole
 might cause slight delays in other traffic if the volume of kernel

[PATCH v2 -mm 5/9] netconsole: Introduce netconsole_target

[Satyam Sharma]

From: Satyam Sharma <ssatyam@cse.iitk.ac.in>

[5/9] netconsole: Introduce netconsole_target

Introduce a wrapper structure over netpoll to represent logging targets
configured in netconsole. This will get extended with other members in
further patches.

The original patchset did this along with (and inside the #ifdef) of
CONFIG_NETCONSOLE_DYNAMIC itself. I decided otherwise, and was able to
drastically cut down on the #ifdef-complexity of final netconsole.c.
Also, struct netconsole_target would be required for multiple targets
support also, and not just dynamic reconfigurability. Previously these
two things were coupled, but I want to de-link that (more on this later).

Note that this patch in itself looks quite redundant / stupid, but it is
purposefully made this way, so further patches are more readable.

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>

---

 drivers/net/netconsole.c |   34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

---

diff -ruNp a/drivers/net/netconsole.c b/drivers/net/netconsole.c
--- a/drivers/net/netconsole.c	2007-07-10 06:42:11.000000000 +0530
+++ b/drivers/net/netconsole.c	2007-07-10 06:50:46.000000000 +0530
@@ -62,24 +62,35 @@ static int __init option_setup(char *opt
 __setup("netconsole=", option_setup);
 #endif	/* MODULE */
 
-static struct netpoll np = {
-	.name		= "netconsole",
-	.dev_name	= "eth0",
-	.local_port	= 6665,
-	.remote_port	= 6666,
-	.remote_mac	= {0xff, 0xff, 0xff, 0xff, 0xff, 0xff},
+/**
+ * struct netconsole_target - Represents a configured netconsole target.
+ * @np:		The netpoll structure for this target.
+ */
+struct netconsole_target {
+	struct netpoll		np;
+};
+
+static struct netconsole_target default_target = {
+	.np		= {
+		.name		= "netconsole",
+		.dev_name	= "eth0",
+		.local_port	= 6665,
+		.remote_port	= 6666,
+		.remote_mac	= {0xff, 0xff, 0xff, 0xff, 0xff, 0xff},
+	},
 };
 
 static void write_msg(struct console *con, const char *msg, unsigned int len)
 {
 	int frag, left;
 	unsigned long flags;
+	struct netconsole_target *nt = &default_target;
 
 	local_irq_save(flags);
 
 	for (left = len; left;) {
 		frag = min(left, MAX_PRINT_CHUNK);
-		netpoll_send_udp(&np, msg, frag);
+		netpoll_send_udp(&nt->np, msg, frag);
 		msg += frag;
 		left -= frag;
 	}
@@ -96,17 +107,18 @@ static struct console netconsole = {
 static int __init init_netconsole(void)
 {
 	int err = 0;
+	struct netconsole_target *nt = &default_target;
 
 	if (!strnlen(config, MAX_PARAM_LENGTH)) {
 		printk(KERN_INFO "netconsole: not configured, aborting\n");
 		goto out;
 	}
 
-	err = netpoll_parse_options(&np, config);
+	err = netpoll_parse_options(&nt->np, config);
 	if (err)
 		goto out;
 
-	err = netpoll_setup(&np);
+	err = netpoll_setup(&nt->np);
 	if (err)
 		goto out;
 
@@ -119,8 +131,10 @@ out:
 
 static void __exit cleanup_netconsole(void)
 {
+	struct netconsole_target *nt = &default_target;
+
 	unregister_console(&netconsole);
-	netpoll_cleanup(&np);
+	netpoll_cleanup(&nt->np);
 }
 
 module_init(init_netconsole);

[PATCH v2 -mm 6/9] netconsole: Introduce netconsole_netdev_notifier

[Satyam Sharma]

From: Satyam Sharma <ssatyam@cse.iitk.ac.in>

[6/9] netconsole: Introduce netconsole_netdev_notifier

To update fields of underlying netpoll structure at runtime on
corresponding NETDEV_CHANGEADDR or NETDEV_CHANGENAME notifications.

ioctl(SIOCSIFHWADDR) {or ioctl(SIOCSIFNAME)} could be used to change the
hardware/MAC address {or name} of the local interface that our netpoll is
attached to. Whenever this happens, netdev notifier chain is called out
with the NETDEV_CHANGEADDR {or NETDEV_CHANGENAME} event message. We respond
to that and update the local_mac {or dev_name} field of the struct netpoll.
This makes sense anyway, but is especially required for dynamic netconsole
because the netpoll structure's internal members become user visible files
when either sysfs or configfs are used. So this helps us to keep up with the
MAC address / name changes and keep the values in struct netpoll uptodate.

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>

---

 drivers/net/netconsole.c |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

---

diff -ruNp a/drivers/net/netconsole.c b/drivers/net/netconsole.c
--- a/drivers/net/netconsole.c	2007-07-10 06:57:44.000000000 +0530
+++ b/drivers/net/netconsole.c	2007-07-10 07:24:27.000000000 +0530
@@ -80,6 +80,33 @@ static struct netconsole_target default_
 	},
 };
 
+/* Handle network interface device notifications */
+static int netconsole_netdev_event(struct notifier_block *this,
+				   unsigned long event,
+				   void *ptr)
+{
+	struct net_device *dev = ptr;
+	struct netconsole_target *nt = &default_target;
+
+	if (nt->np.dev == dev) {
+		switch (event) {
+		case NETDEV_CHANGEADDR:
+			memcpy(nt->np.local_mac, dev->dev_addr, ETH_ALEN);
+			break;
+
+		case NETDEV_CHANGENAME:
+			strlcpy(nt->np.dev_name, dev->name, IFNAMSIZ);
+			break;
+		}
+	}
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block netconsole_netdev_notifier = {
+	.notifier_call  = netconsole_netdev_event,
+};
+
 static void write_msg(struct console *con, const char *msg, unsigned int len)
 {
 	int frag, left;
@@ -122,6 +149,10 @@ static int __init init_netconsole(void)
 	if (err)
 		goto out;
 
+	err = register_netdevice_notifier(&netconsole_netdev_notifier);
+	if (err)
+		goto out;
+
 	register_console(&netconsole);
 	printk(KERN_INFO "netconsole: network logging started\n");
 
@@ -134,6 +165,7 @@ static void __exit cleanup_netconsole(vo
 	struct netconsole_target *nt = &default_target;
 
 	unregister_console(&netconsole);
+	unregister_netdevice_notifier(&netconsole_netdev_notifier);
 	netpoll_cleanup(&nt->np);
 }
 

[PATCH v2 -mm 7/9] netconsole: Use netif_running() in write_msg()

[Satyam Sharma]

From: Satyam Sharma <ssatyam@cse.iitk.ac.in>

[7/9] netconsole: Use netif_running() in write_msg()

Avoid unnecessarily disabling interrupts and calling netpoll_send_udp()
if the corresponding local interface is not up.

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>

---

 drivers/net/netconsole.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

---

diff -ruNp a/drivers/net/netconsole.c b/drivers/net/netconsole.c
--- a/drivers/net/netconsole.c	2007-07-10 07:26:47.000000000 +0530
+++ b/drivers/net/netconsole.c	2007-07-10 07:45:51.000000000 +0530
@@ -113,16 +113,16 @@ static void write_msg(struct console *co
 	unsigned long flags;
 	struct netconsole_target *nt = &default_target;
 
-	local_irq_save(flags);
-
-	for (left = len; left;) {
-		frag = min(left, MAX_PRINT_CHUNK);
-		netpoll_send_udp(&nt->np, msg, frag);
-		msg += frag;
-		left -= frag;
+	if (netif_running(nt->np.dev)) {
+		local_irq_save(flags);
+		for (left = len; left;) {
+			frag = min(left, MAX_PRINT_CHUNK);
+			netpoll_send_udp(&nt->np, msg, frag);
+			msg += frag;
+			left -= frag;
+		}
+		local_irq_restore(flags);
 	}
-
-	local_irq_restore(flags);
 }
 
 static struct console netconsole = {

[PATCH v2 -mm 8/9] netconsole: Support multiple logging targets

[Satyam Sharma]

From: Satyam Sharma <ssatyam@cse.iitk.ac.in>

[8/9] netconsole: Support multiple logging targets

This patch introduces support for multiple targets:

Let's keep this out of CONFIG_NETCONSOLE_DYNAMIC as well -- this is useful
even in the default case and (including the infrastructure introduced in
previous patches) doesn't really add too many bytes to module text. All the
complexity (and size) comes with the dynamic reconfigurability / userspace
interface patch, and so it's plausible users may want to keep this enabled
but that disabled (say to avoid a dep on CONFIG_CONFIGFS_FS too).

Also update documentation to mention the use of ";" separator to specify
multiple logging targets in the boot/module option string.

Brief overview:

We maintain a target_list (and corresponding lock). Get rid of the static
"default_target" and introduce allocation and release functions for our
netconsole_target objects (but keeping sure to preserve previous behaviour
such as default values). During init_netconsole(), ";" is used as the
separator to identify multiple target specifications in the boot/module
option string. The target specifications are parsed and netpolls setup.
During exit, the target_list is torn down and all items released.

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>

---

 Documentation/networking/netconsole.txt |    6 +
 drivers/net/netconsole.c                |  173 ++++++++++++++++++++++++--------
 2 files changed, 139 insertions(+), 40 deletions(-)

---

diff -ruNp a/Documentation/networking/netconsole.txt b/Documentation/networking/netconsole.txt
--- a/Documentation/networking/netconsole.txt	2007-07-10 07:35:17.000000000 +0530
+++ b/Documentation/networking/netconsole.txt	2007-07-10 08:25:17.000000000 +0530
@@ -34,6 +34,12 @@ Examples:
 
  insmod netconsole netconsole=@/,@10.0.0.2/
 
+It also supports logging to multiple remote agents by specifying
+parameters for the multiple agents separated by semicolons and the
+complete string enclosed in "quotes", thusly:
+
+ modprobe netconsole netconsole="@/,@10.0.0.2/;@/eth1,6892@10.0.0.3/"
+
 Built-in netconsole starts immediately after the TCP stack is
 initialized and attempts to bring up the supplied dev at the supplied
 address.
diff -ruNp a/drivers/net/netconsole.c b/drivers/net/netconsole.c
--- a/drivers/net/netconsole.c	2007-07-10 07:52:44.000000000 +0530
+++ b/drivers/net/netconsole.c	2007-07-10 08:34:26.000000000 +0530
@@ -62,44 +62,93 @@ static int __init option_setup(char *opt
 __setup("netconsole=", option_setup);
 #endif	/* MODULE */
 
+/* Linked list of all configured targets */
+static LIST_HEAD(target_list);
+
+/* This needs to be a spinlock because write_msg() cannot sleep */
+static DEFINE_SPINLOCK(target_list_lock);
+
 /**
  * struct netconsole_target - Represents a configured netconsole target.
+ * @list:	Links this target into the target_list.
  * @np:		The netpoll structure for this target.
  */
 struct netconsole_target {
+	struct list_head	list;
 	struct netpoll		np;
 };
 
-static struct netconsole_target default_target = {
-	.np		= {
-		.name		= "netconsole",
-		.dev_name	= "eth0",
-		.local_port	= 6665,
-		.remote_port	= 6666,
-		.remote_mac	= {0xff, 0xff, 0xff, 0xff, 0xff, 0xff},
-	},
-};
+/* Allocate new target and setup netpoll for it */
+static struct netconsole_target *alloc_target(char *target_config)
+{
+	int err = -ENOMEM;
+	struct netconsole_target *nt;
+
+	/* Allocate and initialize with defaults */
+	nt = kzalloc(sizeof(*nt), GFP_KERNEL);
+	if (!nt) {
+		printk(KERN_ERR "netconsole: failed to allocate memory\n");
+		goto fail;
+	}
+
+	nt->np.name = "netconsole";
+	strlcpy(nt->np.dev_name, "eth0", IFNAMSIZ);
+	nt->np.local_port = 6665;
+	nt->np.remote_port = 6666;
+	memset(nt->np.remote_mac, 0xff, ETH_ALEN);
+
+	/* Parse parameters and setup netpoll */
+	err = netpoll_parse_options(&nt->np, target_config);
+	if (err)
+		goto fail;
+
+	err = netpoll_setup(&nt->np);
+	if (err)
+		goto fail;
+
+	return nt;
+
+fail:
+	kfree(nt);
+	return ERR_PTR(err);
+}
+
+/* Cleanup netpoll for given target and free it */
+static void free_target(struct netconsole_target *nt)
+{
+	netpoll_cleanup(&nt->np);
+	kfree(nt);
+}
 
 /* Handle network interface device notifications */
 static int netconsole_netdev_event(struct notifier_block *this,
 				   unsigned long event,
 				   void *ptr)
 {
+	unsigned long flags;
+	struct netconsole_target *nt;
 	struct net_device *dev = ptr;
-	struct netconsole_target *nt = &default_target;
 
-	if (nt->np.dev == dev) {
-		switch (event) {
-		case NETDEV_CHANGEADDR:
-			memcpy(nt->np.local_mac, dev->dev_addr, ETH_ALEN);
-			break;
-
-		case NETDEV_CHANGENAME:
-			strlcpy(nt->np.dev_name, dev->name, IFNAMSIZ);
-			break;
+	if (!(event == NETDEV_CHANGEADDR || event == NETDEV_CHANGENAME))
+		goto done;
+
+	spin_lock_irqsave(&target_list_lock, flags);
+	list_for_each_entry(nt, &target_list, list) {
+		if (nt->np.dev == dev) {
+			switch (event) {
+			case NETDEV_CHANGEADDR:
+				memcpy(nt->np.local_mac, dev->dev_addr, ETH_ALEN);
+				break;
+
+			case NETDEV_CHANGENAME:
+				strlcpy(nt->np.dev_name, dev->name, IFNAMSIZ);
+				break;
+			}
 		}
 	}
+	spin_unlock_irqrestore(&target_list_lock, flags);
 
+done:
 	return NOTIFY_DONE;
 }
 
@@ -111,18 +160,32 @@ static void write_msg(struct console *co
 {
 	int frag, left;
 	unsigned long flags;
-	struct netconsole_target *nt = &default_target;
+	struct netconsole_target *nt;
+	const char *tmp;
 
-	if (netif_running(nt->np.dev)) {
-		local_irq_save(flags);
-		for (left = len; left;) {
-			frag = min(left, MAX_PRINT_CHUNK);
-			netpoll_send_udp(&nt->np, msg, frag);
-			msg += frag;
-			left -= frag;
+	/* Avoid taking lock and disabling interrupts unnecessarily */
+	if (unlikely(list_empty(&target_list)))
+		return;
+
+	spin_lock_irqsave(&target_list_lock, flags);
+	list_for_each_entry(nt, &target_list, list) {
+		if (netif_running(nt->np.dev)) {
+			/*
+			 * We nest this inside the for-each-target loop above
+			 * so that we're able to get as much logging out to
+			 * at least one target if we die inside here, instead
+			 * of unnecessarily keeping all targets in lock-step.
+			 */
+			tmp = msg;
+			for (left = len; left;) {
+				frag = min(left, MAX_PRINT_CHUNK);
+				netpoll_send_udp(&nt->np, tmp, frag);
+				tmp += frag;
+				left -= frag;
+			}
 		}
-		local_irq_restore(flags);
 	}
+	spin_unlock_irqrestore(&target_list_lock, flags);
 }
 
 static struct console netconsole = {
@@ -134,39 +197,69 @@ static struct console netconsole = {
 static int __init init_netconsole(void)
 {
 	int err = 0;
-	struct netconsole_target *nt = &default_target;
+	struct netconsole_target *nt, *tmp;
+	unsigned long flags;
+	char *target_config;
+	char *input = config;
 
-	if (!strnlen(config, MAX_PARAM_LENGTH)) {
+	if (!strnlen(input, MAX_PARAM_LENGTH)) {
 		printk(KERN_INFO "netconsole: not configured, aborting\n");
 		goto out;
 	}
 
-	err = netpoll_parse_options(&nt->np, config);
-	if (err)
-		goto out;
-
-	err = netpoll_setup(&nt->np);
-	if (err)
-		goto out;
+	while ((target_config = strsep(&input, ";"))) {
+		nt = alloc_target(target_config);
+		if (IS_ERR(nt)) {
+			err = PTR_ERR(nt);
+			goto fail;
+		}
+		spin_lock_irqsave(&target_list_lock, flags);
+		list_add(&nt->list, &target_list);
+		spin_unlock_irqrestore(&target_list_lock, flags);
+	}
 
 	err = register_netdevice_notifier(&netconsole_netdev_notifier);
 	if (err)
-		goto out;
+		goto fail;
 
 	register_console(&netconsole);
 	printk(KERN_INFO "netconsole: network logging started\n");
 
 out:
 	return err;
+
+fail:
+	printk(KERN_ERR "netconsole: cleaning up\n");
+
+	/*
+	 * Remove all targets and destroy them.
+	 * The notifier is off, so nobody's racing us. So let's skip
+	 * the lock, netpoll_cleanup() wants to sleep anyway.
+	 */
+	list_for_each_entry_safe(nt, tmp, &target_list, list) {
+		list_del(&nt->list);
+		free_target(nt);
+	}
+
+	return err;
 }
 
 static void __exit cleanup_netconsole(void)
 {
-	struct netconsole_target *nt = &default_target;
+	struct netconsole_target *nt, *tmp;
 
 	unregister_console(&netconsole);
 	unregister_netdevice_notifier(&netconsole_netdev_notifier);
-	netpoll_cleanup(&nt->np);
+
+	/*
+	 * Remove all targets and destroy them.
+	 * Nobody's racing us, and netpoll_cleanup() wants to
+	 * sleep -- so skip the lock.
+	 */
+	list_for_each_entry_safe(nt, tmp, &target_list, list) {
+		list_del(&nt->list);
+		free_target(nt);
+	}
 }
 
 module_init(init_netconsole);

[PATCH v2 -mm 9/9] netconsole: Support dynamic reconfiguration using configfs

[Satyam Sharma]

From: Satyam Sharma <ssatyam@cse.iitk.ac.in>

[9/9] netconsole: Support dynamic reconfiguration using configfs

This patch introduces support for dynamic reconfiguration (adding, removing
and/or modifying parameters of netconsole targets at runtime) using a
userspace interface exported via configfs. Documentation is also updated
accordingly.

Issues and brief design overview:

(1) Kernel-initiated creation / destruction of kernel objects is not
possible with configfs -- the lifetimes of the "config items" is managed
exclusively from userspace. But netconsole must support boot/module params
too, and these are parsed in kernel and hence netpolls must be setup from
the kernel. Joel Becker suggested to separately manage the lifetimes of
the two kinds of netconsole_target objects -- those created via configfs
mkdir(2) from userspace and those specified from the boot/module option
string. This adds complexity and some redundancy here and also means that
boot/module param-created targets are not exposed through the configfs
namespace (and hence cannot be updated / destroyed dynamically). However,
this saves us from locking / refcounting complexities that would need to
be introduced in configfs to support kernel-initiated item creation /
destroy there. Also, this is similar to present behaviour in any case so
not really a problem.

(2) In configfs, item creation takes place in the call chain of the mkdir(2)
syscall in the driver subsystem. If we used an ioctl(2) to create / destroy
objects from userspace, the special userspace program is able to fill out
the structure to be passed into the ioctl and hence specify attributes such
as local interface that are required at the time we set up the netpoll.
For configfs, this information is not available at the time of mkdir(2).
So, we keep all newly-created targets (via configfs) disabled by default.
The user is expected to set various attributes appropriately (including the
local network interface if required) and then write(2) "1" to the "enabled"
attribute. Thus, netpoll_setup() is then called on the set parameters in the
context of _this_ write(2) on the "enabled" attribute itself. This design
enables the user to reconfigure existing netconsole targets at runtime to
be attached to newly-come-up interfaces that may not have existed when
netconsole was loaded or when the targets were actually created. This all
enables us to get rid of custom ioctls.

(3) Ultra-paranoid configfs attribute show() and store() operations, with
sanity and input range checking, using only safe string primitives, and
compliant with the recommendations in Documentation/filesystems/sysfs.txt.

(4) A new function netpoll_print_options() is created in the netpoll API,
that just prints out the configured parameters for a netpoll structure.
netpoll_parse_options() is modified to use that and it is also exported to
be used from netconsole.

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>

---

 Documentation/networking/netconsole.txt |   68 +++
 drivers/net/Kconfig                     |   10
 drivers/net/netconsole.c                |  607 ++++++++++++++++++++++++++++++--
 include/linux/netpoll.h                 |    1
 net/core/netpoll.c                      |   44 +-
 5 files changed, 686 insertions(+), 44 deletions(-)

---

diff -ruNp a/Documentation/networking/netconsole.txt b/Documentation/networking/netconsole.txt
--- a/Documentation/networking/netconsole.txt	2007-07-10 08:56:16.000000000 +0530
+++ b/Documentation/networking/netconsole.txt	2007-07-10 11:07:48.000000000 +0530
@@ -3,6 +3,10 @@ started by Ingo Molnar <mingo@redhat.com
 2.6 port and netpoll api by Matt Mackall <mpm@selenic.com>, Sep 9 2003
 
 Please send bug reports to Matt Mackall <mpm@selenic.com>
+and Satyam Sharma <satyam.sharma@gmail.com>
+
+Introduction:
+=============
 
 This module logs kernel printk messages over UDP allowing debugging of
 problem where disk logging fails and serial consoles are impractical.
@@ -13,6 +17,9 @@ the specified interface as soon as possi
 capture of early kernel panics, it does capture most of the boot
 process.
 
+Sender and receiver configuration:
+==================================
+
 It takes a string configuration parameter "netconsole" in the
 following format:
 
@@ -46,6 +53,67 @@ address.
 
 The remote host can run either 'netcat -u -l -p <port>' or syslogd.
 
+Dynamic reconfiguration:
+========================
+
+Dynamic reconfigurability is a useful addition to netconsole that enables
+remote logging targets to be dynamically added, removed, or have their
+parameters reconfigured at runtime from a configfs-based userspace interface.
+[ Note that the parameters of netconsole targets that were specified/created
+from the boot/module option are not exposed via this interface, and hence
+cannot be modified dynamically. ]
+
+To include this feature, select CONFIG_NETCONSOLE_DYNAMIC when building the
+netconsole module (or kernel, if netconsole is built-in).
+
+Some examples follow (where configfs is mounted at the /sys/kernel/config
+mountpoint).
+
+To add a remote logging target (target names can be arbitrary):
+
+ cd /sys/kernel/config/netconsole/
+ mkdir target1
+
+Note that newly created targets have default parameter values (as mentioned
+above) and are disabled by default -- they must first be enabled by writing
+"1" to the "enabled" attribute (usually after setting parameters accordingly)
+as described below.
+
+To remove a target:
+
+ rmdir /sys/kernel/config/netconsole/othertarget/
+
+The interface exposes these parameters of a netconsole target to userspace:
+
+	enabled		Is this target currently enabled?	(read-write)
+	dev_name	Local network interface name		(read-write)
+	local_port	Source UDP port to use			(read-write)
+	remote_port	Remote agent's UDP port			(read-write)
+	local_ip	Source IP address to use		(read-write)
+	remote_ip	Remote agent's IP address		(read-write)
+	local_mac	Local interface's MAC address		(read-only)
+	remote_mac	Remote agent's MAC address		(read-write)
+
+The "enabled" attribute is also used to control whether the parameters of
+a target can be updated or not -- you can modify the parameters of only
+disabled targets (i.e. if "enabled" is 0).
+
+To update a target's parameters:
+
+ cat enabled				# check if enabled is 1
+ echo 0 > enabled			# disable the target (if required)
+ echo eth2 > dev_name			# set local interface
+ echo 10.0.0.4 > remote_ip		# update some parameter
+ echo cb:a9:87:65:43:21 > remote_mac	# update more parameters
+ echo 1 > enabled			# enable target again
+
+You can also update the local interface dynamically. This is especially
+useful if you want to use interfaces that have newly come up (and may not
+have existed when netconsole was loaded / initialized).
+
+Miscellaneous notes:
+====================
+
 WARNING: the default target ethernet setting uses the broadcast
 ethernet address to send packets, which can cause increased load on
 other systems on the same ethernet segment.
diff -ruNp a/drivers/net/Kconfig b/drivers/net/Kconfig
--- a/drivers/net/Kconfig	2007-07-04 13:56:32.000000000 +0530
+++ b/drivers/net/Kconfig	2007-07-10 10:26:17.000000000 +0530
@@ -3030,6 +3030,16 @@ config NETCONSOLE
 	If you want to log kernel messages over the network, enable this.
 	See <file:Documentation/networking/netconsole.txt> for details.
 
+config NETCONSOLE_DYNAMIC
+	bool "Dynamic reconfiguration of logging targets (EXPERIMENTAL)"
+	depends on NETCONSOLE && SYSFS && EXPERIMENTAL
+	select CONFIGFS_FS
+	help
+	  This option enables the ability to dynamically reconfigure target
+	  parameters (interface, IP addresses, port numbers, MAC addresses)
+	  at runtime through a userspace interface exported using configfs.
+	  See <file:Documentation/networking/netconsole.txt> for details.
+
 config NETPOLL
 	def_bool NETCONSOLE
 
diff -ruNp a/drivers/net/netconsole.c b/drivers/net/netconsole.c
--- a/drivers/net/netconsole.c	2007-07-10 08:56:16.000000000 +0530
+++ b/drivers/net/netconsole.c	2007-07-10 13:01:39.000000000 +0530
@@ -41,6 +41,8 @@
 #include <linux/moduleparam.h>
 #include <linux/string.h>
 #include <linux/netpoll.h>
+#include <linux/inet.h>
+#include <linux/configfs.h>
 
 MODULE_AUTHOR("Maintainer: Matt Mackall <mpm@selenic.com>");
 MODULE_DESCRIPTION("Console driver for network interfaces");
@@ -71,20 +73,100 @@ static DEFINE_SPINLOCK(target_list_lock)
 /**
  * struct netconsole_target - Represents a configured netconsole target.
  * @list:	Links this target into the target_list.
+ * @item:	Links us into the configfs subsystem hierarchy.
+ * @enabled:	On / off knob to enable / disable target.
+ *		Visible from userspace (read-write).
+ *		We maintain a strict 1:1 correspondence between this and
+ *		whether the corresponding netpoll is active or inactive.
+ *		Also, other parameters of a target may be modified at
+ *		runtime only when it is disabled (enabled == 0).
  * @np:		The netpoll structure for this target.
+ *		Contains the other userspace visible parameters:
+ *		dev_name	(read-write)
+ *		local_port	(read-write)
+ *		remote_port	(read-write)
+ *		local_ip	(read-write)
+ *		remote_ip	(read-write)
+ *		local_mac	(read-only)
+ *		remote_mac	(read-write)
  */
 struct netconsole_target {
 	struct list_head	list;
+#ifdef	CONFIG_NETCONSOLE_DYNAMIC
+	struct config_item	item;
+	int			enabled;
+#endif
 	struct netpoll		np;
 };
 
-/* Allocate new target and setup netpoll for it */
-static struct netconsole_target *alloc_target(char *target_config)
+#ifdef	CONFIG_NETCONSOLE_DYNAMIC
+
+static struct configfs_subsystem netconsole_subsys;
+
+static int __init dynamic_netconsole_init(void)
+{
+	config_group_init(&netconsole_subsys.su_group);
+	mutex_init(&netconsole_subsys.su_mtx);
+	return configfs_register_subsystem(&netconsole_subsys);
+}
+
+static void __exit dynamic_netconsole_exit(void)
+{
+	configfs_unregister_subsystem(&netconsole_subsys);
+}
+
+/*
+ * Targets that were created by parsing the boot/module option string
+ * do not exist in the configfs hierarchy (and have NULL names) and will
+ * never go away, so make these a no-op for them.
+ */
+static void netconsole_target_get(struct netconsole_target *nt)
+{
+	if (config_item_name(&nt->item))
+		config_item_get(&nt->item);
+}
+
+static void netconsole_target_put(struct netconsole_target *nt)
+{
+	if (config_item_name(&nt->item))
+		config_item_put(&nt->item);
+}
+
+#else	/* !CONFIG_NETCONSOLE_DYNAMIC */
+
+static int __init dynamic_netconsole_init(void)
+{
+	return 0;
+}
+
+static void __exit dynamic_netconsole_exit(void)
+{
+}
+
+/*
+ * No danger of targets going away from under us when dynamic
+ * reconfigurability is off.
+ */
+static void netconsole_target_get(struct netconsole_target *nt)
+{
+}
+
+static void netconsole_target_put(struct netconsole_target *nt)
+{
+}
+
+#endif	/* CONFIG_NETCONSOLE_DYNAMIC */
+
+/* Allocate new target (from boot/module param) and setup netpoll for it */
+static struct netconsole_target *alloc_param_target(char *target_config)
 {
 	int err = -ENOMEM;
 	struct netconsole_target *nt;
 
-	/* Allocate and initialize with defaults */
+	/*
+	 * Allocate and initialize with defaults.
+	 * Note that these targets get their config_item fields zeroed-out.
+	 */
 	nt = kzalloc(sizeof(*nt), GFP_KERNEL);
 	if (!nt) {
 		printk(KERN_ERR "netconsole: failed to allocate memory\n");
@@ -106,6 +188,8 @@ static struct netconsole_target *alloc_t
 	if (err)
 		goto fail;
 
+	nt->enabled = 1;
+
 	return nt;
 
 fail:
@@ -113,13 +197,469 @@ fail:
 	return ERR_PTR(err);
 }
 
-/* Cleanup netpoll for given target and free it */
-static void free_target(struct netconsole_target *nt)
+/* Cleanup netpoll for given target (from boot/module param) and free it */
+static void free_param_target(struct netconsole_target *nt)
 {
 	netpoll_cleanup(&nt->np);
 	kfree(nt);
 }
 
+#ifdef	CONFIG_NETCONSOLE_DYNAMIC
+
+/*
+ * Our subsystem hierarchy is:
+ *
+ * /sys/kernel/config/netconsole/
+ *				|
+ *				<target>/
+ *				|	enabled
+ *				|	dev_name
+ *				|	local_port
+ *				|	remote_port
+ *				|	local_ip
+ *				|	remote_ip
+ *				|	local_mac
+ *				|	remote_mac
+ *				|
+ *				<target>/...
+ */
+
+struct netconsole_target_attr {
+	struct configfs_attribute	attr;
+	ssize_t				(*show)(struct netconsole_target *nt,
+						char *buf);
+	ssize_t				(*store)(struct netconsole_target *nt,
+						 const char *buf,
+						 size_t count);
+};
+
+static struct netconsole_target *to_target(struct config_item *item)
+{
+	return item ?
+		container_of(item, struct netconsole_target, item) :
+		NULL;
+}
+
+/*
+ * Wrapper over simple_strtol (base 10) with sanity and range checking.
+ * We return (signed) long only because we may want to return errors.
+ * Do not use this to convert numbers that are allowed to be negative.
+ */
+static long strtol10_check_range(const char *cp, long min, long max)
+{
+	long ret;
+	char *p = (char *) cp;
+
+	WARN_ON(min < 0);
+	WARN_ON(max < min);
+
+	ret = simple_strtol(p, &p, 10);
+
+	if (*p && (*p != '\n')) {
+		printk(KERN_ERR "netconsole: invalid input\n");
+		return -EINVAL;
+	}
+	if ((ret < min) || (ret > max)) {
+		printk(KERN_ERR "netconsole: input %ld must be between "
+				"%ld and %ld\n", ret, min, max);
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+/*
+ * Attribute operations for netconsole_target.
+ */
+
+static ssize_t show_enabled(struct netconsole_target *nt, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d\n", nt->enabled);
+}
+
+static ssize_t show_dev_name(struct netconsole_target *nt, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%s\n", nt->np.dev_name);
+}
+
+static ssize_t show_local_port(struct netconsole_target *nt, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d\n", nt->np.local_port);
+}
+
+static ssize_t show_remote_port(struct netconsole_target *nt, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d\n", nt->np.remote_port);
+}
+
+static ssize_t show_local_ip(struct netconsole_target *nt, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d.%d.%d.%d\n",
+			HIPQUAD(nt->np.local_ip));
+}
+
+static ssize_t show_remote_ip(struct netconsole_target *nt, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d.%d.%d.%d\n",
+			HIPQUAD(nt->np.remote_ip));
+}
+
+static ssize_t show_local_mac(struct netconsole_target *nt, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%02x:%02x:%02x:%02x:%02x:%02x\n",
+			nt->np.local_mac[0], nt->np.local_mac[1],
+			nt->np.local_mac[2], nt->np.local_mac[3],
+			nt->np.local_mac[4], nt->np.local_mac[5]);
+}
+
+static ssize_t show_remote_mac(struct netconsole_target *nt, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%02x:%02x:%02x:%02x:%02x:%02x\n",
+			nt->np.remote_mac[0], nt->np.remote_mac[1],
+			nt->np.remote_mac[2], nt->np.remote_mac[3],
+			nt->np.remote_mac[4], nt->np.remote_mac[5]);
+}
+
+/*
+ * This one is special -- targets created through the configfs interface
+ * are not enabled (and the corresponding netpoll activated) by default.
+ * The user is expected to set the desired parameters first (which
+ * would enable him to dynamically add new netpoll targets for new
+ * network interfaces as and when they come up).
+ */
+static ssize_t store_enabled(struct netconsole_target *nt,
+			     const char *buf,
+			     size_t count)
+{
+	int err;
+	long enabled;
+
+	enabled = strtol10_check_range(buf, 0, 1);
+	if (enabled < 0)
+		return enabled;
+
+	if (enabled) {	/* 1 */
+
+		/*
+		 * Skip netpoll_parse_options() -- all the attributes are
+		 * already configured via configfs. Just print them out.
+		 */
+		netpoll_print_options(&nt->np);
+
+		err = netpoll_setup(&nt->np);
+		if (err)
+			return err;
+
+		printk(KERN_INFO "netconsole: network logging started\n");
+
+	} else {	/* 0 */
+		netpoll_cleanup(&nt->np);
+	}
+
+	nt->enabled = enabled;
+
+	return strnlen(buf, count);
+}
+
+static ssize_t store_dev_name(struct netconsole_target *nt,
+			      const char *buf,
+			      size_t count)
+{
+	size_t len;
+
+	if (nt->enabled) {
+		printk(KERN_ERR "netconsole: target (%s) is enabled, "
+				"disable to update parameters\n",
+				config_item_name(&nt->item));
+		return -EINVAL;
+	}
+
+	strlcpy(nt->np.dev_name, buf, IFNAMSIZ);
+
+	/* Get rid of possible trailing newline from echo(1) */
+	len = strnlen(nt->np.dev_name, IFNAMSIZ);
+	if (nt->np.dev_name[len - 1] == '\n')
+		nt->np.dev_name[len - 1] = '\0';
+
+	return strnlen(buf, count);
+}
+
+static ssize_t store_local_port(struct netconsole_target *nt,
+				const char *buf,
+				size_t count)
+{
+	long local_port;
+#define __U16_MAX	((__u16) ~0U)
+
+	if (nt->enabled) {
+		printk(KERN_ERR "netconsole: target (%s) is enabled, "
+				"disable to update parameters\n",
+				config_item_name(&nt->item));
+		return -EINVAL;
+	}
+
+	local_port = strtol10_check_range(buf, 0, __U16_MAX);
+	if (local_port < 0)
+		return local_port;
+
+	nt->np.local_port = local_port;
+
+	return strnlen(buf, count);
+}
+
+static ssize_t store_remote_port(struct netconsole_target *nt,
+				 const char *buf,
+				 size_t count)
+{
+	long remote_port;
+#define __U16_MAX	((__u16) ~0U)
+
+	if (nt->enabled) {
+		printk(KERN_ERR "netconsole: target (%s) is enabled, "
+				"disable to update parameters\n",
+				config_item_name(&nt->item));
+		return -EINVAL;
+	}
+
+	remote_port = strtol10_check_range(buf, 0, __U16_MAX);
+	if (remote_port < 0)
+		return remote_port;
+
+	nt->np.remote_port = remote_port;
+
+	return strnlen(buf, count);
+}
+
+static ssize_t store_local_ip(struct netconsole_target *nt,
+			      const char *buf,
+			      size_t count)
+{
+	if (nt->enabled) {
+		printk(KERN_ERR "netconsole: target (%s) is enabled, "
+				"disable to update parameters\n",
+				config_item_name(&nt->item));
+		return -EINVAL;
+	}
+
+	nt->np.local_ip = ntohl(in_aton(buf));
+
+	return strnlen(buf, count);
+}
+
+static ssize_t store_remote_ip(struct netconsole_target *nt,
+			       const char *buf,
+			       size_t count)
+{
+	if (nt->enabled) {
+		printk(KERN_ERR "netconsole: target (%s) is enabled, "
+				"disable to update parameters\n",
+				config_item_name(&nt->item));
+		return -EINVAL;
+	}
+
+	nt->np.remote_ip = ntohl(in_aton(buf));
+
+	return strnlen(buf, count);
+}
+
+static ssize_t store_remote_mac(struct netconsole_target *nt,
+				const char *buf,
+				size_t count)
+{
+	u8 remote_mac[ETH_ALEN];
+	char *p = (char *) buf;
+	int i;
+
+	if (nt->enabled) {
+		printk(KERN_ERR "netconsole: target (%s) is enabled, "
+				"disable to update parameters\n",
+				config_item_name(&nt->item));
+		return -EINVAL;
+	}
+
+	for (i = 0; i < ETH_ALEN - 1; i++) {
+		remote_mac[i] = simple_strtoul(p, &p, 16);
+		if (*p != ':')
+			goto invalid;
+		p++;
+	}
+	remote_mac[ETH_ALEN - 1] = simple_strtoul(p, &p, 16);
+	if (*p && (*p != '\n'))
+		goto invalid;
+
+	memcpy(nt->np.remote_mac, remote_mac, ETH_ALEN);
+
+	return strnlen(buf, count);
+
+invalid:
+	printk(KERN_ERR "netconsole: invalid input\n");
+	return -EINVAL;
+}
+
+/*
+ * Attribute definitions for netconsole_target.
+ */
+
+#define NETCONSOLE_TARGET_ATTR_RO(_name)				\
+static struct netconsole_target_attr netconsole_target_##_name =	\
+	__CONFIGFS_ATTR(_name, S_IRUGO, show_##_name, NULL)
+
+#define NETCONSOLE_TARGET_ATTR_RW(_name)				\
+static struct netconsole_target_attr netconsole_target_##_name =	\
+	__CONFIGFS_ATTR(_name, S_IRUGO | S_IWUSR, show_##_name, store_##_name)
+
+NETCONSOLE_TARGET_ATTR_RW(enabled);
+NETCONSOLE_TARGET_ATTR_RW(dev_name);
+NETCONSOLE_TARGET_ATTR_RW(local_port);
+NETCONSOLE_TARGET_ATTR_RW(remote_port);
+NETCONSOLE_TARGET_ATTR_RW(local_ip);
+NETCONSOLE_TARGET_ATTR_RW(remote_ip);
+NETCONSOLE_TARGET_ATTR_RO(local_mac);
+NETCONSOLE_TARGET_ATTR_RW(remote_mac);
+
+static struct configfs_attribute *netconsole_target_attrs[] = {
+	&netconsole_target_enabled.attr,
+	&netconsole_target_dev_name.attr,
+	&netconsole_target_local_port.attr,
+	&netconsole_target_remote_port.attr,
+	&netconsole_target_local_ip.attr,
+	&netconsole_target_remote_ip.attr,
+	&netconsole_target_local_mac.attr,
+	&netconsole_target_remote_mac.attr,
+	NULL,
+};
+
+/*
+ * Item operations and type for netconsole_target.
+ */
+
+static void netconsole_target_release(struct config_item *item)
+{
+	kfree(to_target(item));
+}
+
+static ssize_t netconsole_target_attr_show(struct config_item *item,
+					   struct configfs_attribute *attr,
+					   char *buf)
+{
+	ssize_t ret = -EINVAL;
+	struct netconsole_target *nt = to_target(item);
+	struct netconsole_target_attr *na =
+		container_of(attr, struct netconsole_target_attr, attr);
+
+	if (na->show)
+		ret = na->show(nt, buf);
+
+	return ret;
+}
+
+static ssize_t netconsole_target_attr_store(struct config_item *item,
+					    struct configfs_attribute *attr,
+					    const char *buf,
+					    size_t count)
+{
+	ssize_t ret = -EINVAL;
+	struct netconsole_target *nt = to_target(item);
+	struct netconsole_target_attr *na =
+		container_of(attr, struct netconsole_target_attr, attr);
+
+	if (na->store)
+		ret = na->store(nt, buf, count);
+
+	return ret;
+}
+
+static struct configfs_item_operations netconsole_target_item_ops = {
+	.release		= netconsole_target_release,
+	.show_attribute		= netconsole_target_attr_show,
+	.store_attribute	= netconsole_target_attr_store,
+};
+
+static struct config_item_type netconsole_target_type = {
+	.ct_attrs		= netconsole_target_attrs,
+	.ct_item_ops		= &netconsole_target_item_ops,
+	.ct_owner		= THIS_MODULE,
+};
+
+/*
+ * Group operations and type for netconsole_subsys.
+ */
+
+static struct config_item *make_netconsole_target(struct config_group *group,
+						  const char *name)
+{
+	unsigned long flags;
+	struct netconsole_target *nt;
+
+	/*
+	 * Allocate and initialize with defaults.
+	 * Target is disabled at creation (enabled == 0).
+	 */
+	nt = kzalloc(sizeof(*nt), GFP_KERNEL);
+	if (!nt) {
+		printk(KERN_ERR "netconsole: failed to allocate memory\n");
+		return NULL;
+	}
+
+	nt->np.name = "netconsole";
+	strlcpy(nt->np.dev_name, "eth0", IFNAMSIZ);
+	nt->np.local_port = 6665;
+	nt->np.remote_port = 6666;
+	memset(nt->np.remote_mac, 0xff, ETH_ALEN);
+
+	/* Initialize the config_item member */
+	config_item_init_type_name(&nt->item, name, &netconsole_target_type);
+
+	/* Adding, but it is disabled */
+	spin_lock_irqsave(&target_list_lock, flags);
+	list_add(&nt->list, &target_list);
+	spin_unlock_irqrestore(&target_list_lock, flags);
+
+	return &nt->item;
+}
+
+static void drop_netconsole_target(struct config_group *group,
+				   struct config_item *item)
+{
+	unsigned long flags;
+	struct netconsole_target *nt = to_target(item);
+
+	spin_lock_irqsave(&target_list_lock, flags);
+	list_del(&nt->list);
+	spin_unlock_irqrestore(&target_list_lock, flags);
+
+	/*
+	 * The target may have never been enabled, or was manually disabled
+	 * before being removed so netpoll may have already been cleaned up.
+	 */
+	if (nt->enabled)
+		netpoll_cleanup(&nt->np);
+
+	config_item_put(&nt->item);
+}
+
+static struct configfs_group_operations netconsole_subsys_group_ops = {
+	.make_item	= make_netconsole_target,
+	.drop_item	= drop_netconsole_target,
+};
+
+static struct config_item_type netconsole_subsys_type = {
+	.ct_group_ops	= &netconsole_subsys_group_ops,
+	.ct_owner	= THIS_MODULE,
+};
+
+/* The netconsole configfs subsystem */
+static struct configfs_subsystem netconsole_subsys = {
+	.su_group	= {
+		.cg_item	= {
+			.ci_namebuf	= "netconsole",
+			.ci_type	= &netconsole_subsys_type,
+		},
+	},
+};
+
+#endif	/* CONFIG_NETCONSOLE_DYNAMIC */
+
 /* Handle network interface device notifications */
 static int netconsole_netdev_event(struct notifier_block *this,
 				   unsigned long event,
@@ -134,6 +674,7 @@ static int netconsole_netdev_event(struc
 
 	spin_lock_irqsave(&target_list_lock, flags);
 	list_for_each_entry(nt, &target_list, list) {
+		netconsole_target_get(nt);
 		if (nt->np.dev == dev) {
 			switch (event) {
 			case NETDEV_CHANGEADDR:
@@ -145,6 +686,7 @@ static int netconsole_netdev_event(struc
 				break;
 			}
 		}
+		netconsole_target_put(nt);
 	}
 	spin_unlock_irqrestore(&target_list_lock, flags);
 
@@ -169,7 +711,8 @@ static void write_msg(struct console *co
 
 	spin_lock_irqsave(&target_list_lock, flags);
 	list_for_each_entry(nt, &target_list, list) {
-		if (netif_running(nt->np.dev)) {
+		netconsole_target_get(nt);
+		if (nt->enabled && netif_running(nt->np.dev)) {
 			/*
 			 * We nest this inside the for-each-target loop above
 			 * so that we're able to get as much logging out to
@@ -184,6 +727,7 @@ static void write_msg(struct console *co
 				left -= frag;
 			}
 		}
+		netconsole_target_put(nt);
 	}
 	spin_unlock_irqrestore(&target_list_lock, flags);
 }
@@ -196,49 +740,55 @@ static struct console netconsole = {
 
 static int __init init_netconsole(void)
 {
-	int err = 0;
+	int err;
 	struct netconsole_target *nt, *tmp;
 	unsigned long flags;
 	char *target_config;
 	char *input = config;
 
-	if (!strnlen(input, MAX_PARAM_LENGTH)) {
-		printk(KERN_INFO "netconsole: not configured, aborting\n");
-		goto out;
-	}
-
-	while ((target_config = strsep(&input, ";"))) {
-		nt = alloc_target(target_config);
-		if (IS_ERR(nt)) {
-			err = PTR_ERR(nt);
-			goto fail;
+	if (strnlen(input, MAX_PARAM_LENGTH)) {
+		while ((target_config = strsep(&input, ";"))) {
+			nt = alloc_param_target(target_config);
+			if (IS_ERR(nt)) {
+				err = PTR_ERR(nt);
+				goto fail;
+			}
+			spin_lock_irqsave(&target_list_lock, flags);
+			list_add(&nt->list, &target_list);
+			spin_unlock_irqrestore(&target_list_lock, flags);
 		}
-		spin_lock_irqsave(&target_list_lock, flags);
-		list_add(&nt->list, &target_list);
-		spin_unlock_irqrestore(&target_list_lock, flags);
 	}
 
 	err = register_netdevice_notifier(&netconsole_netdev_notifier);
 	if (err)
 		goto fail;
 
+	err = dynamic_netconsole_init();
+	if (err)
+		goto undonotifier;
+
 	register_console(&netconsole);
 	printk(KERN_INFO "netconsole: network logging started\n");
 
-out:
 	return err;
 
+undonotifier:
+	unregister_netdevice_notifier(&netconsole_netdev_notifier);
+
 fail:
 	printk(KERN_ERR "netconsole: cleaning up\n");
 
 	/*
-	 * Remove all targets and destroy them.
+	 * Remove all targets and destroy them -- only targets created
+	 * while parsing the boot/module option exist, we cannot destroy
+	 * configfs items from kernel anyway.
+	 *
 	 * The notifier is off, so nobody's racing us. So let's skip
 	 * the lock, netpoll_cleanup() wants to sleep anyway.
 	 */
 	list_for_each_entry_safe(nt, tmp, &target_list, list) {
 		list_del(&nt->list);
-		free_target(nt);
+		free_param_target(nt);
 	}
 
 	return err;
@@ -249,16 +799,23 @@ static void __exit cleanup_netconsole(vo
 	struct netconsole_target *nt, *tmp;
 
 	unregister_console(&netconsole);
+	dynamic_netconsole_exit();
 	unregister_netdevice_notifier(&netconsole_netdev_notifier);
 
 	/*
-	 * Remove all targets and destroy them.
+	 * Targets created via configfs would have pinned references on
+	 * our module and wouldn't let us unload at all. They would first
+	 * need to be rmdir(2)'ed from userspace (and hence would be
+	 * removed and released already when we reach here). Only those
+	 * targets that were created for the boot/module option string
+	 * are left, remove and destroy them.
+	 *
 	 * Nobody's racing us, and netpoll_cleanup() wants to
 	 * sleep -- so skip the lock.
 	 */
 	list_for_each_entry_safe(nt, tmp, &target_list, list) {
 		list_del(&nt->list);
-		free_target(nt);
+		free_param_target(nt);
 	}
 }
 
diff -ruNp a/include/linux/netpoll.h b/include/linux/netpoll.h
--- a/include/linux/netpoll.h	2007-06-28 17:33:21.000000000 +0530
+++ b/include/linux/netpoll.h	2007-07-10 12:36:37.000000000 +0530
@@ -37,6 +37,7 @@ struct netpoll_info {
 
 void netpoll_poll(struct netpoll *np);
 void netpoll_send_udp(struct netpoll *np, const char *msg, int len);
+void netpoll_print_options(struct netpoll *np);
 int netpoll_parse_options(struct netpoll *np, char *opt);
 int netpoll_setup(struct netpoll *np);
 int netpoll_trap(void);
diff -ruNp a/net/core/netpoll.c b/net/core/netpoll.c
--- a/net/core/netpoll.c	2007-06-28 17:33:29.000000000 +0530
+++ b/net/core/netpoll.c	2007-07-10 10:36:33.000000000 +0530
@@ -519,6 +519,29 @@ out:
 	return 0;
 }
 
+void netpoll_print_options(struct netpoll *np)
+{
+	printk(KERN_INFO "%s: local port %d\n",
+			 np->name, np->local_port);
+	printk(KERN_INFO "%s: local IP %d.%d.%d.%d\n",
+			 np->name, HIPQUAD(np->local_ip));
+	printk(KERN_INFO "%s: interface %s\n",
+			 np->name, np->dev_name);
+	printk(KERN_INFO "%s: remote port %d\n",
+			 np->name, np->remote_port);
+	printk(KERN_INFO "%s: remote IP %d.%d.%d.%d\n",
+			 np->name, HIPQUAD(np->remote_ip));
+	printk(KERN_INFO "%s: remote ethernet address "
+			 "%02x:%02x:%02x:%02x:%02x:%02x\n",
+			 np->name,
+			 np->remote_mac[0],
+			 np->remote_mac[1],
+			 np->remote_mac[2],
+			 np->remote_mac[3],
+			 np->remote_mac[4],
+			 np->remote_mac[5]);
+}
+
 int netpoll_parse_options(struct netpoll *np, char *opt)
 {
 	char *cur=opt, *delim;
@@ -531,7 +554,6 @@ int netpoll_parse_options(struct netpoll
 		cur = delim;
 	}
 	cur++;
-	printk(KERN_INFO "%s: local port %d\n", np->name, np->local_port);
 
 	if (*cur != '/') {
 		if ((delim = strchr(cur, '/')) == NULL)
@@ -539,9 +561,6 @@ int netpoll_parse_options(struct netpoll
 		*delim = 0;
 		np->local_ip = ntohl(in_aton(cur));
 		cur = delim;
-
-		printk(KERN_INFO "%s: local IP %d.%d.%d.%d\n",
-		       np->name, HIPQUAD(np->local_ip));
 	}
 	cur++;
 
@@ -555,8 +574,6 @@ int netpoll_parse_options(struct netpoll
 	}
 	cur++;
 
-	printk(KERN_INFO "%s: interface %s\n", np->name, np->dev_name);
-
 	if (*cur != '@') {
 		/* dst port */
 		if ((delim = strchr(cur, '@')) == NULL)
@@ -566,7 +583,6 @@ int netpoll_parse_options(struct netpoll
 		cur = delim;
 	}
 	cur++;
-	printk(KERN_INFO "%s: remote port %d\n", np->name, np->remote_port);
 
 	/* dst ip */
 	if ((delim = strchr(cur, '/')) == NULL)
@@ -575,9 +591,6 @@ int netpoll_parse_options(struct netpoll
 	np->remote_ip = ntohl(in_aton(cur));
 	cur = delim + 1;
 
-	printk(KERN_INFO "%s: remote IP %d.%d.%d.%d\n",
-	       np->name, HIPQUAD(np->remote_ip));
-
 	if (*cur != 0) {
 		/* MAC address */
 		if ((delim = strchr(cur, ':')) == NULL)
@@ -608,15 +621,7 @@ int netpoll_parse_options(struct netpoll
 		np->remote_mac[5] = simple_strtol(cur, NULL, 16);
 	}
 
-	printk(KERN_INFO "%s: remote ethernet address "
-	       "%02x:%02x:%02x:%02x:%02x:%02x\n",
-	       np->name,
-	       np->remote_mac[0],
-	       np->remote_mac[1],
-	       np->remote_mac[2],
-	       np->remote_mac[3],
-	       np->remote_mac[4],
-	       np->remote_mac[5]);
+	netpoll_print_options(np);
 
 	return 0;
 
@@ -821,6 +826,7 @@ void netpoll_set_trap(int trap)
 
 EXPORT_SYMBOL(netpoll_set_trap);
 EXPORT_SYMBOL(netpoll_trap);
+EXPORT_SYMBOL(netpoll_print_options);
 EXPORT_SYMBOL(netpoll_parse_options);
 EXPORT_SYMBOL(netpoll_setup);
 EXPORT_SYMBOL(netpoll_cleanup);

Re: [PATCH v2 -mm 3/9] netconsole: Simplify boot/module option setup logic

[Matt Mackall]

On Tue, Jul 10, 2007 at 02:49:36PM +0530, Satyam Sharma wrote:
> From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> 
> [3/9] netconsole: Simplify boot/module option setup logic

Acked-by: Matt Mackall <mpm@selenic.com>

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH v2 -mm 4/9] netconsole: Add some useful tips to documentation

[Matt Mackall]

On Tue, Jul 10, 2007 at 02:49:41PM +0530, Satyam Sharma wrote:
> From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> 
> [4/9] netconsole: Add some useful tips to documentation
> 
> Add some useful general-purpose tips.
> 
> Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>

Acked-by: Matt Mackall <mpm@selenic.com>

As long as we're on the subject, I've been meaning to add a note
telling people to set their console log level to something useful, as
having that set too low is the most common problem people encounter.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH v2 -mm 8/9] netconsole: Support multiple logging targets

[Duane Griffin]

On 10/07/07, Satyam Sharma <ssatyam@cse.iitk.ac.in> wrote:
> +       /* Avoid taking lock and disabling interrupts unnecessarily */
> +       if (unlikely(list_empty(&target_list)))
> +               return;

Is the unlikely a good idea here? Not having any targets may be
unusual but it isn't ridiculous. It might even be a sensible default
for distros. My (very limited) understanding of unlikely is that it
could impose a very large penalty in that case and would make a very
marginal difference at best in the common case.

Cheers,
Duane.

-- 
"I never could learn to drink that blood and call it wine" - Bob Dylan

Re: [PATCH v2 -mm 4/9] netconsole: Add some useful tips to documentation

[Jesper Juhl]

On Tuesday 10 July 2007 11:41:43 Matt Mackall wrote:
> On Tue, Jul 10, 2007 at 02:49:41PM +0530, Satyam Sharma wrote:
> > From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> >
> > [4/9] netconsole: Add some useful tips to documentation
> >
> > Add some useful general-purpose tips.
> >
> > Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> > Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>
>
> Acked-by: Matt Mackall <mpm@selenic.com>
>
> As long as we're on the subject, I've been meaning to add a note
> telling people to set their console log level to something useful, as
> having that set too low is the most common problem people encounter.

How about this?


From: Satyam Sharma <ssatyam@cse.iitk.ac.in>

Add some useful general-purpose tips.

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>
Acked-by: Matt Mackall <mpm@selenic.com>
Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>

---

 Documentation/networking/netconsole.txt |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/Documentation/networking/netconsole.txt b/Documentation/networking/netconsole.txt
index 1caa6c7..ffd5058 100644
--- a/Documentation/networking/netconsole.txt
+++ b/Documentation/networking/netconsole.txt
@@ -44,11 +44,29 @@ WARNING: the default target ethernet setting uses the broadcast
 ethernet address to send packets, which can cause increased load on
 other systems on the same ethernet segment.
 
+TIP: some LAN switches may be configured to suppress ethernet broadcasts
+so it is advised to explicitly specify the remote agents' MAC addresses
+from the config parameters passed to netconsole.
+
+TIP: to find out the MAC address of, say, 10.0.0.2, you may try using:
+
+ ping -c 1 10.0.0.2 ; /sbin/arp -n | grep 10.0.0.2
+
+TIP: in case the remote logging agent is on a separate LAN subnet than
+the sender, it is suggested to try specifying the MAC address of the
+default gateway (you may use /sbin/route -n to find it out) as the
+remote MAC address instead.
+
 NOTE: the network device (eth1 in the above case) can run any kind
 of other network traffic, netconsole is not intrusive. Netconsole
 might cause slight delays in other traffic if the volume of kernel
 messages is high, but should have no other impact.
 
+Some people forget to raise the kernels log_level to an
+appropriate level and thus don't see all the kernel log messages they
+expect. You can add the kernel boot option "ignore_loglevel" to see all
+messages or you can use SysRq to set the log level to a specific value.
+
 Netconsole was designed to be as instantaneous as possible, to
 enable the logging of even the most critical kernel bugs. It works
 from IRQ contexts as well, and does not enable interrupts while

Re: [PATCH v2 -mm 4/9] netconsole: Add some useful tips to documentation

[Satyam Sharma]

Hi Jesper,

On Tue, 10 Jul 2007, Jesper Juhl wrote:
> On Tuesday 10 July 2007 11:41:43 Matt Mackall wrote:
> > On Tue, Jul 10, 2007 at 02:49:41PM +0530, Satyam Sharma wrote:
> > > From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> > >
> > > [4/9] netconsole: Add some useful tips to documentation
> > >
> > > Add some useful general-purpose tips.
> > >
> > > Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> > > Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>
> >
> > Acked-by: Matt Mackall <mpm@selenic.com>
> >
> > As long as we're on the subject, I've been meaning to add a note
> > telling people to set their console log level to something useful, as
> > having that set too low is the most common problem people encounter.
> 
> How about this?
> [...]
> +Some people forget to raise the kernels log_level to an
> +appropriate level and thus don't see all the kernel log messages they
> +expect. You can add the kernel boot option "ignore_loglevel" to see all
> +messages or you can use SysRq to set the log level to a specific value.
> +

IMHO something that mentions /proc/sys/kernel/printk would be better.

You don't need to have built with SysRq support for that, it's clearly
more flexible than the ignore_loglevel option and wouldn't require a
reboot either. I'll send out an updated patch shortly.

Satyam

Re: [PATCH v2 -mm 8/9] netconsole: Support multiple logging targets

[Satyam Sharma]

Hi,

On Tue, 10 Jul 2007, Duane Griffin wrote:
> On 10/07/07, Satyam Sharma <ssatyam@cse.iitk.ac.in> wrote:
> > +       /* Avoid taking lock and disabling interrupts unnecessarily */
> > +       if (unlikely(list_empty(&target_list)))
> > +               return;
> 
> Is the unlikely a good idea here? Not having any targets may be
> unusual but it isn't ridiculous. It might even be a sensible default
> for distros.

Hmm, I put it in there because I expected that the user must have had
at least one target configured (added to target_list) if he's got the
module loaded/built-in (and netconsole registered), which is when this
function would be triggered anyway.

> My (very limited) understanding of unlikely is that it
> could impose a very large penalty in that case and would make a very
> marginal difference at best in the common case.

I wouldn't call it a large penalty, but yeah, probably the unlikely()
there isn't all that important either.

Satyam

Re: [PATCH v2 -mm 4/9] netconsole: Add some useful tips to documentation

[Joel Becker]

On Wed, Jul 11, 2007 at 03:40:22AM +0530, Satyam Sharma wrote:
> IMHO something that mentions /proc/sys/kernel/printk would be better.
> 
> You don't need to have built with SysRq support for that, it's clearly
> more flexible than the ignore_loglevel option and wouldn't require a
> reboot either. I'll send out an updated patch shortly.

	Why not dmesg -n?  We've been using that for years.  Or is there
some extra change in /proc/sys/kernel/printk?

Joel

-- 

"Against stupidity the Gods themselves contend in vain."
	- Friedrich von Schiller

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [PATCH v2 -mm 8/9] netconsole: Support multiple logging targets

[Joel Becker]

On Wed, Jul 11, 2007 at 03:47:09AM +0530, Satyam Sharma wrote:
> Hmm, I put it in there because I expected that the user must have had
> at least one target configured (added to target_list) if he's got the
> module loaded/built-in (and netconsole registered), which is when this
> function would be triggered anyway.

	Loading the module won't happen without a config, but there's no
requirement that a built-in netconsole has a kernel command-line
argument.  I always build netconsole for my kernels.  Sure, I usually do
it as a module, but if I build a monolithic kernel I compile it in.
That doesn't mean I always have a server, let alone a netconsole= option
:-)

Joel

-- 

"When choosing between two evils, I always like to try the one
 I've never tried before."
        - Mae West

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [PATCH v2 -mm 4/9] netconsole: Add some useful tips to documentation

[Satyam Sharma]

On Tue, 10 Jul 2007, Joel Becker wrote:
> On Wed, Jul 11, 2007 at 03:40:22AM +0530, Satyam Sharma wrote:
> > IMHO something that mentions /proc/sys/kernel/printk would be better.
> > 
> > You don't need to have built with SysRq support for that, it's clearly
> > more flexible than the ignore_loglevel option and wouldn't require a
> > reboot either. I'll send out an updated patch shortly.
> 
> 	Why not dmesg -n?  We've been using that for years.  Or is there
> some extra change in /proc/sys/kernel/printk?

Yes, "dmesg -n" sounds the most straightforward. There are multiple ways
of course, for some reason I've always used /proc/sys/kernel/printk for
this (which has the extra "feature" that it accepts values greater than
8 too :-)

Satyam

Re: [PATCH v2 -mm 8/9] netconsole: Support multiple logging targets

[Satyam Sharma]

Hi Joel,

On Tue, 10 Jul 2007, Joel Becker wrote:
> On Wed, Jul 11, 2007 at 03:47:09AM +0530, Satyam Sharma wrote:
> > Hmm, I put it in there because I expected that the user must have had
> > at least one target configured (added to target_list) if he's got the
> > module loaded/built-in (and netconsole registered), which is when this
> > function would be triggered anyway.
> 
> 	Loading the module won't happen without a config,

That's no longer the case (doesn't need to be enforced actually) now
with being able to add logging targets _after_ the module has loaded
and initialized / registered itself. It's plausible a user may simply
want to load netconsole without any "netconsole=" option, and then
specify/add targets later -- this wasn't possible previously, of course.

> but there's no
> requirement that a built-in netconsole has a kernel command-line
> argument.

The above is applicable for the built-in case too, with
CONFIG_NETCONSOLE_DYNAMIC. So now built-in netconsole will initialize
(configfs subsystem, etc) and register itself (register_console) even
if no "netconsole=" was provided, again for the same reason.

And I now notice that CONFIG_NETCONSOLE=y in the defconfig's of
most arch's. So I think I did get the common case wrong there ...
ok, I'll get rid of the unlikely() in that case.

Satyam

Re: [RFC][PATCH v2 -mm 0/9] netconsole: Multiple targets and dynamic reconfigurability

[Keiichi KII]

Hi Satyam,

I'm going to test/review these patches and report the result of tests 
on IA64/x86 until this Friday.

> [0/9] netconsole: Multiple targets and dynamic reconfigurability
> 
> This patchset is a rework of the original idea and patches posted by
> Keiichi Kii and Takayoshi Kochi at: http://lkml.org/lkml/2007/6/13/72
> 
> This is v2 of the patchset, the previous version is available at:
> http://lkml.org/lkml/2007/7/4/107
> 
> This is diffed against 2.6.22-rc6-mm1 + the earlier netpoll fixlet and the
> configfs cleanup patches (those are already merged in -mm AFAIK, and hence
> not reproduced here).
> 
> [1/9] netconsole: Cleanups, codingstyle, prettyfication
> [2/9] netconsole: Remove bogus check
> [3/9] netconsole: Simplify boot/module option setup logic
> [4/9] netconsole: Add some useful tips to documentation
> [5/9] netconsole: Introduce netconsole_target
> [6/9] netconsole: Introduce netconsole_netdev_notifier
> [7/9] netconsole: Use netif_running() in write_msg()
> [8/9] netconsole: Support multiple logging targets
> [9/9] netconsole: Support dynamic reconfiguration using configfs

Thanks
-- 
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com

Re: [PATCH v2 -mm 4/9] netconsole: Add some useful tips to documentation

[Jesper Juhl]

On 11/07/07, Satyam Sharma <ssatyam@cse.iitk.ac.in> wrote:
> On Tue, 10 Jul 2007, Joel Becker wrote:
> > On Wed, Jul 11, 2007 at 03:40:22AM +0530, Satyam Sharma wrote:
> > > IMHO something that mentions /proc/sys/kernel/printk would be better.
> > >
> > > You don't need to have built with SysRq support for that, it's clearly
> > > more flexible than the ignore_loglevel option and wouldn't require a
> > > reboot either. I'll send out an updated patch shortly.
> >
> >       Why not dmesg -n?  We've been using that for years.  Or is there
> > some extra change in /proc/sys/kernel/printk?
>
> Yes, "dmesg -n" sounds the most straightforward. There are multiple ways
> of course, for some reason I've always used /proc/sys/kernel/printk for
> this (which has the extra "feature" that it accepts values greater than
> 8 too :-)
>
Why not mention all the various methods, dmesg -n,
/proc/sys/kernel/printk, SysRq, ignore_loglevel  ??

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH v2 -mm 9/9] netconsole: Support dynamic reconfiguration using configfs

[Keiichi KII]

Hi Satyam,

>  struct netconsole_target {
>  	struct list_head	list;
> +#ifdef	CONFIG_NETCONSOLE_DYNAMIC
> +	struct config_item	item;
> +	int			enabled;
> +#endif
>  	struct netpoll		np;
>  };

If CONFIG_NETCONSOLE_DYNAMIC is unset, we can't access to the "enabled" member.
So, the compile errors occur because the following functions make use of 
the above one.

> +/* Allocate new target (from boot/module param) and setup netpoll for it */
> +static struct netconsole_target *alloc_param_target(char *target_config)
>  {
>  	int err = -ENOMEM;
>  	struct netconsole_target *nt;
>  
> -	/* Allocate and initialize with defaults */
> +	/*
> +	 * Allocate and initialize with defaults.
> +	 * Note that these targets get their config_item fields zeroed-out.
> +	 */
>  	nt = kzalloc(sizeof(*nt), GFP_KERNEL);
>  	if (!nt) {
>  		printk(KERN_ERR "netconsole: failed to allocate memory\n");
> @@ -106,6 +188,8 @@ static struct netconsole_target *alloc_t
>  	if (err)
>  		goto fail;
>  
> +	nt->enabled = 1;
> +
>  	return nt;
>  
>  fail:
> @@ -113,13 +197,469 @@ fail:
>  	return ERR_PTR(err);
>  }


> @@ -169,7 +711,8 @@ static void write_msg(struct console *co
>  
>  	spin_lock_irqsave(&target_list_lock, flags);
>  	list_for_each_entry(nt, &target_list, list) {
> -		if (netif_running(nt->np.dev)) {
> +		netconsole_target_get(nt);
> +		if (nt->enabled && netif_running(nt->np.dev)) {
>  			/*
>  			 * We nest this inside the for-each-target loop above
>  			 * so that we're able to get as much logging out to
> @@ -184,6 +727,7 @@ static void write_msg(struct console *co
>  				left -= frag;
>  			}
>  		}
> +		netconsole_target_put(nt);
>  	}
>  	spin_unlock_irqrestore(&target_list_lock, flags);
>  }

I created the following patch for performing some tests.
If there is nothing wrong with the patch, I'm going to continue to test.

Signed-off-by: Keiichi Kii <k-keiichi@bx.jp.nec.com>

Index: mm/drivers/net/netconsole.c
===================================================================
--- mm.orig/drivers/net/netconsole.c
+++ mm/drivers/net/netconsole.c
@@ -94,8 +94,8 @@ struct netconsole_target {
        struct list_head        list;
 #ifdef CONFIG_NETCONSOLE_DYNAMIC
        struct config_item      item;
-       int                     enabled;
 #endif
+       int                     enabled;
        struct netpoll          np;
 };


Thanks
-- 
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com

Re: [PATCH v2 -mm 9/9] netconsole: Support dynamic reconfiguration using configfs

[Satyam Sharma]

Hi,

On Thu, 12 Jul 2007, Keiichi KII wrote:
> Hi Satyam,
> 
> >  struct netconsole_target {
> >  	struct list_head	list;
> > +#ifdef	CONFIG_NETCONSOLE_DYNAMIC
> > +	struct config_item	item;
> > +	int			enabled;
> > +#endif
> >  	struct netpoll		np;
> >  };
> 
> If CONFIG_NETCONSOLE_DYNAMIC is unset, we can't access to the "enabled" member.
> So, the compile errors occur because the following functions make use of 
> the above one.

Gargh, yes.

> Signed-off-by: Keiichi Kii <k-keiichi@bx.jp.nec.com>
> 
> Index: mm/drivers/net/netconsole.c
> ===================================================================
> --- mm.orig/drivers/net/netconsole.c
> +++ mm/drivers/net/netconsole.c
> @@ -94,8 +94,8 @@ struct netconsole_target {
>         struct list_head        list;
>  #ifdef CONFIG_NETCONSOLE_DYNAMIC
>         struct config_item      item;
> -       int                     enabled;
>  #endif
> +       int                     enabled;
>         struct netpoll          np;
>  };

Yup, enabled should be out of CONFIG_NETCONSOLE_DYNAMIC. I'll include this
change in the next version as well.

Thanks,
Satyam

Re: [RFC][PATCH v2 -mm 0/9] netconsole: Multiple targets and dynamic reconfigurability

[KII Keiichi]

Hi Satyam,

> [0/9] netconsole: Multiple targets and dynamic reconfigurability
> 
> This patchset is a rework of the original idea and patches posted by
> Keiichi Kii and Takayoshi Kochi at: http://lkml.org/lkml/2007/6/13/72
> 
> This is v2 of the patchset, the previous version is available at:
> http://lkml.org/lkml/2007/7/4/107
> 
> This is diffed against 2.6.22-rc6-mm1 + the earlier netpoll fixlet and the
> configfs cleanup patches (those are already merged in -mm AFAIK, and hence
> not reproduced here).
> 
> [1/9] netconsole: Cleanups, codingstyle, prettyfication
> [2/9] netconsole: Remove bogus check
> [3/9] netconsole: Simplify boot/module option setup logic
> [4/9] netconsole: Add some useful tips to documentation
> [5/9] netconsole: Introduce netconsole_target
> [6/9] netconsole: Introduce netconsole_netdev_notifier
> [7/9] netconsole: Use netif_running() in write_msg()
> [8/9] netconsole: Support multiple logging targets
> [9/9] netconsole: Support dynamic reconfiguration using configfs
> 

I tested your v2 patches on the x86/IA64 architecture.
There was no problem on my tests.

Signed-off-by: Keiichi Kii <k-keiichi@bx.jp.nec.com>

Thanks
-- 
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com 

Re: [PATCH v2 -mm 1/9] netconsole: Cleanups, codingstyle, prettyfication

[KII Keiichi]

Hi Satyam,

> From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> 
> [1/9] netconsole: Cleanups, codingstyle, prettyfication
> 
> (1) Remove unwanted headers.
> (2) Mark __init and __exit as appropriate.
> (3) Various trivial codingstyle and prettification stuff.
> 
> Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>
> 
Signed-off-by: Keiichi Kii <k-keiichi@bx.jp.nec.com>

Thanks
-- 
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com 

Re: [PATCH v2 -mm 2/9] netconsole: Remove bogus check

[KII Keiichi]

Hi Satyam,

> From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> 
> [2/9] netconsole: Remove bogus check
> 
> The (!np.dev) check in write_msg() is bogus (always false), because:
> np.dev is set by netpoll_setup(), which is called by the target init
> code in init_netconsole() _before_ register_console() => write_msg() cannot
> be triggered unless netpoll_setup() returns with success. And that will not
> happen if netpoll_setup() failed to set np.dev. Also np.dev cannot go from
> under us while netconsole is loaded. This is because netpoll_setup() grabs
> a reference for us on that dev. So let's remove the pointless check.
> 
> Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>
> 
Acked-by: Keiichi Kii <k-keiichi@bx.jp.nec.com>

Thanks
-- 
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com 

Re: [PATCH v2 -mm 3/9] netconsole: Simplify boot/module option setup logic

[KII Keiichi]

Hi Satyam,

> From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> 
> [3/9] netconsole: Simplify boot/module option setup logic
> 
> Presently, for built-in netconsole:
> 
> __setup(..., option_setup) ensures that the option_setup() function is
> called at boot-time from obsolete_checksetup() with the string matching
> "netconsole=" passed to it from the kernel's command line. We call the
> netpoll_parse_options() from in there, and populate the netpoll struct
> with the passed values. Then, when init_netconsole() is called during the
> initcall phase, strlen(config) fails, thus skipping option_setup().
> 
> For modular netconsole:
> 
> module_param_string() ensures that the string corresponding to the
> "netconsole" module parameter passed from the modprobe command line is
> copied into the "config" static variable. This time, when the module is
> being initialized, strlen(config) is true and so option_setup() is called
> on "config" from init_netconsole() and the input string is parsed and the
> netpoll struct populated.
> 
> Hence, quite different things happen in the copying and parsing of the
> passed netpoll parameters for the modular / built-in cases. This patch
> makes both of them similar by doing exactly the equivalent of a
> module_param_string() in option_setup() also -- just copying the param
> string passed from the kernel command line into the "config" static
> variable. So, init_netconsole() parses (using netpoll_parse_options())
> it in both the cases, and makes the logic somewhat simpler.
> 
> Now, option_setup() is only ever called / used for the built-in case,
> so we put it inside a #ifndef MODULE, otherwise gcc will complain about
> option_setup() being "defined but not used".
> 
> Also, the "configured" variable is redundant with this patch and so removed.
> 
> Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>
> 
Signed-off-by: Keiichi Kii <k-keiichi@bx.jp.nec.com>

Thanks
-- 
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com 

Re: [PATCH v2 -mm 5/9] netconsole: Introduce netconsole_target

[KII Keiichi]

Hi Satyam,

> From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> 
> [5/9] netconsole: Introduce netconsole_target
> 
> Introduce a wrapper structure over netpoll to represent logging targets
> configured in netconsole. This will get extended with other members in
> further patches.
> 
> The original patchset did this along with (and inside the #ifdef) of
> CONFIG_NETCONSOLE_DYNAMIC itself. I decided otherwise, and was able to
> drastically cut down on the #ifdef-complexity of final netconsole.c.
> Also, struct netconsole_target would be required for multiple targets
> support also, and not just dynamic reconfigurability. Previously these
> two things were coupled, but I want to de-link that (more on this later).
> 
> Note that this patch in itself looks quite redundant / stupid, but it is
> purposefully made this way, so further patches are more readable.
> 
> Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>
> 
Signed-off-by: Keiichi Kii <k-keiichi@bx.jp.nec.com>

Thanks
--
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com

Re: [PATCH v2 -mm 6/9] netconsole: Introduce netconsole_netdev_notifier

[KII Keiichi]

Hi Satyam,

> From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> 
> [6/9] netconsole: Introduce netconsole_netdev_notifier
> 
> To update fields of underlying netpoll structure at runtime on
> corresponding NETDEV_CHANGEADDR or NETDEV_CHANGENAME notifications.
> 
> ioctl(SIOCSIFHWADDR) {or ioctl(SIOCSIFNAME)} could be used to change the
> hardware/MAC address {or name} of the local interface that our netpoll is
> attached to. Whenever this happens, netdev notifier chain is called out
> with the NETDEV_CHANGEADDR {or NETDEV_CHANGENAME} event message. We respond
> to that and update the local_mac {or dev_name} field of the struct netpoll.
> This makes sense anyway, but is especially required for dynamic netconsole
> because the netpoll structure's internal members become user visible files
> when either sysfs or configfs are used. So this helps us to keep up with the
> MAC address / name changes and keep the values in struct netpoll uptodate.
> 
> Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>
> 
Signed-off-by: Keiichi Kii <k-keiichi@bx.jp.nec.com>

Thanks
--
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com

Re: [PATCH v2 -mm 7/9] netconsole: Use netif_running() in write_msg()

[KII Keiichi]

Hi Satyam,

> From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> 
> [7/9] netconsole: Use netif_running() in write_msg()
> 
> Avoid unnecessarily disabling interrupts and calling netpoll_send_udp()
> if the corresponding local interface is not up.
> 
> Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>
> 
Acked-by: Keiichi Kii <k-keiichi@bx.jp.nec.com>

Thanks
--
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com

Re: [PATCH v2 -mm 8/9] netconsole: Support multiple logging targets

[KII Keiichi]

Hi Satyam,

> From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> 
> [8/9] netconsole: Support multiple logging targets
> 
> This patch introduces support for multiple targets:
> 
> Let's keep this out of CONFIG_NETCONSOLE_DYNAMIC as well -- this is useful
> even in the default case and (including the infrastructure introduced in
> previous patches) doesn't really add too many bytes to module text. All the
> complexity (and size) comes with the dynamic reconfigurability / userspace
> interface patch, and so it's plausible users may want to keep this enabled
> but that disabled (say to avoid a dep on CONFIG_CONFIGFS_FS too).
> 
> Also update documentation to mention the use of ";" separator to specify
> multiple logging targets in the boot/module option string.
> 
> Brief overview:
> 
> We maintain a target_list (and corresponding lock). Get rid of the static
> "default_target" and introduce allocation and release functions for our
> netconsole_target objects (but keeping sure to preserve previous behaviour
> such as default values). During init_netconsole(), ";" is used as the
> separator to identify multiple target specifications in the boot/module
> option string. The target specifications are parsed and netpolls setup.
> During exit, the target_list is torn down and all items released.
> 
> Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>
> 
Signed-off-by: Keiichi Kii <k-keiichi@bx.jp.nec.com>

Thanks
--
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com

Re: [PATCH v2 -mm 9/9] netconsole: Support dynamic reconfiguration using configfs

[KII Keiichi]

Hi Satyam,

> From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> 
> [9/9] netconsole: Support dynamic reconfiguration using configfs
> 
> This patch introduces support for dynamic reconfiguration (adding, removing
> and/or modifying parameters of netconsole targets at runtime) using a
> userspace interface exported via configfs. Documentation is also updated
> accordingly.
> 
> Issues and brief design overview:
> 
> (1) Kernel-initiated creation / destruction of kernel objects is not
> possible with configfs -- the lifetimes of the "config items" is managed
> exclusively from userspace. But netconsole must support boot/module params
> too, and these are parsed in kernel and hence netpolls must be setup from
> the kernel. Joel Becker suggested to separately manage the lifetimes of
> the two kinds of netconsole_target objects -- those created via configfs
> mkdir(2) from userspace and those specified from the boot/module option
> string. This adds complexity and some redundancy here and also means that
> boot/module param-created targets are not exposed through the configfs
> namespace (and hence cannot be updated / destroyed dynamically). However,
> this saves us from locking / refcounting complexities that would need to
> be introduced in configfs to support kernel-initiated item creation /
> destroy there. Also, this is similar to present behaviour in any case so
> not really a problem.
> 
> (2) In configfs, item creation takes place in the call chain of the mkdir(2)
> syscall in the driver subsystem. If we used an ioctl(2) to create / destroy
> objects from userspace, the special userspace program is able to fill out
> the structure to be passed into the ioctl and hence specify attributes such
> as local interface that are required at the time we set up the netpoll.
> For configfs, this information is not available at the time of mkdir(2).
> So, we keep all newly-created targets (via configfs) disabled by default.
> The user is expected to set various attributes appropriately (including the
> local network interface if required) and then write(2) "1" to the "enabled"
> attribute. Thus, netpoll_setup() is then called on the set parameters in the
> context of _this_ write(2) on the "enabled" attribute itself. This design
> enables the user to reconfigure existing netconsole targets at runtime to
> be attached to newly-come-up interfaces that may not have existed when
> netconsole was loaded or when the targets were actually created. This all
> enables us to get rid of custom ioctls.
> 
> (3) Ultra-paranoid configfs attribute show() and store() operations, with
> sanity and input range checking, using only safe string primitives, and
> compliant with the recommendations in Documentation/filesystems/sysfs.txt.
> 
> (4) A new function netpoll_print_options() is created in the netpoll API,
> that just prints out the configured parameters for a netpoll structure.
> netpoll_parse_options() is modified to use that and it is also exported to
> be used from netconsole.
> 
> Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>
> 
Acked-by: Keiichi Kii <k-keiichi@bx.jp.nec.com>

Thanks
--
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com

Re: [RFC][PATCH v2 -mm 0/9] netconsole: Multiple targets and dynamic reconfigurability

[Satyam Sharma]

Hi Keiichi,

On Fri, 13 Jul 2007, KII Keiichi wrote:
> Hi Satyam,
> 
> > [0/9] netconsole: Multiple targets and dynamic reconfigurability
> > 
> > This patchset is a rework of the original idea and patches posted by
> > Keiichi Kii and Takayoshi Kochi at: http://lkml.org/lkml/2007/6/13/72
> > 
> > This is v2 of the patchset, the previous version is available at:
> > http://lkml.org/lkml/2007/7/4/107
> > 
> > This is diffed against 2.6.22-rc6-mm1 + the earlier netpoll fixlet and the
> > configfs cleanup patches (those are already merged in -mm AFAIK, and hence
> > not reproduced here).
> > 
> > [1/9] netconsole: Cleanups, codingstyle, prettyfication
> > [2/9] netconsole: Remove bogus check
> > [3/9] netconsole: Simplify boot/module option setup logic
> > [4/9] netconsole: Add some useful tips to documentation
> > [5/9] netconsole: Introduce netconsole_target
> > [6/9] netconsole: Introduce netconsole_netdev_notifier
> > [7/9] netconsole: Use netif_running() in write_msg()
> > [8/9] netconsole: Support multiple logging targets
> > [9/9] netconsole: Support dynamic reconfiguration using configfs
> > 
> 
> I tested your v2 patches on the x86/IA64 architecture.
> There was no problem on my tests.
> 
> Signed-off-by: Keiichi Kii <k-keiichi@bx.jp.nec.com>

Ok, thanks! I'll send out the next series (with the various few
fixes and cleanups suggested this time) with your Sign-offs/Acks.

Satyam