[RFC][PATCH 2.6.19 0/6] proposal for dynamic configurable netconsole

[RFC][PATCH 2.6.19 0/6] proposal for dynamic configurable netconsole

[Keiichi KII]

From: Keiichi KII <k-keiichi@bx.jp.nec.com>

The netconsole is a very useful module for collecting kernel message under
certain circumstances(e.g. disk logging fails, serial port is unavailable).

But 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 reload netconsole module.

So, I propose the following extended features for netconsole.

1) support for multiple logging agents.
2) add interface to access each parameter of netconsole
   using sysfs.

This patch is for linux-2.6.19 and is divided to each function.
Your comments are very welcome.

Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
---
-- 
Keiichi KII
NEC Corporation OSS Promotion Center
E-mail: k-keiichi@bx.jp.nec.com

[RFC][PATCH 2.6.19 1/6] cleanup for netconsole

[Keiichi KII]

From: Keiichi KII <k-keiichi@bx.jp.nec.com>

This patch contains the following cleanups.
 - add __init for initialization functions(option_setup() and init_netconsole()).
 - define name of magic number.

Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
---
--- linux-2.6.19/drivers/net/netconsole.c	2006-12-06 14:37:06.985584500 +0900
+++ enhanced-netconsole/drivers/net/netconsole.c.cleanup	2006-12-06
14:34:52.561183500 +0900
@@ -50,8 +50,14 @@ MODULE_AUTHOR("Maintainer: Matt Mackall
 MODULE_DESCRIPTION("Console driver for network interfaces");
 MODULE_LICENSE("GPL");

-static char config[256];
-module_param_string(netconsole, config, 256, 0);
+enum {
+	MAX_PRINT_CHUNK = 1000,
+	MAX_CONFIG_LENGTH = 256,
+};
+
+static char config[MAX_CONFIG_LENGTH];
+
+module_param_string(netconsole, config, MAX_CONFIG_LENGTH, 0);
 MODULE_PARM_DESC(netconsole, "
netconsole=[src-port]@[src-ip]/[dev],[tgt-port]@<tgt-ip>/[tgt-macaddr]\n");

 static struct netpoll np = {
@@ -62,9 +68,8 @@ static struct netpoll np = {
 	.remote_mac = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff},
 	.drop = netpoll_queue,
 };
-static int configured = 0;

-#define MAX_PRINT_CHUNK 1000
+static int configured = 0;

 static void write_msg(struct console *con, const char *msg, unsigned int len)
 {
@@ -75,14 +80,12 @@ static void write_msg(struct console *co
 		return;

 	local_irq_save(flags);
-
 	for(left = len; left; ) {
 		frag = min(left, MAX_PRINT_CHUNK);
 		netpoll_send_udp(&np, msg, frag);
 		msg += frag;
 		left -= frag;
 	}
-
 	local_irq_restore(flags);
 }

@@ -92,7 +95,7 @@ static struct console netconsole = {
 	.write = write_msg
 };

-static int option_setup(char *opt)
+static int __init option_setup(char *opt)
 {
 	configured = !netpoll_parse_options(&np, opt);
 	return 1;
@@ -100,7 +103,7 @@ static int option_setup(char *opt)

 __setup("netconsole=", option_setup);

-static int init_netconsole(void)
+static int __init init_netconsole(void)
 {
 	if(strlen(config))
 		option_setup(config);

-- 
Keiichi KII
NEC Corporation OSS Promotion Center
E-mail: k-keiichi@bx.jp.nec.com

Re: [RFC][PATCH 2.6.19 1/6] cleanup for netconsole

[Matt Mackall]

On Tue, Dec 12, 2006 at 03:28:17PM +0900, Keiichi KII wrote:
> From: Keiichi KII <k-keiichi@bx.jp.nec.com>
> 
> This patch contains the following cleanups.
>  - add __init for initialization functions(option_setup() and init_netconsole()).
>  - define name of magic number.
> 
> Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
> ---
> --- linux-2.6.19/drivers/net/netconsole.c	2006-12-06 14:37:06.985584500 +0900
> +++ enhanced-netconsole/drivers/net/netconsole.c.cleanup	2006-12-06
> 14:34:52.561183500 +0900
> @@ -50,8 +50,14 @@ MODULE_AUTHOR("Maintainer: Matt Mackall
>  MODULE_DESCRIPTION("Console driver for network interfaces");
>  MODULE_LICENSE("GPL");
> 
> -static char config[256];
> -module_param_string(netconsole, config, 256, 0);
> +enum {
> +	MAX_PRINT_CHUNK = 1000,
> +	MAX_CONFIG_LENGTH = 256,
> +};

Converting defines to anonymous enums is generally not considered a net
improvement around here. It doesn't provide any improvement in type safety.

> 
>  static void write_msg(struct console *con, const char *msg, unsigned int len)
>  {
> @@ -75,14 +80,12 @@ static void write_msg(struct console *co
>  		return;
> 
>  	local_irq_save(flags);
> -
>  	for(left = len; left; ) {
>  		frag = min(left, MAX_PRINT_CHUNK);
>  		netpoll_send_udp(&np, msg, frag);
>  		msg += frag;
>  		left -= frag;
>  	}
> -
>  	local_irq_restore(flags);
>  }

It's not a good idea to mix unrelated changes in, especially if
they're gratuitous formatting changes.

> +static int __init init_netconsole(void)
>  {
>  	if(strlen(config))
>  		option_setup(config);

I seem to recall there was a reason for not marking that __init. It
may no longer apply.

-- 
Mathematics is the supreme nostalgia of our time.

[RFC][PATCH 2.6.19 3/6] add interface for netconsole using sysfs

[Keiichi KII]

From: Keiichi KII <k-keiichi@bx.jp.nec.com>

This patch contains the following changes.

create a sysfs entry for netconsole in /sys/class/misc.
This entry has elements related to netconsole as follows.
You can change configuration of netconsole(writable attributes such as IP
address, port number and so on) and check current configuration of netconsole.

-+- /sys/class/misc/
 |-+- netconsole/
   |-+- port1/
   | |--- id          [r--r--r--]  unique port id
   | |--- remove      [-w-------]  if you write something to "remove",
   | |                             this port is removed.
   | |--- dev_name    [r--r--r--]  network interface name
   | |--- local_ip    [rw-r--r--]  source IP to use, writable
   | |--- local_port  [rw-r--r--]  source port number for UDP packets, writable
   | |--- local_mac   [r--r--r--]  source MAC address
   | |--- remote_ip   [rw-r--r--]  port number for logging agent, writable
   | |--- remote_port [rw-r--r--]  IP address for logging agent, writable
   | ---- remote_mac  [rw-r--r--]  MAC address for logging agent, writable
   |--- port2/
   |--- port3/
   ...

Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
---
--- linux-2.6.19/drivers/net/netconsole.c	2006-12-06 14:37:26.842825500 +0900
+++ enhanced-netconsole/drivers/net/netconsole.c.sysfs	2006-12-06
13:32:47.488381000 +0900
@@ -45,6 +45,8 @@
 #include <linux/sysrq.h>
 #include <linux/smp.h>
 #include <linux/netpoll.h>
+#include <linux/miscdevice.h>
+#include <linux/inet.h>

 MODULE_AUTHOR("Maintainer: Matt Mackall <mpm@selenic.com>");
 MODULE_DESCRIPTION("Console driver for network interfaces");
@@ -53,6 +55,7 @@ MODULE_LICENSE("GPL");
 enum {
 	MAX_PRINT_CHUNK = 1000,
 	MAX_CONFIG_LENGTH = 256,
+	MAC_ADDR_DIGIT = 6,
 };

 static char config[MAX_CONFIG_LENGTH];
@@ -62,19 +65,214 @@ MODULE_PARM_DESC(netconsole, " netconsol

 struct netconsole_device {
 	struct list_head list;
+	struct kobject obj;
 	spinlock_t netpoll_lock;
 	int id;
 	struct netpoll np;
 };

+struct netcon_dev_attr {
+	struct attribute attr;
+	ssize_t (*show)(struct netconsole_device*, char*);
+	ssize_t (*store)(struct netconsole_device*, const char*,
+			 size_t count);
+};
+
 static int add_netcon_dev(const char*);
+static void setup_netcon_dev_sysfs(struct netconsole_device*);
 static void cleanup_netconsole(void);
 static void netcon_dev_cleanup(struct netconsole_device *nd);

+static int netcon_miscdev_configured = 0;
+
 static LIST_HEAD(active_netconsole_dev);

 static DEFINE_SPINLOCK(netconsole_dev_list_lock);

+#define SHOW_CLASS_ATTR(field, type, format, ...) \
+static ssize_t show_##field(type, char *buf) \
+{ \
+     return sprintf(buf, format, __VA_ARGS__); \
+} \
+
+SHOW_CLASS_ATTR(id, struct netconsole_device *nd, "%d\n", nd->id);
+SHOW_CLASS_ATTR(dev_name, struct netconsole_device *nd,
+		"%s\n", nd->np.dev_name);
+SHOW_CLASS_ATTR(local_port, struct netconsole_device *nd,
+		"%d\n", nd->np.local_port);
+SHOW_CLASS_ATTR(remote_port, struct netconsole_device *nd,
+		"%d\n", nd->np.remote_port);
+SHOW_CLASS_ATTR(local_ip, struct netconsole_device *nd,
+		"%d.%d.%d.%d\n", HIPQUAD(nd->np.local_ip));
+SHOW_CLASS_ATTR(remote_ip, struct netconsole_device *nd,
+		"%d.%d.%d.%d\n", HIPQUAD(nd->np.remote_ip));
+SHOW_CLASS_ATTR(local_mac, struct netconsole_device *nd,
+		"%02x:%02x:%02x:%02x:%02x:%02x\n",
+		nd->np.local_mac[0], nd->np.local_mac[1], nd->np.local_mac[2],
+		nd->np.local_mac[3], nd->np.local_mac[4], nd->np.local_mac[5]);
+SHOW_CLASS_ATTR(remote_mac, struct netconsole_device *nd,
+		"%02x:%02x:%02x:%02x:%02x:%02x\n",
+		nd->np.remote_mac[0], nd->np.remote_mac[1],
+		nd->np.remote_mac[2], nd->np.remote_mac[3],
+		nd->np.remote_mac[4], nd->np.remote_mac[5]);
+
+static ssize_t store_local_port(struct netconsole_device *nd, const char *buf,
+				size_t count)
+{
+	spin_lock(&nd->netpoll_lock);
+	nd->np.local_port = simple_strtol(buf, NULL, 10);
+	spin_unlock(&nd->netpoll_lock);
+
+	return count;
+}
+
+static ssize_t store_remote_port(struct netconsole_device *nd, const char *buf,
+				size_t count)
+{
+	spin_lock(&nd->netpoll_lock);
+	nd->np.remote_port = simple_strtol(buf, NULL, 10);
+	spin_unlock(&nd->netpoll_lock);
+
+	return count;
+}
+
+static ssize_t store_local_ip(struct netconsole_device *nd, const char *buf,
+			      size_t count)
+{
+	spin_lock(&nd->netpoll_lock);
+	nd->np.local_ip = ntohl(in_aton(buf));
+	spin_unlock(&nd->netpoll_lock);
+
+	return count;
+}
+
+static ssize_t store_remote_ip(struct netconsole_device *nd, const char *buf,
+			       size_t count)
+{
+	spin_lock(&nd->netpoll_lock);
+	nd->np.remote_ip = ntohl(in_aton(buf));
+	spin_unlock(&nd->netpoll_lock);
+
+	return count;
+}
+
+static ssize_t store_remote_mac(struct netconsole_device *nd, const char *buf,
+			       size_t count)
+{
+	unsigned char input_mac[MAC_ADDR_DIGIT] =
+		{0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+	const char *cur=buf;
+	char *delim;
+	int i = 0;
+
+	for(i=0; i < MAC_ADDR_DIGIT; i++) {
+		input_mac[i] = simple_strtol(cur, NULL, 16);
+		if (i != MAC_ADDR_DIGIT - 1 &&
+		    (delim = strchr(cur, ':')) == NULL) {
+			return -EINVAL;
+		}
+		cur=delim+1;
+	}
+	spin_lock(&nd->netpoll_lock);
+	memcpy(nd->np.remote_mac, input_mac, MAC_ADDR_DIGIT);
+	spin_unlock(&nd->netpoll_lock);
+
+	return count;
+}
+
+static ssize_t store_remove(struct netconsole_device *nd, const char *buf,
+			    size_t count)
+{
+	kobject_unregister(&nd->obj);
+	return count;
+}
+
+#define NETCON_CLASS_ATTR(_name, _mode, _show, _store) \
+struct netcon_dev_attr netcon_dev_attr_##_name = \
+                           __ATTR(_name, _mode, _show, _store)
+
+static NETCON_CLASS_ATTR(id, S_IRUGO, show_id, NULL);
+static NETCON_CLASS_ATTR(dev_name, S_IRUGO, show_dev_name, NULL);
+static NETCON_CLASS_ATTR(local_port, S_IRUGO | S_IWUSR,
+			 show_local_port, store_local_port);
+static NETCON_CLASS_ATTR(remote_port, S_IRUGO | S_IWUSR,
+			 show_remote_port, store_remote_port);
+static NETCON_CLASS_ATTR(local_ip, S_IRUGO | S_IWUSR,
+			 show_local_ip, store_local_ip);
+static NETCON_CLASS_ATTR(remote_ip, S_IRUGO | S_IWUSR,
+			 show_remote_ip, store_remote_ip);
+static NETCON_CLASS_ATTR(local_mac, S_IRUGO, show_local_mac, NULL);
+static NETCON_CLASS_ATTR(remote_mac, S_IRUGO | S_IWUSR,
+			 show_remote_mac, store_remote_mac);
+static NETCON_CLASS_ATTR(remove, S_IWUSR, NULL, store_remove);
+
+static struct attribute *netcon_dev_attrs[] = {
+	&netcon_dev_attr_id.attr,
+	&netcon_dev_attr_dev_name.attr,
+	&netcon_dev_attr_local_port.attr,
+	&netcon_dev_attr_remote_port.attr,
+	&netcon_dev_attr_local_ip.attr,
+	&netcon_dev_attr_remote_ip.attr,
+	&netcon_dev_attr_local_mac.attr,
+	&netcon_dev_attr_remote_mac.attr,
+	&netcon_dev_attr_remove.attr,
+	NULL
+};
+
+static ssize_t show_netcon_dev_attr(struct kobject *kobj,
+				    struct attribute *attr,
+				    char *buffer)
+{
+	struct netcon_dev_attr *na = container_of(attr, struct netcon_dev_attr,
+						  attr);
+	struct netconsole_device * nd =
+		container_of(kobj, struct netconsole_device, obj);
+	if (na->show) {
+		return na->show(nd, buffer);
+	} else {
+		return -EACCES;
+	}
+}
+
+static ssize_t store_netcon_dev_attr(struct kobject *kobj,
+				     struct attribute *attr,
+				     const char *buffer, size_t count)
+{
+	struct netcon_dev_attr *na = container_of(attr, struct netcon_dev_attr,
+						  attr);
+	struct netconsole_device *nd =
+		container_of(kobj, struct netconsole_device, obj);
+	if (na->store) {
+		return na->store(nd, buffer, count);
+	} else {
+		return -EACCES;
+	}
+}
+
+static void netcon_dev_release(struct kobject *kobj)
+{
+	struct netconsole_device *nd =
+		container_of(kobj, struct netconsole_device, obj);
+
+	netcon_dev_cleanup(nd);
+}
+
+static struct sysfs_ops netcon_dev_sysfs_ops = {
+	.show = show_netcon_dev_attr,
+	.store = store_netcon_dev_attr
+};
+
+static struct kobj_type netcon_dev_ktype = {
+	.release = netcon_dev_release,
+	.sysfs_ops = &netcon_dev_sysfs_ops,
+	.default_attrs = netcon_dev_attrs,
+};
+
+static struct miscdevice netcon_miscdev = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "netconsole",
+};
+
 static struct netpoll np = {
 	.name = "netconsole",
 	.dev_name = "eth0",
@@ -129,6 +327,8 @@ static int add_netcon_dev(const char* ta
 	list_add(&new_dev->list, &active_netconsole_dev);
 	spin_unlock(&netconsole_dev_list_lock);

+	setup_netcon_dev_sysfs(new_dev);
+
 	kfree(netcon_dev_config);
 	return 0;

@@ -138,6 +338,14 @@ static int add_netcon_dev(const char* ta
 	return -EINVAL;
 }

+static void setup_netcon_dev_sysfs(struct netconsole_device *target)
+{
+	kobject_set_name(&target->obj, "port%d", target->id);
+	target->obj.parent = &netcon_miscdev.class->kobj;
+	target->obj.ktype = &netcon_dev_ktype;
+	kobject_register(&target->obj);
+}
+
 static void netcon_dev_cleanup(struct netconsole_device *nd)
 {
 	spin_lock(&netconsole_dev_list_lock);
@@ -191,6 +399,13 @@ static int __init init_netconsole(void)
 	char *p;
 	char *tmp = config;

+	if (misc_register(&netcon_miscdev)) {
+		printk(KERN_INFO
+		       "netconsole: unable netconsole misc device\n");
+		return -EIO;
+	} else {
+		netcon_miscdev_configured = 1;
+	}
 	register_console(&netconsole);

 	if(!strlen(config)) {
@@ -215,8 +430,13 @@ static void cleanup_netconsole(void)
 	struct netconsole_device *dev, *tmp;

 	unregister_console(&netconsole);
+
 	list_for_each_entry_safe(dev, tmp, &active_netconsole_dev, list) {
-		netcon_dev_cleanup(dev);
+		kobject_unregister(&dev->obj);
+	}
+
+	if (netcon_miscdev_configured) {
+		misc_deregister(&netcon_miscdev);
 	}
 }

-- 
Keiichi KII
NEC Corporation OSS Promotion Center
E-mail: k-keiichi@bx.jp.nec.com

Re: [RFC][PATCH 2.6.19 3/6] add interface for netconsole using sysfs

[Matt Mackall]

On Tue, Dec 12, 2006 at 03:34:54PM +0900, Keiichi KII wrote:
> From: Keiichi KII <k-keiichi@bx.jp.nec.com>
> 
> This patch contains the following changes.
> 
> create a sysfs entry for netconsole in /sys/class/misc.
> This entry has elements related to netconsole as follows.
> You can change configuration of netconsole(writable attributes such as IP
> address, port number and so on) and check current configuration of netconsole.
> 
> -+- /sys/class/misc/
>  |-+- netconsole/
>    |-+- port1/
>    | |--- id          [r--r--r--]  unique port id
>    | |--- remove      [-w-------]  if you write something to "remove",
>    | |                             this port is removed.
>    | |--- dev_name    [r--r--r--]  network interface name
>    | |--- local_ip    [rw-r--r--]  source IP to use, writable
>    | |--- local_port  [rw-r--r--]  source port number for UDP packets, writable
>    | |--- local_mac   [r--r--r--]  source MAC address
>    | |--- remote_ip   [rw-r--r--]  port number for logging agent, writable
>    | |--- remote_port [rw-r--r--]  IP address for logging agent, writable
>    | ---- remote_mac  [rw-r--r--]  MAC address for logging agent, writable
>    |--- port2/
>    |--- port3/
>    ...
> 
> Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
> ---
> --- linux-2.6.19/drivers/net/netconsole.c	2006-12-06 14:37:26.842825500 +0900
> +++ enhanced-netconsole/drivers/net/netconsole.c.sysfs	2006-12-06
> 13:32:47.488381000 +0900
> @@ -45,6 +45,8 @@
>  #include <linux/sysrq.h>
>  #include <linux/smp.h>
>  #include <linux/netpoll.h>
> +#include <linux/miscdevice.h>
> +#include <linux/inet.h>
> 
>  MODULE_AUTHOR("Maintainer: Matt Mackall <mpm@selenic.com>");
>  MODULE_DESCRIPTION("Console driver for network interfaces");
> @@ -53,6 +55,7 @@ MODULE_LICENSE("GPL");
>  enum {
>  	MAX_PRINT_CHUNK = 1000,
>  	MAX_CONFIG_LENGTH = 256,
> +	MAC_ADDR_DIGIT = 6,
>  };
> 
>  static char config[MAX_CONFIG_LENGTH];
> @@ -62,19 +65,214 @@ MODULE_PARM_DESC(netconsole, " netconsol
> 
>  struct netconsole_device {
>  	struct list_head list;
> +	struct kobject obj;
>  	spinlock_t netpoll_lock;
>  	int id;
>  	struct netpoll np;
>  };
> 
> +struct netcon_dev_attr {
> +	struct attribute attr;
> +	ssize_t (*show)(struct netconsole_device*, char*);
> +	ssize_t (*store)(struct netconsole_device*, const char*,
> +			 size_t count);
> +};
> +
>  static int add_netcon_dev(const char*);
> +static void setup_netcon_dev_sysfs(struct netconsole_device*);
>  static void cleanup_netconsole(void);
>  static void netcon_dev_cleanup(struct netconsole_device *nd);
> 
> +static int netcon_miscdev_configured = 0;
> +
>  static LIST_HEAD(active_netconsole_dev);
> 
>  static DEFINE_SPINLOCK(netconsole_dev_list_lock);
> 
> +#define SHOW_CLASS_ATTR(field, type, format, ...) \
> +static ssize_t show_##field(type, char *buf) \
> +{ \
> +     return sprintf(buf, format, __VA_ARGS__); \
> +} \

I see this sort of thing is pretty common with sysfs stuff.. but yuck.

> +static ssize_t show_netcon_dev_attr(struct kobject *kobj,
> +				    struct attribute *attr,
> +				    char *buffer)
> +{
> +	struct netcon_dev_attr *na = container_of(attr, struct netcon_dev_attr,
> +						  attr);
> +	struct netconsole_device * nd =
> +		container_of(kobj, struct netconsole_device, obj);
> +	if (na->show) {
> +		return na->show(nd, buffer);
> +	} else {
> +		return -EACCES;
> +	}

Kernel style is to skip the braces for single statement clauses.

> +	if (misc_register(&netcon_miscdev)) {
> +		printk(KERN_INFO
> +		       "netconsole: unable netconsole misc device\n");

This error message seems to be missing a word or two.

-- 
Mathematics is the supreme nostalgia of our time.

[RFC][PATCH 2.6.19 4/6] switch function of netpoll

[Keiichi KII]

From: Keiichi KII <k-keiichi@bx.jp.nec.com>

This patch contains switch function of netpoll.

if "enable" attribute of certain port is '1', this port is used.
if "enable" attribute of certain port is '0', this port isn't used.

active_netconsole_dev list manages a list of active ports.
inactive_netconsole_dev list manages a list of inactive ports.

If you write '0' to "enable" attribute of a port included in
active_netconsole_dev_list, This port moves from active_netconsole_dev to
inactive_netconsole_dev and won't used to send kernel message.

-+- /sys/class/misc/
 |-+- netconsole/
   |-+- port1/
   | |--- id          [r--r--r--]  id
   | |--- enable      [rw-r--r--]  0: disable, 1: enable, writable
   | ...
   |--- port2/
   ...

Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
---
--- linux-2.6.19/drivers/net/netconsole.c	2006-12-06 14:37:26.858826500 +0900
+++ enhanced-netconsole/drivers/net/netconsole.c.switch	2006-12-06
13:32:56.744959500 +0900
@@ -86,9 +86,25 @@ static void netcon_dev_cleanup(struct ne
 static int netcon_miscdev_configured = 0;

 static LIST_HEAD(active_netconsole_dev);
+static LIST_HEAD(inactive_netconsole_dev);

 static DEFINE_SPINLOCK(netconsole_dev_list_lock);

+static ssize_t show_enable(struct netconsole_device *nd, char *buf) {
+	struct netconsole_device *dev;
+
+	spin_lock(&netconsole_dev_list_lock);
+	list_for_each_entry(dev, &active_netconsole_dev, list) {
+		if (dev->id == nd->id) {
+			spin_unlock(&netconsole_dev_list_lock);
+			return sprintf(buf, "1\n");
+		}
+	}
+	spin_unlock(&netconsole_dev_list_lock);
+
+	return sprintf(buf, "0\n");
+}
+
 #define SHOW_CLASS_ATTR(field, type, format, ...) \
 static ssize_t show_##field(type, char *buf) \
 { \
@@ -180,6 +196,36 @@ static ssize_t store_remote_mac(struct n
 	return count;
 }

+static ssize_t store_enable(struct netconsole_device *nd, const char *buf,
+			    size_t count)
+{
+	struct netconsole_device *dev, *tmp;
+	struct list_head *src, *dst;
+
+	if (strncmp(buf, "1", 1) == 0) {
+		src = &inactive_netconsole_dev;
+		dst = &active_netconsole_dev;
+	} else if(strncmp(buf, "0", 1) == 0) {
+		src = &active_netconsole_dev;
+		dst = &inactive_netconsole_dev;
+	} else {
+		printk(KERN_INFO "netconsole: invalid argument: %s\n", buf);
+		return -EINVAL;
+	}
+	
+	spin_lock(&netconsole_dev_list_lock);
+	list_for_each_entry_safe(dev, tmp, src, list) {
+		if (dev->id == nd->id) {
+			list_del(&nd->list);
+			list_add(&nd->list, dst);
+			break;
+		}
+	}
+	spin_unlock(&netconsole_dev_list_lock);
+
+	return count;
+}
+
 static ssize_t store_remove(struct netconsole_device *nd, const char *buf,
 			    size_t count)
 {
@@ -204,6 +250,7 @@ static NETCON_CLASS_ATTR(remote_ip, S_IR
 static NETCON_CLASS_ATTR(local_mac, S_IRUGO, show_local_mac, NULL);
 static NETCON_CLASS_ATTR(remote_mac, S_IRUGO | S_IWUSR,
 			 show_remote_mac, store_remote_mac);
+static NETCON_CLASS_ATTR(enable, S_IRUGO | S_IWUSR, show_enable, store_enable);
 static NETCON_CLASS_ATTR(remove, S_IWUSR, NULL, store_remove);

 static struct attribute *netcon_dev_attrs[] = {
@@ -215,6 +262,7 @@ static struct attribute *netcon_dev_attr
 	&netcon_dev_attr_remote_ip.attr,
 	&netcon_dev_attr_local_mac.attr,
 	&netcon_dev_attr_remote_mac.attr,
+	&netcon_dev_attr_enable.attr,
 	&netcon_dev_attr_remove.attr,
 	NULL
 };
@@ -434,6 +482,9 @@ static void cleanup_netconsole(void)
 	list_for_each_entry_safe(dev, tmp, &active_netconsole_dev, list) {
 		kobject_unregister(&dev->obj);
 	}
+	list_for_each_entry_safe(dev, tmp, &inactive_netconsole_dev, list) {
+		kobject_unregister(&dev->obj);
+	}

 	if (netcon_miscdev_configured) {
 		misc_deregister(&netcon_miscdev);

-- 
Keiichi KII
NEC Corporation OSS Promotion Center
E-mail: k-keiichi@bx.jp.nec.com

Re: [RFC][PATCH 2.6.19 4/6] switch function of netpoll

[Matt Mackall]

On Tue, Dec 12, 2006 at 03:36:00PM +0900, Keiichi KII wrote:
> From: Keiichi KII <k-keiichi@bx.jp.nec.com>
> 
> This patch contains switch function of netpoll.
> 
> if "enable" attribute of certain port is '1', this port is used.
> if "enable" attribute of certain port is '0', this port isn't used.
> 
> active_netconsole_dev list manages a list of active ports.
> inactive_netconsole_dev list manages a list of inactive ports.
> 
> If you write '0' to "enable" attribute of a port included in
> active_netconsole_dev_list, This port moves from active_netconsole_dev to
> inactive_netconsole_dev and won't used to send kernel message.
> 
> -+- /sys/class/misc/
>  |-+- netconsole/
>    |-+- port1/
>    | |--- id          [r--r--r--]  id
>    | |--- enable      [rw-r--r--]  0: disable, 1: enable, writable
>    | ...
>    |--- port2/
>    ...
> 
> Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
> ---
> --- linux-2.6.19/drivers/net/netconsole.c	2006-12-06 14:37:26.858826500 +0900
> +++ enhanced-netconsole/drivers/net/netconsole.c.switch	2006-12-06
> 13:32:56.744959500 +0900
> @@ -86,9 +86,25 @@ static void netcon_dev_cleanup(struct ne
>  static int netcon_miscdev_configured = 0;
> 
>  static LIST_HEAD(active_netconsole_dev);
> +static LIST_HEAD(inactive_netconsole_dev);
> 
>  static DEFINE_SPINLOCK(netconsole_dev_list_lock);
> 
> +static ssize_t show_enable(struct netconsole_device *nd, char *buf) {
> +	struct netconsole_device *dev;
> +
> +	spin_lock(&netconsole_dev_list_lock);
> +	list_for_each_entry(dev, &active_netconsole_dev, list) {
> +		if (dev->id == nd->id) {
> +			spin_unlock(&netconsole_dev_list_lock);
> +			return sprintf(buf, "1\n");
> +		}
> +	}
> +	spin_unlock(&netconsole_dev_list_lock);
> +
> +	return sprintf(buf, "0\n");
> +}

If we intend to have no more than a couple dozen of these, having a
separate list makes things too complicated. Simply add an enabled
field in the struct and skip disabled targets on netconsole writes.
This function becomes 3 lines.

> +static ssize_t store_enable(struct netconsole_device *nd, const char *buf,
> +			    size_t count)
> +{
> +	struct netconsole_device *dev, *tmp;
> +	struct list_head *src, *dst;
> +
> +	if (strncmp(buf, "1", 1) == 0) {
> +		src = &inactive_netconsole_dev;
> +		dst = &active_netconsole_dev;
> +	} else if(strncmp(buf, "0", 1) == 0) {
> +		src = &active_netconsole_dev;
> +		dst = &inactive_netconsole_dev;
> +	} else {
> +		printk(KERN_INFO "netconsole: invalid argument: %s\n", buf);
> +		return -EINVAL;
> +	}
> +	
> +	spin_lock(&netconsole_dev_list_lock);
> +	list_for_each_entry_safe(dev, tmp, src, list) {
> +		if (dev->id == nd->id) {
> +			list_del(&nd->list);
> +			list_add(&nd->list, dst);
> +			break;
> +		}
> +	}
> +	spin_unlock(&netconsole_dev_list_lock);
> +
> +	return count;
> +}

As does this one.

-- 
Mathematics is the supreme nostalgia of our time.

[RFC][PATCH 2.6.19 5/6] add "add" element in /sys/class/misc/netconsole

[Keiichi KII]

From: Keiichi KII <k-keiichi@bx.jp.nec.com>

This patch contains the following changes.

To add port dynamically, create "add" element in /sys/class/misc/netconsole.

ex)
1. echo "eth0" > /sys/clas/misc/netconsole/add
   then the port is added with the default settings.

2. echo "@/eth0,@192.168.0.1/" > /sys/class/misc/netconsole/add
   then the port is added with the settings sending kernel messages
   to 192.168.0.1 using eth0 device.

-+- /sys/class/misc/
 |-+- netconsole/
   |--- add       [-w-------]  If you write parameter(network interface name
   |                           or one config parameter of netconsole), The
   |                            port related its config is added
   |--- port1/
   |--- port2/
   ...

Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
---
--- linux-2.6.19/drivers/net/netconsole.c	2006-12-06 14:37:26.874827500 +0900
+++ enhanced-netconsole/drivers/net/netconsole.c.add	2006-12-06
13:33:05.661516750 +0900
@@ -321,6 +321,50 @@ static struct miscdevice netcon_miscdev
 	.name = "netconsole",
 };

+static ssize_t set_netconmisc_add(struct class_device *cdev, const char *buf,
+				  size_t count)
+{
+	char *target;
+	char *target_param;
+
+	target_param = (char*)kmalloc(count+1, GFP_ATOMIC);
+	if (!target_param) {
+		printk(KERN_INFO "netconsole: kmalloc() failed!\n");
+		return -ENOMEM;
+	}
+
+	strcpy(target_param, buf);
+	if (target_param[count - 1] == '\n') {
+		target_param[count - 1] = '\0';
+	}
+
+	if (dev_get_by_name(target_param)) {
+		printk(KERN_INFO "netconsole: device name = [%s]\n",
+		       target_param);
+		target = (char*)kmalloc(MAX_CONFIG_LENGTH+1, GFP_ATOMIC);
+		if (!target) {
+			printk(KERN_INFO "netconsole: kmalloc() failed!\n");
+			kfree(target_param);
+			return -ENOMEM;
+		}
+		sprintf(target,"@/%s,@/", target_param);
+		add_netcon_dev(target);
+		kfree(target);
+	} else {
+		printk(KERN_INFO "netconsole: config = [%s]\n", target_param);
+		add_netcon_dev(target_param);
+	}
+	kfree(target_param);
+
+	return count;
+}
+
+static CLASS_DEVICE_ATTR(add, S_IWUSR, NULL, set_netconmisc_add);
+
+static struct class_device_attribute *netcon_misc_attr[] = {
+	&class_device_attr_add,
+};
+
 static struct netpoll np = {
 	.name = "netconsole",
 	.dev_name = "eth0",
@@ -446,6 +490,7 @@ static int __init init_netconsole(void)
 {
 	char *p;
 	char *tmp = config;
+	int i = 0;

 	if (misc_register(&netcon_miscdev)) {
 		printk(KERN_INFO
@@ -456,6 +501,11 @@ static int __init init_netconsole(void)
 	}
 	register_console(&netconsole);

+	for(i=0; i < ARRAY_SIZE(netcon_misc_attr); i++) {
+		class_device_create_file(netcon_miscdev.class,
+					 netcon_misc_attr[i]);
+	}
+
 	if(!strlen(config)) {
 		printk(KERN_INFO "netconsole: not configured\n");
 		return 0;

-- 
Keiichi KII
NEC Corporation OSS Promotion Center
E-mail: k-keiichi@bx.jp.nec.com

Re: [RFC][PATCH 2.6.19 5/6] add "add" element in /sys/class/misc/netconsole

[Matt Mackall]

On Tue, Dec 12, 2006 at 03:37:20PM +0900, Keiichi KII wrote:
> From: Keiichi KII <k-keiichi@bx.jp.nec.com>
> 
> This patch contains the following changes.
> 
> To add port dynamically, create "add" element in /sys/class/misc/netconsole.
> 
> ex)
> 1. echo "eth0" > /sys/clas/misc/netconsole/add
>    then the port is added with the default settings.

What are the default settings for target IP address?

> 2. echo "@/eth0,@192.168.0.1/" > /sys/class/misc/netconsole/add
>    then the port is added with the settings sending kernel messages
>    to 192.168.0.1 using eth0 device.
> 
> -+- /sys/class/misc/
>  |-+- netconsole/
>    |--- add       [-w-------]  If you write parameter(network interface name
>    |                           or one config parameter of netconsole), The
>    |                            port related its config is added
>    |--- port1/
>    |--- port2/
>    ...
> 
> Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
> ---
> --- linux-2.6.19/drivers/net/netconsole.c	2006-12-06 14:37:26.874827500 +0900
> +++ enhanced-netconsole/drivers/net/netconsole.c.add	2006-12-06
> 13:33:05.661516750 +0900
> @@ -321,6 +321,50 @@ static struct miscdevice netcon_miscdev
>  	.name = "netconsole",
>  };
> 
> +static ssize_t set_netconmisc_add(struct class_device *cdev, const char *buf,
> +				  size_t count)
> +{
> +	char *target;
> +	char *target_param;
> +
> +	target_param = (char*)kmalloc(count+1, GFP_ATOMIC);

Unnecessary cast.

> +	for(i=0; i < ARRAY_SIZE(netcon_misc_attr); i++) {
> +		class_device_create_file(netcon_miscdev.class,
> +					 netcon_misc_attr[i]);
> +	}
> +

This chunk looks like it goes in an earlier patch.

-- 
Mathematics is the supreme nostalgia of our time.

[RFC][PATCH 2.6.19 6/6] update modification history

[Keiichi KII]

From: Keiichi KII <k-keiichi@bx.jp.nec.com>

Update modification history.

Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
---
--- linux-2.6.19/drivers/net/netconsole.c	2006-12-12 14:57:45.588967500 +0900
+++ enhanced-netconsole/drivers/net/netconsole.c.sign	2006-12-12
14:54:49.541965250 +0900
@@ -15,6 +15,9 @@
  *               generic card hooks
  *               works non-modular
  * 2003-09-07    rewritten with netpoll api
+ * 2006-12-12    add extended features for
+ *               dynamic configurable netconsole
+ *               by Keiichi KII <k-keiichi@bx.jp.nec.com>
  */

 /****************************************************************

-- 
Keiichi KII
NEC Corporation OSS Promotion Center
E-mail: k-keiichi@bx.jp.nec.com

Re: [RFC][PATCH 2.6.19 6/6] update modification history

[Stephen Hemminger]

On Tue, 12 Dec 2006 15:38:42 +0900
Keiichi KII <k-keiichi@bx.jp.nec.com> wrote:

> From: Keiichi KII <k-keiichi@bx.jp.nec.com>
> 
> Update modification history.
> 
> Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
> ---
> --- linux-2.6.19/drivers/net/netconsole.c	2006-12-12 14:57:45.588967500 +0900
> +++ enhanced-netconsole/drivers/net/netconsole.c.sign	2006-12-12
> 14:54:49.541965250 +0900
> @@ -15,6 +15,9 @@
>   *               generic card hooks
>   *               works non-modular
>   * 2003-09-07    rewritten with netpoll api
> + * 2006-12-12    add extended features for
> + *               dynamic configurable netconsole
> + *               by Keiichi KII <k-keiichi@bx.jp.nec.com>
>   */
> 
>  /****************************************************************
> 

We use a version control system now. The history comments in the kernel
source are considered legacy and rarely if ever changed.

If you want to see the revision history use git.


-- 
Stephen Hemminger <shemminger@osdl.org>

Re: [RFC][PATCH 2.6.19 0/6] proposal for dynamic configurable netconsole

[Matt Mackall]

On Tue, Dec 12, 2006 at 03:17:48PM +0900, Keiichi KII wrote:
> From: Keiichi KII <k-keiichi@bx.jp.nec.com>
> 
> The netconsole is a very useful module for collecting kernel message under
> certain circumstances(e.g. disk logging fails, serial port is unavailable).

FYI, there's a robot named Dave Miller who will eventually notice your
email and scold you for not cc:ing it to netdev, claiming that he
doesn't read LKML.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [RFC][PATCH 2.6.19 0/6] proposal for dynamic configurable netconsole

[Keiichi KII]

Matt Mackall wrote:
> On Tue, Dec 12, 2006 at 03:17:48PM +0900, Keiichi KII wrote:
> 
> FYI, there's a robot named Dave Miller who will eventually notice your
> email and scold you for not cc:ing it to netdev, claiming that he
> doesn't read LKML.
> 

Thank you for your replies and reviews.

When I reflect your reviews to my modification, I'm going to also send a set of
patches to netdev in a few days.

-- 
Keiichi KII
NEC Corporation OSS Promotion Center
E-mail: k-keiichi@bx.jp.nec.com

Re: [RFC][PATCH 2.6.19 2/6] support multiple logging agents

[Matt Mackall]

On Tue, Dec 12, 2006 at 03:29:57PM +0900, Keiichi KII wrote:
> From: Keiichi KII <k-keiichi@bx.jp.nec.com>
> 
> This patch contains the following changes for supporting multiple logging agents.
> 
> 1. extend netconsole to multiple netpolls
>    To send kernel messages to multiple logging agents, extend netcosnole
>     to be able to use multiple netpolls. Each netpoll sends kernel messages
>     to its own logging agent.
> 
> 2. change config parameter format
>    I change config parameter format as follows.
> 
>    [BNF - netconsole config parameter format - ]
>    -before
>     netconsole  ::= "netconsole=" netcon_port
>    -after
>     netconsole  ::= "netconsole=" netcon_port ( ":" netcon_port )*
> 
>     netcon_port ::= src-conf "," dst-conf
>     src-conf    ::= src-port? "@" src-ip? "/" src-dev?
>     dst-conf    ::= dst-port? "@" dst-ip "/" dst-macaddr?
> 
>     src-port    ::= source port number for UDP packets
>     src-ip      ::= source IP to use
>     src-dev     ::= source network interface
>     dst-port    ::= port number for logging agent
>     dst-ip      ::= IP address for logging agent
>     dst-macaddr ::= MAC address for logging agent
> 
>    ex?$B!Ksending kernel messages to 192.168.0.1 and 192.168.1.1.
>     modprobe netconsole netconsole=@/eth0,@192.168.0.1/:@/eth0,@192.168.1.1/
> 
> Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
> ---
> --- linux-2.6.19/drivers/net/netconsole.c	2006-12-06 14:37:26.806823250 +0900
> +++ enhanced-netconsole/drivers/net/netconsole.c.multiplex	2006-12-06
> 14:35:34.431800250 +0900
> @@ -60,6 +60,21 @@ static char config[MAX_CONFIG_LENGTH];
>  module_param_string(netconsole, config, MAX_CONFIG_LENGTH, 0);
>  MODULE_PARM_DESC(netconsole, "
> netconsole=[src-port]@[src-ip]/[dev],[tgt-port]@<tgt-ip>/[tgt-macaddr]\n");
> 
> +struct netconsole_device {
> +	struct list_head list;
> +	spinlock_t netpoll_lock;

The structure name here is not quite right - there's no real
correspondence between this struct and a device. 'netconsole_target'
or simply 'netconsole' might be a better name.

> +	int id;
> +	struct netpoll np;
> +};
> +
> +static int add_netcon_dev(const char*);
> +static void cleanup_netconsole(void);
> +static void netcon_dev_cleanup(struct netconsole_device *nd);

Please keep function naming consistent. I'd prefer netconsole_add/del
here. Also, please keep all the argument names in your function prototypes.

>  static struct netpoll np = {
>  	.name = "netconsole",
>  	.dev_name = "eth0",
> @@ -69,23 +84,91 @@ static struct netpoll np = {
>  	.drop = netpoll_queue,
>  };

Shouldn't this piece get dropped in this patch?

> -static int configured = 0;
> +static int add_netcon_dev(const char* target_opt)
> +{
> +	static atomic_t netcon_dev_count = ATOMIC_INIT(0);

Hiding this inside a function seems wrong. Why do we need a count? If
we've already got a spinlock, why does it need to be atomic?

> +	struct netconsole_device *new_dev;
> +	char *netcon_dev_config;
> +
> +	netcon_dev_config = (char*)kmalloc(MAX_CONFIG_LENGTH+1, GFP_ATOMIC);
> +	if (!netcon_dev_config) {
> +		printk(KERN_INFO "netconsole: kmalloc() failed!\n");
> +		return -ENOMEM;
> +	}
> +	strncpy(netcon_dev_config, target_opt, MAX_CONFIG_LENGTH);

Why do we need to copy this string?

> +	
> +	new_dev = (struct netconsole_device*)kmalloc(
> +		sizeof(struct netconsole_device), GFP_ATOMIC);

Cast of void * is unnecessary.

> +	if (!new_dev) {
> +		printk(KERN_INFO "netconsole: kmalloc() failed!\n");
> +		kfree(netcon_dev_config);
> +		return -ENOMEM;
> +	}
> +	
> +	memset(new_dev, 0, sizeof(struct netconsole_device));
> +	spin_lock_init(&new_dev->netpoll_lock);
> +
> +	new_dev->np = np;
> +	if (netpoll_parse_options(&new_dev->np, netcon_dev_config)) {
> +		printk(KERN_INFO
> +		       "netconsole: can't parse config at %d\n",new_dev->id);
> +		goto setup_fail;
> +	}

I think we should print the config string we couldn't parse here instead.

> +	if (netpoll_setup(&new_dev->np)) {
> +		printk(KERN_INFO "netconsole: id:%d can't setup netpoll\n",
> +		       new_dev->id);
> +		goto setup_fail;
> +	}
> +	
> +	atomic_inc(&netcon_dev_count);
> +	new_dev->id = atomic_read(&netcon_dev_count);
> +
> +	printk(KERN_INFO "netconsole: add target id:%d\n", new_dev->id);

And perhaps the target IP address/port here.

> +
> +	spin_lock(&netconsole_dev_list_lock);
> +	list_add(&new_dev->list, &active_netconsole_dev);
> +	spin_unlock(&netconsole_dev_list_lock);
> +
> +	kfree(netcon_dev_config);
> +	return 0;
> +
> + setup_fail:
> +	kfree(netcon_dev_config);
> +	kfree(new_dev);
> +	return -EINVAL;
> +}
> +


>  static void write_msg(struct console *con, const char *msg, unsigned int len)
>  {
>  	int frag, left;
>  	unsigned long flags;
> +	struct netconsole_device *dev;
> 
> -	if (!np.dev)
> +	if (list_empty(&active_netconsole_dev))
>  		return;
> 
>  	local_irq_save(flags);
> +	spin_lock(&netconsole_dev_list_lock);
>  	for(left = len; left; ) {
>  		frag = min(left, MAX_PRINT_CHUNK);
> -		netpoll_send_udp(&np, msg, frag);
> +		list_for_each_entry(dev, &active_netconsole_dev, list) {
> +			spin_lock(&dev->netpoll_lock);
> +			netpoll_send_udp(&dev->np, msg, frag);
> +			spin_unlock(&dev->netpoll_lock);

Why do we need a lock here? Why isn't the list lock sufficient? What
happens if either lock is held when we get here?

> +		}
>  		msg += frag;
>  		left -= frag;
>  	}
> +	spin_unlock(&netconsole_dev_list_lock);
>  	local_irq_restore(flags);
>  }
> 
> @@ -95,9 +178,9 @@ static struct console netconsole = {
>  	.write = write_msg
>  };
> 
> -static int __init option_setup(char *opt)
> +static int __init __attribute_used__ option_setup(char *opt)

Why do we need __attribute_used__ here?

>  {
> -	configured = !netpoll_parse_options(&np, opt);
> +	strncpy(config, opt, MAX_CONFIG_LENGTH);
>  	return 1;
>  }
> 
> @@ -105,26 +188,36 @@ __setup("netconsole=", option_setup);
> 
>  static int __init init_netconsole(void)
>  {
> -	if(strlen(config))
> -		option_setup(config);
> +	char *p;
> +	char *tmp = config;
> 
> -	if(!configured) {
> -		printk("netconsole: not configured, aborting\n");
> +	register_console(&netconsole);
> +
> +	if(!strlen(config)) {
> +		printk(KERN_INFO "netconsole: not configured\n");
>  		return 0;
>  	}
> +	while ((p = strsep(&tmp, ":")) != NULL) {
> +		if (add_netcon_dev(p)) {
> +			printk(KERN_INFO
> +			       "netconsole: can't add target to netconsole\n");
> +			cleanup_netconsole();
> +			return -EINVAL;
> +		}
> +	}
> 
> -	if(netpoll_setup(&np))
> -		return -EINVAL;
> -
> -	register_console(&netconsole);
>  	printk(KERN_INFO "netconsole: network logging started\n");
>  	return 0;
>  }
> 
>  static void cleanup_netconsole(void)
>  {
> +	struct netconsole_device *dev, *tmp;
> +
>  	unregister_console(&netconsole);
> -	netpoll_cleanup(&np);
> +	list_for_each_entry_safe(dev, tmp, &active_netconsole_dev, list) {
> +		netcon_dev_cleanup(dev);
> +	}
>  }
> 
>  module_init(init_netconsole);
> 
> -- 
> Keiichi KII
> NEC Corporation OSS Promotion Center
> E-mail: k-keiichi@bx.jp.nec.com

-- 
Mathematics is the supreme nostalgia of our time.

Re: [RFC][PATCH 2.6.19 2/6] support multiple logging agents

[Andy Isaacson]

On Tue, Dec 12, 2006 at 12:42:50PM -0600, Matt Mackall wrote:
> > +	new_dev = (struct netconsole_device*)kmalloc(
> > +		sizeof(struct netconsole_device), GFP_ATOMIC);
> 
> Cast of void * is unnecessary.

Also,
1. use kzalloc rather than kmalloc+memset
2. use p = kzalloc(sizeof(*p) rather than p = kzalloc(sizeof(struct foo)
3. use goto to common error exit code rather than local return

> > +	if (!new_dev) {
> > +		printk(KERN_INFO "netconsole: kmalloc() failed!\n");
> > +		kfree(netcon_dev_config);
> > +		return -ENOMEM;
> > +	}
> > +	memset(new_dev, 0, sizeof(struct netconsole_device));

-andy

Re: [RFC][PATCH 2.6.19 2/6] support multiple logging agents

[Keiichi KII]

 >>  static struct netpoll np = {
 >> >      .name = "netconsole",
 >> >      .dev_name = "eth0",
 >> > @@ -69,23 +84,91 @@ static struct netpoll np = {
 >> >      .drop = netpoll_queue,
 >> >  };
 >
 > Shouldn't this piece get dropped in this patch?
 >

This piece isn't in -mm tree, but this piece is in 2.6.19.
Which version should I follow ?

 >> -static int configured = 0;
 >> +static int add_netcon_dev(const char* target_opt)
 >> +{
 >> +    static atomic_t netcon_dev_count = ATOMIC_INIT(0);
 >
 > Hiding this inside a function seems wrong. Why do we need a count? If
 > we've already got a spinlock, why does it need to be atomic?
 >

We don't have a spinlock for add_netcon_dev, because we don't need
to get a spinlock for add_netcon_dev except for list operation.
So, it must be atomic.

 >>      local_irq_save(flags);
 >> +    spin_lock(&netconsole_dev_list_lock);
 >>      for(left = len; left; ) {
 >>          frag = min(left, MAX_PRINT_CHUNK);
 >> -        netpoll_send_udp(&np, msg, frag);
 >> +        list_for_each_entry(dev, &active_netconsole_dev, list) {
 >> +            spin_lock(&dev->netpoll_lock);
 >> +            netpoll_send_udp(&dev->np, msg, frag);
 >> +            spin_unlock(&dev->netpoll_lock);
 >
 > Why do we need a lock here? Why isn't the list lock sufficient? What
 > happens if either lock is held when we get here?
 >

The netpoll_lock is for each structure containing information related to netpoll
(remote IP address and port, local IP address and port and so on).
If we don't take a spinlock for each structure, the target IP address and port
number are subject to change on the way sending packets.

-- 
Keiichi KII
NEC Corporation OSS Promotion Center
E-mail: k-keiichi@bx.jp.nec.com

Re: [RFC][PATCH 2.6.19 2/6] support multiple logging agents

[Matt Mackall]

On Wed, Dec 20, 2006 at 06:35:41PM +0900, Keiichi KII wrote:
> >>  static struct netpoll np = {
> >> >      .name = "netconsole",
> >> >      .dev_name = "eth0",
> >> > @@ -69,23 +84,91 @@ static struct netpoll np = {
> >> >      .drop = netpoll_queue,
> >> >  };
> >
> > Shouldn't this piece get dropped in this patch?
> >
> 
> This piece isn't in -mm tree, but this piece is in 2.6.19.
> Which version should I follow ?

-mm, probably.

> >> -static int configured = 0;
> >> +static int add_netcon_dev(const char* target_opt)
> >> +{
> >> +    static atomic_t netcon_dev_count = ATOMIC_INIT(0);
> >
> > Hiding this inside a function seems wrong. Why do we need a count? If
> > we've already got a spinlock, why does it need to be atomic?
> >
> 
> We don't have a spinlock for add_netcon_dev, because we don't need
> to get a spinlock for add_netcon_dev except for list operation.
> So, it must be atomic.

Or you can just increment the list counter inside the lock.

> 
> >>      local_irq_save(flags);
> >> +    spin_lock(&netconsole_dev_list_lock);
> >>      for(left = len; left; ) {
> >>          frag = min(left, MAX_PRINT_CHUNK);
> >> -        netpoll_send_udp(&np, msg, frag);
> >> +        list_for_each_entry(dev, &active_netconsole_dev, list) {
> >> +            spin_lock(&dev->netpoll_lock);
> >> +            netpoll_send_udp(&dev->np, msg, frag);
> >> +            spin_unlock(&dev->netpoll_lock);
> >
> > Why do we need a lock here? Why isn't the list lock sufficient? What
> > happens if either lock is held when we get here?
> >
> 
> The netpoll_lock is for each structure containing information related to 
> netpoll
> (remote IP address and port, local IP address and port and so on).
> If we don't take a spinlock for each structure, the target IP address and 
> port
> number are subject to change on the way sending packets.

Why can't you simply define the list lock as protecting all the
structures on the list?

-- 
Mathematics is the supreme nostalgia of our time.