[RFC][PATCH -mm take5 0/7] proposal for dynamic configurable netconsole

[RFC][PATCH -mm take5 0/7] 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 remove it and reload with different
parameters.

By adopting my patches, the current netconsole becomes a little complex.
But the kernel messages(especially panic messages) is significant information
 to solve bugs and troubles promptly and we have been losing serial console
port with PCs and Servers.

I think that we need the environment in which we can collect kernel messages
flexibly.

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.

[changes since take4]
-change kernel base from 2.6.21-rc6-mm1 to 2.6.22-rc4-mm2.
-update Documentation/networking/netconsole.txt
-fix Kconfig
-avoid forward-declared statics
-fix coding style
-use spin_lock_irqsave() and _restore()
-fix race condition(netconsole_event())
-remove extra lock(write in sysfs)
-change ioctl's location
-use kasprintf()
-error handling

Your comments are very welcome.

Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
Signed-off-by: Takayoshi Kochi <t-kochi@bq.jp.nec.com>
---

[RFC][PATCH -mm take5 1/7] marking __init

[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()).

Acked-by: Matt Mackall <mpm@selenic.com>
Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
Signed-off-by: Takayoshi Kochi <t-kochi@bq.jp.nec.com>
---
Index: linux-mm/drivers/net/netconsole.c
===================================================================
--- linux-mm.orig/drivers/net/netconsole.c
+++ linux-mm/drivers/net/netconsole.c
@@ -91,7 +91,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;
@@ -99,7 +99,7 @@ static int option_setup(char *opt)
 
 __setup("netconsole=", option_setup);
 
-static int init_netconsole(void)
+static int __init init_netconsole(void)
 {
 	int err;
 

-- 

Re: [RFC][PATCH -mm take5 1/7] marking __init

[Matt Mackall]

On Wed, Jun 13, 2007 at 07:25:55PM +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()).
> 
> Acked-by: Matt Mackall <mpm@selenic.com>
> Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
> Signed-off-by: Takayoshi Kochi <t-kochi@bq.jp.nec.com>

Dave, please take this one. Not sure what to do about the rest, sigh.

-- 
Mathematics is the supreme nostalgia of our time.

[RFC][PATCH -mm take5 2/7] support multiple logging

[Keiichi KII]

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
   We change config parameter format from single configuration to multiple 
   configurations separated by ';'.

   ex) sending kernel messages to destination1 and destination2 using eth0.
    modprobe netconsole \
            netconsole="@/eth0,@[destination1]/;@/eth0,@[destination2]/"

3. introduce CONFIG_NETCONSOLE_DYNCON config to change between 
   existing netconsole and netconsole applying the above function.

Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
Signed-off-by: Takayoshi Kochi <t-kochi@bq.jp.nec.com>
---
Index: mm/drivers/net/netconsole.c
===================================================================
--- mm.orig/drivers/net/netconsole.c
+++ mm/drivers/net/netconsole.c
@@ -61,21 +61,104 @@ static struct netpoll np = {
 	.remote_port = 6666,
 	.remote_mac = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff},
 };
-static int configured = 0;
 
 #define MAX_PRINT_CHUNK 1000
 
+#ifdef CONFIG_NETCONSOLE_DYNCON
+struct netconsole_target {
+	struct list_head list;
+	int id;
+	struct netpoll np;
+};
+
+static LIST_HEAD(target_list);
+static DEFINE_SPINLOCK(target_list_lock);
+
+static void remove_target(struct netconsole_target *nt)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&target_list_lock, flags);
+	list_del(&nt->list);
+	if (list_empty(&target_list))
+		netpoll_cleanup(&nt->np);
+	spin_unlock_irqrestore(&target_list_lock, flags);
+	kfree(nt);
+}
+
+static int add_target(char* target_config)
+{
+	int retval = 0;
+	unsigned long flags;
+	static atomic_t target_count = ATOMIC_INIT(0);
+	struct netconsole_target *new_target;
+
+	new_target = kzalloc(sizeof(*new_target), GFP_KERNEL);
+	if (!new_target) {
+		printk(KERN_ERR "netconsole: kmalloc() failed!\n");
+		retval = -ENOMEM;
+		goto out;
+	}
+
+	new_target->np = np;
+	if (netpoll_parse_options(&new_target->np, target_config)) {
+		printk(KERN_ERR "netconsole: can't parse config:%s\n",
+		       target_config);
+		kfree(new_target);
+		retval = -EINVAL;
+		goto out;
+	}
+	if (netpoll_setup(&new_target->np)) {
+		printk(KERN_ERR "netconsole: can't setup netpoll:%s\n",
+		       target_config);
+		kfree(new_target);
+		retval = -EINVAL;
+		goto out;
+	}
+
+	new_target->id = atomic_inc_return(&target_count);
+
+	printk(KERN_INFO "netconsole: add target: "
+	       "remote ip_addr=%d.%d.%d.%d remote port=%d\n",
+	       HIPQUAD(new_target->np.remote_ip), new_target->np.remote_port);
+
+	spin_lock_irqsave(&target_list_lock, flags);
+	list_add(&new_target->list, &target_list);
+	spin_unlock_irqrestore(&target_list_lock, flags);
+
+ out:
+	return retval;
+}
+#endif /* CONFIG_NETCONSOLE_DYNCON */
+
 static void write_msg(struct console *con, const char *msg, unsigned int len)
 {
 	int frag, left;
 	unsigned long flags;
+#ifdef CONFIG_NETCONSOLE_DYNCON
+	struct netconsole_target *target;
+
+	if (list_empty(&target_list))
+		return;
+
+	spin_lock_irqsave(&target_list_lock, flags);
 
+	for (left = len; left; ) {
+		frag = min(left, MAX_PRINT_CHUNK);
+		list_for_each_entry(target, &target_list, list)
+			netpoll_send_udp(&target->np, msg, frag);
+		msg += frag;
+		left -= frag;
+	}
+
+	spin_unlock_irqrestore(&target_list_lock, flags);
+#else
 	if (!np.dev)
 		return;
 
 	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;
@@ -83,6 +166,7 @@ static void write_msg(struct console *co
 	}
 
 	local_irq_restore(flags);
+#endif /* CONFIG_NETCONSOLE_DYNCON */
 }
 
 static struct console netconsole = {
@@ -91,40 +175,59 @@ static struct console netconsole = {
 	.write = write_msg
 };
 
+#ifndef MODULE
 static int __init option_setup(char *opt)
 {
-	configured = !netpoll_parse_options(&np, opt);
+	strncpy(config, opt, 256);
 	return 1;
 }
 
 __setup("netconsole=", option_setup);
+#endif
 
-static int __init init_netconsole(void)
+static void cleanup_netconsole(void)
 {
-	int err;
+#ifdef CONFIG_NETCONSOLE_DYNCON
+	struct netconsole_target *nt, *tmp;
 
-	if(strlen(config))
-		option_setup(config);
+	unregister_console(&netconsole);
+	list_for_each_entry_safe(nt, tmp, &target_list, list)
+		remove_target(nt);
+#else
+	unregister_console(&netconsole);
+	netpoll_cleanup(&np);
+#endif /* CONFIG_NETCONSOLE_DYNCON */
+}
 
-	if(!configured) {
-		printk("netconsole: not configured, aborting\n");
+static int __init init_netconsole(void)
+{
+	char *tmp = config;
+#ifdef CONFIG_NETCONSOLE_DYNCON
+	char *p;
+
+	register_console(&netconsole);
+	if (!strlen(config)) {
+		printk(KERN_ERR "netconsole: not configured\n");
 		return 0;
 	}
-
-	err = netpoll_setup(&np);
-	if (err)
-		return err;
-
+	while ((p = strsep(&tmp, ";")) != NULL)
+		if (add_target(p)) {
+			printk(KERN_ERR
+			       "netconsole: can't add target to netconsole\n");
+			cleanup_netconsole();
+			return -EINVAL;
+		}
+#else
+	if (!strlen(config) ||
+	    netpoll_parse_options(&np, tmp) || netpoll_setup(&np)) {
+		printk(KERN_ERR "netconsole: can't parse config:%s\n", tmp);
+		return -EINVAL;
+	}
 	register_console(&netconsole);
+#endif /* CONFIG_NETCONSOLE_DYNCON */
 	printk(KERN_INFO "netconsole: network logging started\n");
 	return 0;
 }
 
-static void cleanup_netconsole(void)
-{
-	unregister_console(&netconsole);
-	netpoll_cleanup(&np);
-}
-
 module_init(init_netconsole);
 module_exit(cleanup_netconsole);
Index: mm/drivers/net/Kconfig
===================================================================
--- mm.orig/drivers/net/Kconfig
+++ mm/drivers/net/Kconfig
@@ -3019,6 +3019,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_DYNCON
+	default y
+	bool "Support for multiple logging and UI for netconsole"
+	depends on NETCONSOLE
+	---help---
+	  This option enables multiple logging and changing dynamically
+	  configurations (e.g. IP adderss, port number and so on)
+	  by using sysfs and ioctl.
+	  See <file:Documentation/networking/netconsole.txt> for details.
+
 config NETPOLL
 	def_bool NETCONSOLE
 

-- 

Re: [RFC][PATCH -mm take5 2/7] support multiple logging

[Satyam Sharma]

Hi Keiichi,

On 6/13/07, Keiichi KII <k-keiichi@bx.jp.nec.com> wrote:

> +struct netconsole_target {
> +       struct list_head list;
> +       int id;
> +       struct netpoll np;
> +};
> +
> +static LIST_HEAD(target_list);
> +static DEFINE_SPINLOCK(target_list_lock);

Some description of the struct netconsole_target / target_list
would be nice. For instance, here I tried to read from the code
what the "int id" member is all about, but noticed that it is
only being set during add_target(), and never read / used
anywhere else later. You should just remove it, then.

> +       if (netpoll_setup(&new_target->np)) {
> +               printk(KERN_ERR "netconsole: can't setup netpoll:%s\n",
> +                      target_config);
> +               kfree(new_target);
> +               retval = -EINVAL;

Please prefer something like "err = netpoll_setup(...);"
and then "if (err) { printk(); kfree(); return err; }" here.

netpoll_setup() returns perfectly reasonable error codes,
we must be returning the same back up.

> +#ifndef MODULE
>  static int __init option_setup(char *opt)
>  {
> -       configured = !netpoll_parse_options(&np, opt);
> +       strncpy(config, opt, 256);

strlcpy would be safer than strncpy here, IMHO.

> -static int __init init_netconsole(void)
> +static void cleanup_netconsole(void)
>  {
> -       int err;
> +#ifdef CONFIG_NETCONSOLE_DYNCON
> +       struct netconsole_target *nt, *tmp;
>
> -       if(strlen(config))
> -               option_setup(config);
> +       unregister_console(&netconsole);

> +       list_for_each_entry_safe(nt, tmp, &target_list, list)
> +               remove_target(nt);

You should be doing the spin_lock_irqsave() and
spin_unlock_irqrestore() around this code here,
and lose it from the remove_target() ...

> +static int __init init_netconsole(void)
> +{
> +       char *tmp = config;
> +#ifdef CONFIG_NETCONSOLE_DYNCON
> +       char *p;
> +
> +       register_console(&netconsole);

register_console() needs to be _after_ netpoll_setup().

> +       if (!strlen(config)) {
> +               printk(KERN_ERR "netconsole: not configured\n");
>                 return 0;
>         }

You could move this out of the #ifdef. It's applicable to both
DYNCON and !DYNCON cases.

> -       err = netpoll_setup(&np);
> -       if (err)
> -               return err;
> -
> +       while ((p = strsep(&tmp, ";")) != NULL)
> +               if (add_target(p)) {
> +                       printk(KERN_ERR
> +                              "netconsole: can't add target to netconsole\n");
> +                       cleanup_netconsole();
> +                       return -EINVAL;
> +               }
> +#else
> +       if (!strlen(config) ||
> +           netpoll_parse_options(&np, tmp) || netpoll_setup(&np)) {

Joining these 3 expressions (statements, actually) together
is so gross :-)
Please do them separately, like the previous code did. That way
you could preserve the error code from netpoll_setup() too ...

Satyam

[RFC][PATCH -mm take5 3/7] 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
   | |--- local_ip    [rw-r--r--]  source IP to use, writable
   | |--- local_mac   [r--r--r--]  source MAC address
   | |--- local_port  [rw-r--r--]  source port number for UDP packets, writable
   | |--- remote_ip   [rw-r--r--]  port number for logging agent, writable
   | |--- remote_mac  [rw-r--r--]  MAC address for logging agent, writable
   | ---- remote_port [rw-r--r--]  IP address for logging agent, writable
   |--- port2/
   |--- port3/
   ...

Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
Signed-off-by: Takayoshi Kochi <t-kochi@bq.jp.nec.com>
---
Index: trunk/drivers/net/netconsole.c
===================================================================
--- trunk.orig/drivers/net/netconsole.c
+++ trunk/drivers/net/netconsole.c
@@ -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");
@@ -67,6 +69,7 @@ static struct netpoll np = {
 #ifdef CONFIG_NETCONSOLE_DYNCON
 struct netconsole_target {
 	struct list_head list;
+	struct kobject obj;
 	int id;
 	struct netpoll np;
 };
@@ -74,6 +77,164 @@ struct netconsole_target {
 static LIST_HEAD(target_list);
 static DEFINE_SPINLOCK(target_list_lock);
 
+static int miscdev_configured;
+
+static ssize_t show_id(struct netconsole_target *nt, char *buf)
+{
+	return sprintf(buf, "%d\n", nt->id);
+}
+
+static ssize_t show_local_port(struct netconsole_target *nt, char *buf)
+{
+	return sprintf(buf, "%d\n", nt->np.local_port);
+}
+
+static ssize_t show_remote_port(struct netconsole_target *nt, char *buf)
+{
+	return sprintf(buf, "%d\n", nt->np.remote_port);
+}
+
+static ssize_t show_local_ip(struct netconsole_target *nt, char *buf)
+{
+	return sprintf(buf, "%d.%d.%d.%d\n", HIPQUAD(nt->np.local_ip));
+}
+
+static ssize_t show_remote_ip(struct netconsole_target *nt, char *buf)
+{
+	return sprintf(buf, "%d.%d.%d.%d\n", HIPQUAD(nt->np.remote_ip));
+}
+
+static ssize_t show_local_mac(struct netconsole_target *nt, char *buf)
+{
+	return sprintf(buf, "%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 sprintf(buf, "%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]);
+}
+
+static ssize_t store_local_port(struct netconsole_target *nt, const char *buf,
+				size_t count)
+{
+	nt->np.local_port = simple_strtol(buf, NULL, 10);
+
+	return count;
+}
+
+static ssize_t store_remote_port(struct netconsole_target *nt, const char *buf,
+				size_t count)
+{
+	nt->np.remote_port = simple_strtol(buf, NULL, 10);
+
+	return count;
+}
+
+static ssize_t store_local_ip(struct netconsole_target *nt, const char *buf,
+			      size_t count)
+{
+	nt->np.local_ip = ntohl(in_aton(buf));
+
+	return count;
+}
+
+static ssize_t store_remote_ip(struct netconsole_target *nt, const char *buf,
+			       size_t count)
+{
+	nt->np.remote_ip = ntohl(in_aton(buf));
+
+	return count;
+}
+
+static ssize_t store_remote_mac(struct netconsole_target *nt, const char *buf,
+			       size_t count)
+{
+	unsigned char input_mac[ETH_ALEN] =
+		{0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+	const char *cur = buf;
+	int i = 0;
+
+	input_mac[i++] = simple_strtol(cur, NULL, 16);
+	while ((cur = strchr(cur, ':')) != NULL) {
+		cur++;
+		input_mac[i++] = simple_strtol(cur, NULL, 16);
+	}
+	if (i != ETH_ALEN)
+		return -EINVAL;
+	memcpy(nt->np.remote_mac, input_mac, ETH_ALEN);
+
+	return count;
+}
+
+struct target_attr {
+	struct attribute attr;
+	ssize_t (*show)(struct netconsole_target*, char*);
+	ssize_t (*store)(struct netconsole_target*, const char*,
+			 size_t count);
+};
+
+#define NETCON_CLASS_ATTR(_name, _mode, _show, _store) \
+struct target_attr target_attr_##_name = \
+                           __ATTR(_name, _mode, _show, _store)
+
+static NETCON_CLASS_ATTR(id, S_IRUGO, show_id, 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 struct attribute *target_attrs[] = {
+	&target_attr_id.attr,
+	&target_attr_local_port.attr,
+	&target_attr_remote_port.attr,
+	&target_attr_local_ip.attr,
+	&target_attr_remote_ip.attr,
+	&target_attr_local_mac.attr,
+	&target_attr_remote_mac.attr,
+	NULL
+};
+
+static ssize_t show_target_attr(struct kobject *kobj,
+				    struct attribute *attr,
+				    char *buffer)
+{
+	struct target_attr *na =
+		container_of(attr, struct target_attr, attr);
+	struct netconsole_target * nt =
+		container_of(kobj, struct netconsole_target, obj);
+	if (na->show)
+		return na->show(nt, buffer);
+	else
+		return -EIO;
+}
+
+static ssize_t store_target_attr(struct kobject *kobj,
+				     struct attribute *attr,
+				     const char *buffer, size_t count)
+{
+	struct target_attr *na =
+		container_of(attr, struct target_attr, attr);
+	struct netconsole_target *nt =
+		container_of(kobj, struct netconsole_target, obj);
+	if (na->store)
+		return na->store(nt, buffer, count);
+	else
+		return -EIO;
+}
+
 static void remove_target(struct netconsole_target *nt)
 {
 	unsigned long flags;
@@ -86,6 +247,38 @@ static void remove_target(struct netcons
 	kfree(nt);
 }
 
+static void release_target(struct kobject *kobj)
+{
+	struct netconsole_target *nt =
+		container_of(kobj, struct netconsole_target, obj);
+
+	remove_target(nt);
+}
+
+static struct sysfs_ops target_sysfs_ops = {
+	.show = show_target_attr,
+	.store = store_target_attr
+};
+
+static struct miscdevice netconsole_miscdev = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "netconsole",
+};
+
+static struct kobj_type target_ktype = {
+	.release = release_target,
+	.sysfs_ops = &target_sysfs_ops,
+	.default_attrs = target_attrs,
+};
+
+static int setup_target_sysfs(struct netconsole_target *nt)
+{
+	kobject_set_name(&nt->obj, "port%d", nt->id);
+	nt->obj.parent = &netconsole_miscdev.this_device->kobj;
+	nt->obj.ktype = &target_ktype;
+	return kobject_register(&nt->obj);
+}
+
 static int add_target(char* target_config)
 {
 	int retval = 0;
@@ -126,6 +319,7 @@ static int add_target(char* target_confi
 	list_add(&new_target->list, &target_list);
 	spin_unlock_irqrestore(&target_list_lock, flags);
 
+	retval = setup_target_sysfs(new_target);
  out:
 	return retval;
 }
@@ -192,7 +386,9 @@ static void cleanup_netconsole(void)
 
 	unregister_console(&netconsole);
 	list_for_each_entry_safe(nt, tmp, &target_list, list)
-		remove_target(nt);
+		kobject_unregister(&nt->obj);
+	if (miscdev_configured)
+		misc_deregister(&netconsole_miscdev);
 #else
 	unregister_console(&netconsole);
 	netpoll_cleanup(&np);
@@ -205,6 +401,15 @@ static int __init init_netconsole(void)
 #ifdef CONFIG_NETCONSOLE_DYNCON
 	char *p;
 
+	if (misc_register(&netconsole_miscdev)) {
+		printk(KERN_ERR
+		       "netconsole: failed to register misc device for "
+		       "name = %s, id = %d\n",
+		       netconsole_miscdev.name, netconsole_miscdev.minor);
+		return -EIO;
+	} else
+		miscdev_configured = 1;
+
 	register_console(&netconsole);
 	if (!strlen(config)) {
 		printk(KERN_ERR "netconsole: not configured\n");

-- 

Re: [RFC][PATCH -mm take5 3/7] add interface for netconsole using sysfs

[Satyam Sharma]

Hi,

On 6/13/07, Keiichi KII <k-keiichi@bx.jp.nec.com> 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
>    | |--- local_ip    [rw-r--r--]  source IP to use, writable
>    | |--- local_mac   [r--r--r--]  source MAC address
>    | |--- local_port  [rw-r--r--]  source port number for UDP packets, writable
>    | |--- remote_ip   [rw-r--r--]  port number for logging agent, writable
>    | |--- remote_mac  [rw-r--r--]  MAC address for logging agent, writable
>    | ---- remote_port [rw-r--r--]  IP address for logging agent, writable
>    |--- port2/
>    |--- port3/

[...]
> +static int miscdev_configured;

Is this really required? We just return with error if misc_register()
fails during module init time itself, so it's not really useful ever, is it?

> +static ssize_t show_target_attr(struct kobject *kobj,
[...]
> +       if (na->show)
> +               return na->show(nt, buffer);
> +       else
> +               return -EIO;

and

> +static ssize_t store_target_attr(struct kobject *kobj,
[...]
> +       if (na->store)
> +               return na->store(nt, buffer, count);
> +       else
> +               return -EIO;

EIO isn't quite appropriate for the above two cases. EPERM?

> +static int setup_target_sysfs(struct netconsole_target *nt)
> +{
> +       kobject_set_name(&nt->obj, "port%d", nt->id);

Ah, I see, so this is where we use the ->id member.

> @@ -192,7 +386,9 @@ static void cleanup_netconsole(void)
>
>         unregister_console(&netconsole);
>         list_for_each_entry_safe(nt, tmp, &target_list, list)
> -               remove_target(nt);
> +               kobject_unregister(&nt->obj);
> +       if (miscdev_configured)
> +               misc_deregister(&netconsole_miscdev);

Yeah, I suspect we can do away with miscdev_configured here.
We reach this code only if it is known to be true.

> +       if (misc_register(&netconsole_miscdev)) {
> +               printk(KERN_ERR
> +                      "netconsole: failed to register misc device for "
> +                      "name = %s, id = %d\n",
> +                      netconsole_miscdev.name, netconsole_miscdev.minor);
> +               return -EIO;
> +       } else
> +               miscdev_configured = 1;
> +

Please prefer:

err = misc_register(...);
if (err)
        ...
        return err;

That way we avoid the weird EIO and maintain the error code
returned by misc_register().

Satyam

Re: [RFC][PATCH -mm take5 3/7] add interface for netconsole using sysfs

[Keiichi KII]

Hello Satyam,

First, I'm sorry I couldn't reply to your comments.
I'm so appreciate for your comments.

I will fix my patches following your advices.
But, I have some questions on the another patches. So, I want to ask you
some questions and answer your questions.

>> +static int miscdev_configured;
> 
> Is this really required? We just return with error if misc_register()
> fails during module init time itself, so it's not really useful ever, is 
> it?

You're right. It isn't required.

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

[RFC][PATCH -mm take5 4/7] using symlink for the net_device

[Keiichi KII]

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

We use symbolic link for net_device.
The link in sysfs represents the corresponding network etherdevice.

-+- /sys/class/misc/
 |-+- netconsole/
 |-+- port1/
 | |--- id             [r--r--r--]  id
 | |--- net:<net_dev>  [r--r--r--]  net_dev: eth0,eth1,...
 | ...
 |--- port2/
 ...

Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
Signed-off-by: Takayoshi Kochi <t-kochi@bq.jp.nec.com>
---
Index: mm/drivers/net/netconsole.c
===================================================================
--- mm.orig/drivers/net/netconsole.c
+++ mm/drivers/net/netconsole.c
@@ -76,6 +76,7 @@ struct netconsole_target {
 
 static LIST_HEAD(target_list);
 static DEFINE_SPINLOCK(target_list_lock);
+static DECLARE_MUTEX(netdev_change_sem);
 
 static int miscdev_configured;
 
@@ -239,12 +240,14 @@ static void remove_target(struct netcons
 {
 	unsigned long flags;
 
+	down(&netdev_change_sem);
 	spin_lock_irqsave(&target_list_lock, flags);
 	list_del(&nt->list);
 	if (list_empty(&target_list))
 		netpoll_cleanup(&nt->np);
 	spin_unlock_irqrestore(&target_list_lock, flags);
 	kfree(nt);
+	up(&netdev_change_sem);
 }
 
 static void release_target(struct kobject *kobj)
@@ -271,12 +274,35 @@ static struct kobj_type target_ktype = {
 	.default_attrs = target_attrs,
 };
 
+static char *make_netdev_class_name(char *netdev_name)
+{
+	char *name;
+
+	name = kasprintf(GFP_KERNEL, "net:%s", netdev_name);
+	if (!name) {
+		printk(KERN_ERR "netconsole: kmalloc() failed!\n");
+		return NULL;
+	}
+
+	return name;
+}
+
 static int setup_target_sysfs(struct netconsole_target *nt)
 {
+	int retval = 0;
+	char *name;
+
 	kobject_set_name(&nt->obj, "port%d", nt->id);
 	nt->obj.parent = &netconsole_miscdev.this_device->kobj;
 	nt->obj.ktype = &target_ktype;
-	return kobject_register(&nt->obj);
+	retval = kobject_register(&nt->obj);
+	name = make_netdev_class_name(nt->np.dev_name);
+	if (!name)
+		return -ENOMEM;
+	retval = sysfs_create_link(&nt->obj, &nt->np.dev->dev.kobj, name);
+	kfree(name);
+
+	return retval;
 }
 
 static int add_target(char* target_config)
@@ -323,6 +349,60 @@ static int add_target(char* target_confi
  out:
 	return retval;
 }
+
+static int netconsole_event(struct notifier_block *this, unsigned long event,
+			    void *ptr)
+{
+	int error = 0;
+	unsigned long flags;
+	char *old_link_name = NULL, *new_link_name = NULL;
+	struct netconsole_target *nt, *tmp;
+	struct net_device *dev = ptr;
+	LIST_HEAD(modify_target_list);
+
+	if (event == NETDEV_CHANGENAME) {
+		spin_lock_irqsave(&target_list_lock, flags);
+		list_for_each_entry_safe(nt, tmp, &target_list, list)
+			if (nt->np.dev == dev)
+				list_move(&nt->list, &modify_target_list);
+		spin_unlock_irqrestore(&target_list_lock, flags);
+
+		down(&netdev_change_sem);
+		list_for_each_entry(nt, &modify_target_list, list) {
+			new_link_name = make_netdev_class_name(dev->name);
+			old_link_name = make_netdev_class_name(nt->np.dev_name);
+			if (!new_link_name || !old_link_name) {
+				printk(KERN_ERR "netconsole: "
+				       "make_netdev_class_name() failed!\n");
+				kfree(new_link_name);
+				kfree(old_link_name);
+				continue;
+			}
+			sysfs_remove_link(&nt->obj, old_link_name);
+			error = sysfs_create_link(&nt->obj,
+						  &nt->np.dev->dev.kobj,
+						  new_link_name);
+			if (error)
+				printk(KERN_ERR "can't create link: %s\n",
+				       new_link_name);
+			strcpy(nt->np.dev_name, dev->name);
+			kfree(new_link_name);
+			kfree(old_link_name);
+		}
+		up(&netdev_change_sem);
+
+		spin_lock_irqsave(&target_list_lock, flags);
+		list_for_each_entry_safe(nt, tmp, &modify_target_list, list)
+			list_move(&nt->list, &target_list);
+		spin_unlock_irqrestore(&target_list_lock, flags);
+	}
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block netconsole_notifier = {
+	.notifier_call = netconsole_event,
+};
 #endif /* CONFIG_NETCONSOLE_DYNCON */
 
 static void write_msg(struct console *con, const char *msg, unsigned int len)
@@ -387,6 +467,7 @@ static void cleanup_netconsole(void)
 	unregister_console(&netconsole);
 	list_for_each_entry_safe(nt, tmp, &target_list, list)
 		kobject_unregister(&nt->obj);
+	unregister_netdevice_notifier(&netconsole_notifier);
 	if (miscdev_configured)
 		misc_deregister(&netconsole_miscdev);
 #else
@@ -410,6 +491,7 @@ static int __init init_netconsole(void)
 	} else
 		miscdev_configured = 1;
 
+	register_netdevice_notifier(&netconsole_notifier);
 	register_console(&netconsole);
 	if (!strlen(config)) {
 		printk(KERN_ERR "netconsole: not configured\n");

-- 

Re: [RFC][PATCH -mm take5 4/7] using symlink for the net_device

[Satyam Sharma]

Hi,

On 6/13/07, Keiichi KII <k-keiichi@bx.jp.nec.com> wrote:
> [...]
> +static DECLARE_MUTEX(netdev_change_sem);

The preferred style these days is to use a DEFINE_MUTEX
(and the struct mutex primitives) for such locks that are used
as binary semaphores.

BTW, a comment here to note what this lock protects is required.
[ You don't really need to give a comment for the target_list_lock
because it's defined just below the "target_list". It's not equally obvious
at first glance what is protected by the netdev_change_sem, however. ]

> +       down(&netdev_change_sem);
> +       up(&netdev_change_sem);

So these become mutex_lock and mutex_unlock.

> +static int netconsole_event(struct notifier_block *this, unsigned long event,
> +                           void *ptr)
> +{
> +       int error = 0;
> +       unsigned long flags;
> +       char *old_link_name = NULL, *new_link_name = NULL;
> +       struct netconsole_target *nt, *tmp;
> +       struct net_device *dev = ptr;
> +       LIST_HEAD(modify_target_list);
> +
> +       if (event == NETDEV_CHANGENAME) {
> +               spin_lock_irqsave(&target_list_lock, flags);
> +               list_for_each_entry_safe(nt, tmp, &target_list, list)
> +                       if (nt->np.dev == dev)
> +                               list_move(&nt->list, &modify_target_list);
> +               spin_unlock_irqrestore(&target_list_lock, flags);
> +
> +               down(&netdev_change_sem);
> +               list_for_each_entry(nt, &modify_target_list, list) {
> +                       new_link_name = make_netdev_class_name(dev->name);
> +                       old_link_name = make_netdev_class_name(nt->np.dev_name);
> +                       if (!new_link_name || !old_link_name) {
> +                               printk(KERN_ERR "netconsole: "
> +                                      "make_netdev_class_name() failed!\n");
> +                               kfree(new_link_name);
> +                               kfree(old_link_name);
> +                               continue;

Sorry, but we're not covering from the error condition fully here. Note
that later you merge the temporary modify_target_list entirely back
into the target_list ... which would still contain these erroneous
nodes. A full cleanup (kobject_unregister the entry, and then list_del
from modify_target_list) is required here, before continuing.

> +                       }
> +                       sysfs_remove_link(&nt->obj, old_link_name);
> +                       error = sysfs_create_link(&nt->obj,
> +                                                 &nt->np.dev->dev.kobj,
> +                                                 new_link_name);
> +                       if (error)
> +                               printk(KERN_ERR "can't create link: %s\n",
> +                                      new_link_name);

Same here, a fuller cleanup is required to recover from the error
condition here, then kfree()'s and then stick a "continue;" here, else ...

> +                       strcpy(nt->np.dev_name, dev->name);

... you'll have move this up.

> +                       kfree(new_link_name);
> +                       kfree(old_link_name);
> +               }
> +               up(&netdev_change_sem);
> +
> +               spin_lock_irqsave(&target_list_lock, flags);
> +               list_for_each_entry_safe(nt, tmp, &modify_target_list, list)
> +                       list_move(&nt->list, &target_list);
> +               spin_unlock_irqrestore(&target_list_lock, flags);
> +       }
> +
> +       return NOTIFY_DONE;
> +}

Satyam

Re: [RFC][PATCH -mm take5 4/7] using symlink for the net_device

[Satyam Sharma]

Hi again,

Ok, so sysfs_create_link() would be illegal from inside spin_lock_irqsave(),
and this is why we have to use the dual-list mechanism to react to the net
device rename. This isn't so obvious, a comment at the point where you
declare modify_target_list would be nice? (BTW temporary_list would be
a better name for that, IMO)

> On 6/13/07, Keiichi KII <k-keiichi@bx.jp.nec.com> wrote:
> > [...]
> > +static DECLARE_MUTEX(netdev_change_sem);
>
> The preferred style these days is to use a DEFINE_MUTEX
> (and the struct mutex primitives) for such locks that are used
> as binary semaphores.
>
> BTW, a comment here to note what this lock protects is required.
> [ You don't really need to give a comment for the target_list_lock
> because it's defined just below the "target_list". It's not equally obvious
> at first glance what is protected by the netdev_change_sem, however. ]

Ok, so reading through the code makes it obvious that this mutex is used
to protect against the following race:

Thread #1                               Thread #2
=========                               =========

[ NETDEV_CHANGENAME notifier ]          [ ioctl(NETCON_REMOVE_TARGET) ]

netconsole_event()
move from target_list to temp list
work on temp list
                                        kobject_unregister()
                                        -> release_target()
                                           -> remove_target()
move back to target_list

Which would mean a deleted/removed target added back => *boom*

But, the race still hasn't been closed properly!

You're taking the mutex only around "work on temp list" which is
insufficient, you need to ensure atomicity inside netconsole_event()
_completely_ like this (renaming netdev_change_sem to
netdev_changename_mtx):

> > +static int netconsole_event(struct notifier_block *this, unsigned long event,
> > +                           void *ptr)
> > +{
> > +       int error = 0;
> > +       unsigned long flags;
> > +       char *old_link_name = NULL, *new_link_name = NULL;
> > +       struct netconsole_target *nt, *tmp;
> > +       struct net_device *dev = ptr;
> > +       LIST_HEAD(modify_target_list);
> > +
> > +       if (event == NETDEV_CHANGENAME) {

mutex_lock(netdev_changename_mtx) here.

> > +               spin_lock_irqsave(&target_list_lock, flags);
> > +               list_for_each_entry_safe(nt, tmp, &target_list, list)
> > +                       if (nt->np.dev == dev)
> > +                               list_move(&nt->list, &modify_target_list);
> > +               spin_unlock_irqrestore(&target_list_lock, flags);

> > +               down(&netdev_change_sem);

This goes away.

> > +               list_for_each_entry(nt, &modify_target_list, list) {
> > + [...]
> > +               }

> > +               up(&netdev_change_sem);

So does this.

> > +               spin_lock_irqsave(&target_list_lock, flags);
> > +               list_for_each_entry_safe(nt, tmp, &modify_target_list, list)
> > +                       list_move(&nt->list, &target_list);
> > +               spin_unlock_irqrestore(&target_list_lock, flags);

mutex_unlock(netdev_changename_mtx) comes here.

> > +       }
> > +
> > +       return NOTIFY_DONE;
> > +}

> @@ -239,12 +240,14 @@ static void remove_target(struct netcons
>  {
>         unsigned long flags;
>
> +       down(&netdev_change_sem);
>         spin_lock_irqsave(&target_list_lock, flags);
>         list_del(&nt->list);
>         if (list_empty(&target_list))
>                 netpoll_cleanup(&nt->np);
>         spin_unlock_irqrestore(&target_list_lock, flags);
>         kfree(nt);
> +       up(&netdev_change_sem);
>  }

As I said earlier, the target_list_lock spin-locking needs to be
pushed out from here to the callers of remove_target.
=> mutex_lock(netdev_changename_mtx) must also be done
by them.

> +static char *make_netdev_class_name(char *netdev_name)
> +{
> +       char *name;
> +
> +       name = kasprintf(GFP_KERNEL, "net:%s", netdev_name);

Why the "net:" prefix in the filename?

> +       if (!name) {
> +               printk(KERN_ERR "netconsole: kmalloc() failed!\n");
> +               return NULL;
> +       }
> +
> +       return name;
> +}

And this doesn't want to be a separate function either.

>  static int setup_target_sysfs(struct netconsole_target *nt)
>  {
> +       int retval = 0;
> +       char *name;
> +
>         kobject_set_name(&nt->obj, "port%d", nt->id);
>         nt->obj.parent = &netconsole_miscdev.this_device->kobj;
>         nt->obj.ktype = &target_ktype;
> -       return kobject_register(&nt->obj);
> +       retval = kobject_register(&nt->obj);
> +       name = make_netdev_class_name(nt->np.dev_name);
> +       if (!name)
> +               return -ENOMEM;

Just call kasprintf() directly, why the obfuscation?

Satyam

Re: [RFC][PATCH -mm take5 4/7] using symlink for the net_device

[Keiichi KII]

Hello Satyam,

> and this is why we have to use the dual-list mechanism to react to the net
> device rename. This isn't so obvious, a comment at the point where you
> declare modify_target_list would be nice? (BTW temporary_list would be
> a better name for that, IMO)

All right, my patches are short of comments. So, I will add comments 
to the ambiguous codes.

> Ok, so reading through the code makes it obvious that this mutex is used
> to protect against the following race:
> 
> Thread #1                               Thread #2
> =========                               =========
> 
> [ NETDEV_CHANGENAME notifier ]          [ ioctl(NETCON_REMOVE_TARGET) ]
> 
> netconsole_event()
> move from target_list to temp list
> work on temp list
>                                        kobject_unregister()
>                                        -> release_target()
>                                           -> remove_target()
> move back to target_list
> 
> Which would mean a deleted/removed target added back => *boom*
> 
> But, the race still hasn't been closed properly!
> 
> You're taking the mutex only around "work on temp list" which is
> insufficient, you need to ensure atomicity inside netconsole_event()
> _completely_ like this (renaming netdev_change_sem to
> netdev_changename_mtx):

After the target moves from target_list to temporary_list, 
the kobject_unregister() of possible raced target isn't called 
in ioctl(NETCON_REMOVE_TARGET) because the target_list doesn't contain 
the target .

I have the wrong idea?

>> +static char *make_netdev_class_name(char *netdev_name)
>> +{
>> +       char *name;
>> +
>> +       name = kasprintf(GFP_KERNEL, "net:%s", netdev_name);
> 
> Why the "net:" prefix in the filename?

Because I drew upon dev_change_name() method in net/core/dev.c.
The device_rename() in the above function makes use of same prefix 
related to netdev.

>>  static int setup_target_sysfs(struct netconsole_target *nt)
>>  {
>> +       int retval = 0;
>> +       char *name;
>> +
>>         kobject_set_name(&nt->obj, "port%d", nt->id);
>>         nt->obj.parent = &netconsole_miscdev.this_device->kobj;
>>         nt->obj.ktype = &target_ktype;
>> -       return kobject_register(&nt->obj);
>> +       retval = kobject_register(&nt->obj);
>> +       name = make_netdev_class_name(nt->np.dev_name);
>> +       if (!name)
>> +               return -ENOMEM;
> 
> Just call kasprintf() directly, why the obfuscation?
> 

I drew upon dev_change_name() method in net/core/dev.c.

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

Re: [RFC][PATCH -mm take5 4/7] using symlink for the net_device

[Satyam Sharma]

Hi,

On 6/19/07, Keiichi KII <k-keiichi@bx.jp.nec.com> wrote:
> Hello Satyam,
>
> > and this is why we have to use the dual-list mechanism to react to the net
> > device rename. This isn't so obvious, a comment at the point where you
> > declare modify_target_list would be nice? (BTW temporary_list would be
> > a better name for that, IMO)
>
> All right, my patches are short of comments. So, I will add comments
> to the ambiguous codes.
>
> > Ok, so reading through the code makes it obvious that this mutex is used
> > to protect against the following race:
> >
> > Thread #1                               Thread #2
> > =========                               =========
> >
> > [ NETDEV_CHANGENAME notifier ]          [ ioctl(NETCON_REMOVE_TARGET) ]
> >
> > netconsole_event()
> > move from target_list to temp list
> > work on temp list
> >                                        kobject_unregister()
> >                                        -> release_target()
> >                                           -> remove_target()
> > move back to target_list
> >
> > Which would mean a deleted/removed target added back => *boom*
> >
> > But, the race still hasn't been closed properly!
> >
> > You're taking the mutex only around "work on temp list" which is
> > insufficient, you need to ensure atomicity inside netconsole_event()
> > _completely_ like this (renaming netdev_change_sem to
> > netdev_changename_mtx):
>
> After the target moves from target_list to temporary_list,
> the kobject_unregister() of possible raced target isn't called
> in ioctl(NETCON_REMOVE_TARGET) because the target_list doesn't contain
> the target .

Hum, then why have we introduced that lock (netdev_change_sem)
in the first place, as in what exactly is it protecting?

In any case, however, the point to extend the critical section here
to encapsulate all the three parts still stands. We wouldn't want
ioctl(NETCON_REMOVE_TARGET) on the specified target to
return without removing the target that the user specified just
because that target's ethernet interface happens to be currently
undergoing a name change. The correct behaviour would be to
sleep on a mutex till the renaming has completed (which will
then relinquish the mutex) and then (after acquiring the mutex)
proceed to remove it, IMHO.

> >> +static char *make_netdev_class_name(char *netdev_name)
> >> +{
> >> +       char *name;
> >> +
> >> +       name = kasprintf(GFP_KERNEL, "net:%s", netdev_name);
> >
> > Why the "net:" prefix in the filename?
>
> Because I drew upon dev_change_name() method in net/core/dev.c.
> The device_rename() in the above function makes use of same prefix
> related to netdev.

I think you're referring to make_class_name() here? That seems to
be somewhat bulkier than simply being a wrapper over kasprintf()
like the make_netdev_class_name() here. I'd definitely recommend
not obfuscating this simple functionality here.

Thanks,
Satyam

Re: [RFC][PATCH -mm take5 4/7] using symlink for the net_device

[Keiichi KII]

Hello Satyam,

> In any case, however, the point to extend the critical section here
> to encapsulate all the three parts still stands. We wouldn't want
> ioctl(NETCON_REMOVE_TARGET) on the specified target to
> return without removing the target that the user specified just
> because that target's ethernet interface happens to be currently
> undergoing a name change. The correct behaviour would be to
> sleep on a mutex till the renaming has completed (which will
> then relinquish the mutex) and then (after acquiring the mutex)
> proceed to remove it, IMHO.

You're right. I misunderstood. All the three parts needs to encapsulate.

>> >> +static char *make_netdev_class_name(char *netdev_name)
>> >> +{
>> >> +       char *name;
>> >> +
>> >> +       name = kasprintf(GFP_KERNEL, "net:%s", netdev_name);
>> >
>> > Why the "net:" prefix in the filename?
>>
>> Because I drew upon dev_change_name() method in net/core/dev.c.
>> The device_rename() in the above function makes use of same prefix
>> related to netdev.
> 
> I think you're referring to make_class_name() here? That seems to
> be somewhat bulkier than simply being a wrapper over kasprintf()
> like the make_netdev_class_name() here. I'd definitely recommend
> not obfuscating this simple functionality here.

I understand. The wrapper method such as make_netdev_class_name() isn't
appropriate in this case.

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

Re: [RFC][PATCH -mm take5 4/7] using symlink for the net_device

[Keiichi KII]

Hello Satyam,

> Sorry, but we're not covering from the error condition fully here. Note
> that later you merge the temporary modify_target_list entirely back
> into the target_list ... which would still contain these erroneous
> nodes. A full cleanup (kobject_unregister the entry, and then list_del
> from modify_target_list) is required here, before continuing.

I will fix this. If the error occurs, I think so that we need to cleanup
completely.

>> +                       strcpy(nt->np.dev_name, dev->name);
> 
> ... you'll have move this up.
> 

Why? I don't have opposition about moving this up, but I'm misplacing the abobe code?
or it isn't appropriate about coding style?

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

Re: [RFC][PATCH -mm take5 4/7] using symlink for the net_device

[Satyam Sharma]

Hi Keiichi,

On 6/19/07, Keiichi KII <k-keiichi@bx.jp.nec.com> wrote:
> Hello Satyam,
>
> > Sorry, but we're not covering from the error condition fully here. Note
> > that later you merge the temporary modify_target_list entirely back
> > into the target_list ... which would still contain these erroneous
> > nodes. A full cleanup (kobject_unregister the entry, and then list_del
> > from modify_target_list) is required here, before continuing.
>
> I will fix this. If the error occurs, I think so that we need to cleanup
> completely.

Yes, thanks.

> >> +                       strcpy(nt->np.dev_name, dev->name);
> >
> > ... you'll have move this up.
> >
>
> Why? I don't have opposition about moving this up, but I'm misplacing the abobe code?
> or it isn't appropriate about coding style?

As I said, _either_ you stick a "continue;" after recovering from the
error above this code (i.e. if the sysfs_create_link() just above this
line fails), _or_ you'll have to move the strcpy() up (above the
sysfs_create_link). Note that recovering fully would mean
unregistering the kobject for that target, deleting the node from the
modify_list and kfree'ing the struct netconsole_target *nt (otherwise
we'll have a memory leak on our hands). And if we've kfree'd nt, we'll
oops on trying to dereference it like we're doing in the strcpy here.

Thanks,
Satyam

[RFC][PATCH -mm take5 5/7] switch function of netpoll

[Keiichi KII]

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

This patch contains switch function of netpoll.

If "enabled" attribute of certain port is '1', this port is used
and the configurations of this port are unable to change.

If "enabled" attribute of certain port is '0', this port isn't used
and the configurations of this port are able to change.

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

Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
Signed-off-by: Takayoshi Kochi <t-kochi@bq.jp.nec.com>
---
Index: mm/drivers/net/netconsole.c
===================================================================
--- mm.orig/drivers/net/netconsole.c
+++ mm/drivers/net/netconsole.c
@@ -71,6 +71,7 @@ struct netconsole_target {
 	struct list_head list;
 	struct kobject obj;
 	int id;
+	int enabled;
 	struct netpoll np;
 };
 
@@ -121,9 +122,16 @@ static ssize_t show_remote_mac(struct ne
 		       nt->np.remote_mac[4], nt->np.remote_mac[5]);
 }
 
+static ssize_t show_enabled(struct netconsole_target *nt, char *buf)
+{
+	return sprintf(buf, "%d\n", nt->enabled);
+}
+
 static ssize_t store_local_port(struct netconsole_target *nt, const char *buf,
 				size_t count)
 {
+	if (nt->enabled)
+		return -EINVAL;
 	nt->np.local_port = simple_strtol(buf, NULL, 10);
 
 	return count;
@@ -132,6 +140,8 @@ static ssize_t store_local_port(struct n
 static ssize_t store_remote_port(struct netconsole_target *nt, const char *buf,
 				size_t count)
 {
+	if (nt->enabled)
+		return -EINVAL;
 	nt->np.remote_port = simple_strtol(buf, NULL, 10);
 
 	return count;
@@ -140,6 +150,8 @@ static ssize_t store_remote_port(struct 
 static ssize_t store_local_ip(struct netconsole_target *nt, const char *buf,
 			      size_t count)
 {
+	if (nt->enabled)
+		return -EINVAL;
 	nt->np.local_ip = ntohl(in_aton(buf));
 
 	return count;
@@ -148,6 +160,8 @@ static ssize_t store_local_ip(struct net
 static ssize_t store_remote_ip(struct netconsole_target *nt, const char *buf,
 			       size_t count)
 {
+	if (nt->enabled)
+		return -EINVAL;
 	nt->np.remote_ip = ntohl(in_aton(buf));
 
 	return count;
@@ -166,13 +180,34 @@ static ssize_t store_remote_mac(struct n
 		cur++;
 		input_mac[i++] = simple_strtol(cur, NULL, 16);
 	}
-	if (i != ETH_ALEN)
+	if (i != ETH_ALEN || nt->enabled)
 		return -EINVAL;
 	memcpy(nt->np.remote_mac, input_mac, ETH_ALEN);
 
 	return count;
 }
 
+static ssize_t store_enabled(struct netconsole_target *nt, const char *buf,
+			    size_t count)
+{
+	int enabled = 0;
+
+	if (count >= 2 && (count != 2 || buf[count - 1] != '\n')) {
+		printk(KERN_ERR "netconsole: invalid argument: %s\n", buf);
+		return -EINVAL;
+	} else if (buf[0] == '1')
+		enabled = 1;
+	else if (buf[0] == '0')
+		enabled = 0;
+	else {
+		printk(KERN_ERR "netconsole: invalid argument: %s\n", buf);
+		return -EINVAL;
+	}
+	nt->enabled = enabled;
+
+	return count;
+}
+
 struct target_attr {
 	struct attribute attr;
 	ssize_t (*show)(struct netconsole_target*, char*);
@@ -196,6 +231,8 @@ 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(enabled, S_IRUGO | S_IWUSR,
+			 show_enabled, store_enabled);
 
 static struct attribute *target_attrs[] = {
 	&target_attr_id.attr,
@@ -205,6 +242,7 @@ static struct attribute *target_attrs[] 
 	&target_attr_remote_ip.attr,
 	&target_attr_local_mac.attr,
 	&target_attr_remote_mac.attr,
+	&target_attr_enabled.attr,
 	NULL
 };
 
@@ -336,6 +374,7 @@ static int add_target(char* target_confi
 	}
 
 	new_target->id = atomic_inc_return(&target_count);
+	new_target->enabled = 1;
 
 	printk(KERN_INFO "netconsole: add target: "
 	       "remote ip_addr=%d.%d.%d.%d remote port=%d\n",
@@ -420,7 +459,8 @@ static void write_msg(struct console *co
 	for (left = len; left; ) {
 		frag = min(left, MAX_PRINT_CHUNK);
 		list_for_each_entry(target, &target_list, list)
-			netpoll_send_udp(&target->np, msg, frag);
+			if (target->enabled)
+				netpoll_send_udp(&target->np, msg, frag);
 		msg += frag;
 		left -= frag;
 	}

-- 

Re: [RFC][PATCH -mm take5 5/7] switch function of netpoll

[Satyam Sharma]

Hi Keiichi,

On 6/13/07, Keiichi KII <k-keiichi@bx.jp.nec.com> wrote:
> From: Keiichi KII <k-keiichi@bx.jp.nec.com>
>
> This patch contains switch function of netpoll.
>
> If "enabled" attribute of certain port is '1', this port is used
> and the configurations of this port are unable to change.
>
> If "enabled" attribute of certain port is '0', this port isn't used
> and the configurations of this port are able to change.
>
> -+- /sys/class/misc/
> |-+- netconsole/
>   |-+- port1/
>   | |--- id          [r--r--r--]  id
>   | |--- enabled     [rw-r--r--]  0: disable 1: enable, writable
>   | ...
>   |--- port2/
>   ...
>
> Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
> Signed-off-by: Takayoshi Kochi <t-kochi@bq.jp.nec.com>
> ---
> Index: mm/drivers/net/netconsole.c
> ===================================================================
> --- mm.orig/drivers/net/netconsole.c
> +++ mm/drivers/net/netconsole.c
> @@ -71,6 +71,7 @@ struct netconsole_target {
>         struct list_head list;
>         struct kobject obj;
>         int id;
> +       int enabled;
>         struct netpoll np;
>  };

I really think you need to document/comment the struct members.
The newly introduced "enabled" for example, is serving a dual-purpose
in your code here (like you mention in the Changelog). It's used not
only to enable/disable a target, but also to ensure atomicity when
changing the (multiple) configurable parameters of a given target ...
the first purpose is self-evident from the name, but the second is
non-obvious. A sysfs node is also a userspace interface, so some
explicit mention of this is required.

Satyam

[RFC][PATCH -mm take5 6/7] add ioctls for adding/removing target

[Keiichi KII]

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

We add ioctls for adding/removing target.
If we use NETCONSOLE_ADD_TARGET ioctl, 
we can dynamically add netconsole target.
If we use NETCONSOLE_REMOVE_TARGET ioctl,
we can dynamically remoe netconsole target.

We attach a sample program for ioctl.

Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
Signed-off-by: Takayoshi Kochi <t-kochi@bq.jp.nec.com>
---
/*
 * This software is a sample program for ioctl of netconsole.
 * You can add/remove netconsole port by using this software.
 *
 * Copyright (C) 2007 NEC Corporation

 * Created by Keiichi KII <k-keiichi@bx.jp.nec.com>

 * This software is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License as
 * published by the Free Software Foundation; either version 2 or
 *  (at your option) any later version.
 */

#include <stdio.h>
#include <unistd.h>
#include <string.h>
#include <stdlib.h>
#include <stropts.h>
#include <fcntl.h>
#include <arpa/inet.h>
#include <net/if.h>
#include <linux/if_ether.h>
#include <linux/netconsole.h>

#define NETCONSOLE_DEV_NAME "/dev/netconsole"
#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))

struct command {
	char *name;
	char *options;
	int (*handle_command)(struct command* command, int argc, char* argv[]);
	void (*usage)(char *msg);
};

extern char *optarg;
extern int opterr, optind, errno;

static void generic_usage(char *msg) {
	fprintf(stderr, "Usage  : netconfig command [option] [args]\n");
	fprintf(stderr, "command: add remove help\n");
	exit(-1);
}

static int handle_command_add(struct command* command, int argc, char** argv)
{
	int i, fd, ch;
	unsigned int address;
	unsigned char mac[ETH_ALEN];
	struct netconsole_request req = {
		.netdev_name = "eth0",
		.local_port = 6665,
		.remote_port = 6666,
		.remote_mac = {0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF},
	};

	while ((ch = getopt(argc, argv, command->options)) != -1) {
		switch (ch) {
		case 'p':
			req.local_port = atoi(optarg);
			break;
		case 's':
			address = inet_addr(optarg);
			if (address == -1)
				(*command->usage)("invlid IP address!\n");
			req.local_ip = address;
			break;
		case 'h':
		default:
			(*command->usage)(NULL);
		}
	}
	argc -= optind;
	argv += optind;

	if (argc < 3 || argc > 4)
		(*command->usage)(NULL);

	memcpy(req.netdev_name, argv[0], IFNAMSIZ);
	address = inet_addr(argv[1]);
	if (address == -1)
		(*command->usage)("invlid IP address!\n");
	req.remote_ip = address;
	req.remote_port = atoi(argv[2]);
	if (argc == 4) {
		i = 0;
		mac[i++] = strtol(argv[3], NULL, 16);
		while ((argv[3] = strchr(argv[3], ':')) != NULL) {
			argv[3]++;
			mac[i++] = strtol(argv[3], NULL, 16);
		}
		if (i != ETH_ALEN)
			(*command->usage)("Invalid MAC address!\n");
		memcpy(req.remote_mac, mac, ETH_ALEN);
	}

	fd = open(NETCONSOLE_DEV_NAME, O_RDWR);
	if (fd < 0) {
		fprintf(stderr, "cannot open device" NETCONSOLE_DEV_NAME "\n");
		return -1;
	}

	if(ioctl(fd, NETCON_ADD_TARGET, &req) != 0)
		perror("add");
	close(fd);

	return 0;
}

static void usage_add(char *msg)
{
	if (msg != NULL)
		fprintf(stderr, "%s", msg);
	fprintf(stderr, "Usage  : netconfig add [-options] dev_name remote_ip "
		"remote_port [remote_mac]\n");
	fprintf(stderr, "options:\n");
	fprintf(stderr, "        -p local_port     :local port number\n");
	fprintf(stderr, "        -s local_up       :local IP address\n");
	exit(-1);
}

static int handle_command_remove(struct command *command,
				 int argc, char** argv)
{
	int fd, id, ch;

	while ((ch = getopt(argc, argv, command->options)) != -1) {
		switch (ch) {
		case 'h':
		default:
			(*command->usage)(NULL);
		}
	}
	argc -= optind;
	argv += optind;

	if (argc != 1)
		(*command->usage)(NULL);

	id = atoi(argv[0]);
	fd = open(NETCONSOLE_DEV_NAME, O_RDWR);
	if (fd < 0) {
		fprintf(stderr, "can't open device " NETCONSOLE_DEV_NAME "\n");
		return -1;
	}
	if(ioctl(fd, NETCON_REMOVE_TARGET, &id) != 0)
		perror("remove");
	close(fd);

	return 0;
}

static void usage_remove(char *msg)
{
	fprintf(stderr, "Usage  : netconfig remove id\n");
	exit(-1);
}

static int handle_command_help(struct command *command, int argc, char** argv)
{
	(*command->usage)(NULL);

	return 0;
}

static void usage_help(char *msg)
{
	generic_usage(NULL);
}

static struct command s_commands[] = {
	{"add", "hp:s:", handle_command_add, usage_add},
	{"remove", "hp:", handle_command_remove, usage_remove},
	{"help", NULL, handle_command_help, usage_help},
};

int main(int argc, char* argv[])
{
	int i;
	struct command *command = NULL;

	for(i = 0; i < ARRAY_SIZE(s_commands); i++) {
		if (argc <= 1)
			break;
		if (strcmp(s_commands[i].name, argv[1]) == 0) {
			command = &s_commands[i];
			break;
		}
	}

	if (command == NULL)
		generic_usage(NULL);
	argc--;
	argv++;

	if (command->handle_command)
		(*command->handle_command)(command, argc, argv);
	else {
		fprintf(stderr, "function handle_command() isn't set.");
		exit(-1);
	}
	return 0;
}
Index: mm/fs/compat_ioctl.c
===================================================================
--- mm.orig/fs/compat_ioctl.c
+++ mm/fs/compat_ioctl.c
@@ -114,6 +114,8 @@
 #include <linux/dvb/video.h>
 #include <linux/lp.h>
 
+#include <linux/netconsole.h>
+
 static int do_ioctl32_pointer(unsigned int fd, unsigned int cmd,
 			      unsigned long arg, struct file *f)
 {
@@ -3489,6 +3491,12 @@ HANDLE_IOCTL(LPSETTIMEOUT, lp_timeout_tr
 
 IGNORE_IOCTL(VFAT_IOCTL_READDIR_BOTH32)
 IGNORE_IOCTL(VFAT_IOCTL_READDIR_SHORT32)
+
+#ifdef CONFIG_NETCONSOLE_DYNCON
+/* netconsole */
+COMPATIBLE_IOCTL(NETCON_ADD_TARGET)
+ULONG_IOCTL(NETCON_REMOVE_TARGET)
+#endif
 };
 
 #define IOCTL_HASHSIZE 256
Index: mm/include/linux/netconsole.h
===================================================================
--- /dev/null
+++ mm/include/linux/netconsole.h
@@ -0,0 +1,18 @@
+#ifndef __NETCONSOLE_H
+#define __NETCONSOLE_H
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+struct netconsole_request {
+	__u32 id;
+	__u8 netdev_name[IFNAMSIZ];
+	__u8 remote_mac[ETH_ALEN];
+	__u32 local_ip, remote_ip;
+	__u16 local_port, remote_port;
+};
+
+#define NETCON_ADD_TARGET _IOW(0xAE, 4, struct netconsole_request)
+#define NETCON_REMOVE_TARGET _IOW(0xAE, 5, int)
+
+#endif /* __NETCONSOLE_H */
Index: mm/drivers/net/netconsole.c
===================================================================
--- mm.orig/drivers/net/netconsole.c
+++ mm/drivers/net/netconsole.c
@@ -47,6 +47,7 @@
 #include <linux/netpoll.h>
 #include <linux/miscdevice.h>
 #include <linux/inet.h>
+#include <linux/netconsole.h>
 
 MODULE_AUTHOR("Maintainer: Matt Mackall <mpm@selenic.com>");
 MODULE_DESCRIPTION("Console driver for network interfaces");
@@ -75,6 +76,8 @@ struct netconsole_target {
 	struct netpoll np;
 };
 
+static struct tty_driver *netconsole_tty_driver;
+
 static LIST_HEAD(target_list);
 static DEFINE_SPINLOCK(target_list_lock);
 static DECLARE_MUTEX(netdev_change_sem);
@@ -442,6 +445,73 @@ static int netconsole_event(struct notif
 static struct notifier_block netconsole_notifier = {
 	.notifier_call = netconsole_event,
 };
+
+static int netconsole_open(struct tty_struct *tty, struct file *file)
+{
+	return 0;
+}
+
+static int netconsole_ioctl(struct tty_struct *tty, struct file *file,
+			    unsigned int cmd, unsigned long arg)
+{
+	int id, count;
+	char config[256];
+	char *cur;
+	struct netconsole_request req;
+	struct netconsole_target *nt, *tmp;
+	void __user *argp = (void __user *)arg;
+
+	switch (cmd) {
+	case NETCON_ADD_TARGET:
+		printk(KERN_INFO "netconsole: cmd=NETCON_ADD_TARGET\n");
+		if (copy_from_user(&req, argp, sizeof(req)))
+			return -EFAULT;
+		cur = config;
+		count = sprintf(cur, "%d@", req.local_port);
+		cur += count;
+		if (req.local_ip)
+			count = sprintf(cur, "%d.%d.%d.%d/",
+					NIPQUAD(req.local_ip));
+		else
+			count = sprintf(cur, "/");
+		cur += count;
+		count = sprintf(cur, "%s,", req.netdev_name);
+		cur += count;
+		count = sprintf(cur, "%d@", req.remote_port);
+		cur += count;
+		count = sprintf(cur, "%d.%d.%d.%d/",
+				NIPQUAD(req.remote_ip));
+		cur += count;
+		count = sprintf(cur, "%02x:%02x:%02x:%02x:%02x:%02x",
+				req.remote_mac[0], req.remote_mac[1],
+				req.remote_mac[2], req.remote_mac[3],
+				req.remote_mac[4], req.remote_mac[5]);
+		printk(KERN_INFO "count = %d config=[%s]\n", count, config);
+		if (add_target(config))
+			return -EINVAL;
+		break;
+	case NETCON_REMOVE_TARGET:
+		printk(KERN_INFO "netconsole: cmd=NETCON_REMOVE_TARGET\n");
+		if (copy_from_user(&id, argp, sizeof(int)))
+			return -EFAULT;
+		printk(KERN_INFO "netconsole: id=%d\n", id);
+		list_for_each_entry_safe(nt, tmp, &target_list, list)
+			if (nt->id == id) {
+				kobject_unregister(&nt->obj);
+				break;
+			}
+		break;
+	default:
+		return -ENOTTY;
+	}
+
+	return 0;
+}
+
+static struct tty_operations netconsole_ops = {
+	.open = netconsole_open,
+	.ioctl = netconsole_ioctl,
+};
 #endif /* CONFIG_NETCONSOLE_DYNCON */
 
 static void write_msg(struct console *con, const char *msg, unsigned int len)
@@ -510,6 +580,8 @@ static void cleanup_netconsole(void)
 	unregister_netdevice_notifier(&netconsole_notifier);
 	if (miscdev_configured)
 		misc_deregister(&netconsole_miscdev);
+	tty_unregister_driver(netconsole_tty_driver);
+	put_tty_driver(netconsole_tty_driver);
 #else
 	unregister_console(&netconsole);
 	netpoll_cleanup(&np);
@@ -521,6 +593,7 @@ static int __init init_netconsole(void)
 	char *tmp = config;
 #ifdef CONFIG_NETCONSOLE_DYNCON
 	char *p;
+	int result;
 
 	if (misc_register(&netconsole_miscdev)) {
 		printk(KERN_ERR
@@ -531,6 +604,27 @@ static int __init init_netconsole(void)
 	} else
 		miscdev_configured = 1;
 
+	netconsole_tty_driver = alloc_tty_driver(1);
+	if (!netconsole_tty_driver)
+		return -ENOMEM;
+	netconsole_tty_driver->owner = THIS_MODULE;
+	netconsole_tty_driver->name = "netcon";
+	netconsole_tty_driver->driver_name = "netconsole";
+	netconsole_tty_driver->major = 0;
+	netconsole_tty_driver->minor_start = 0;
+	netconsole_tty_driver->num = 1;
+	netconsole_tty_driver->type = TTY_DRIVER_TYPE_CONSOLE;
+	tty_set_operations(netconsole_tty_driver, &netconsole_ops);
+
+	if ((result = tty_register_driver(netconsole_tty_driver))) {
+		printk(KERN_ERR "failed to register netconsole tty driver"
+		       " errno=%d\n", result);
+		put_tty_driver(netconsole_tty_driver);
+		if (miscdev_configured)
+			misc_deregister(&netconsole_miscdev);
+		return result;
+	}
+
 	register_netdevice_notifier(&netconsole_notifier);
 	register_console(&netconsole);
 	if (!strlen(config)) {

-- 

Re: [RFC][PATCH -mm take5 6/7] add ioctls for adding/removing target

[Satyam Sharma]

Hi Keiichi,

On 6/13/07, Keiichi KII <k-keiichi@bx.jp.nec.com> wrote:
> From: Keiichi KII <k-keiichi@bx.jp.nec.com>
>
> We add ioctls for adding/removing target.
> If we use NETCONSOLE_ADD_TARGET ioctl,
> we can dynamically add netconsole target.
> If we use NETCONSOLE_REMOVE_TARGET ioctl,
> we can dynamically remoe netconsole target.

*ugh*. I was wondering what a show-stopper this particular patch
was -- introduces a couple of ioctl()'s, exports a new structure to
userspace, adds a hitherto-unneeded header file, brings in
tty_struct/tty_operations and ends up adding so much complexity/
bloat to netconsole.c. Not only that, it must live together (and
side-by-side) with the sysfs interface also, because the two of them
do different things: sysfs to be able to modify target parameters at
run-time and the ioctl()'s to dynamically add/remove targets. We
can't really mkdir(2) or rmdir(2) in sysfs so the ioctl()'s are needed.

So may I suggest:

Just lose *both* the sysfs and ioctl() interfaces and use _configfs_.
It is *precisely* the thing you need in your driver here -- the ability
to create / destroy kernel objects (or config_items in configfs lingo)
from _userspace_ via simple mkdir(2) and rmdir(2). And configfs
makes changing multiple configurable parameters atomically trivial
too, via rename(2) ... not to mention a sysfs+ioctls -> configfs
conversion would help your patchset lose some weight too :-)

Satyam

Re: [RFC][PATCH -mm take5 6/7] add ioctls for adding/removing target

[Keiichi KII]

Hello Satyam,

> *ugh*. I was wondering what a show-stopper this particular patch
> was -- introduces a couple of ioctl()'s, exports a new structure to
> userspace, adds a hitherto-unneeded header file, brings in
> tty_struct/tty_operations and ends up adding so much complexity/
> bloat to netconsole.c. Not only that, it must live together (and
> side-by-side) with the sysfs interface also, because the two of them
> do different things: sysfs to be able to modify target parameters at
> run-time and the ioctl()'s to dynamically add/remove targets. We
> can't really mkdir(2) or rmdir(2) in sysfs so the ioctl()'s are needed.
> 
> So may I suggest:
> 
> Just lose *both* the sysfs and ioctl() interfaces and use _configfs_.
> It is *precisely* the thing you need in your driver here -- the ability
> to create / destroy kernel objects (or config_items in configfs lingo)
> from _userspace_ via simple mkdir(2) and rmdir(2). And configfs
> makes changing multiple configurable parameters atomically trivial
> too, via rename(2) ... not to mention a sysfs+ioctls -> configfs
> conversion would help your patchset lose some weight too :-)

Stephen Hemminger previously advised me	about the user interface such as 
the following messages.

> Some other speculations:
> 1. Would it be possible to add ioctl's to /dev/console? This would be more in
>     keeping with older Unix style model.
> 
> 2. Using sysfs makes sense if there is a device object that exists to
>    add the sysfs attributes to.
> 
> 3. Procfs is handy for summary type tables.
> 
> 4. Netlink does feel like overkill for this. Although newer generic netlink
>    makes it easier.

So, I implemented ioctls to add/remove port like this patch on the tty driver.
But I'm going to search configfs. Thank you for you information.

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

Re: [RFC][PATCH -mm take5 6/7] add ioctls for adding/removing target

[Satyam Sharma]

Hi again Keiichi,

On 6/19/07, Keiichi KII <k-keiichi@bx.jp.nec.com> wrote:
> Hello Satyam,
>
> > *ugh*. I was wondering what a show-stopper this particular patch
> > was -- introduces a couple of ioctl()'s, exports a new structure to
> > userspace, adds a hitherto-unneeded header file, brings in
> > tty_struct/tty_operations and ends up adding so much complexity/
> > bloat to netconsole.c. Not only that, it must live together (and
> > side-by-side) with the sysfs interface also, because the two of them
> > do different things: sysfs to be able to modify target parameters at
> > run-time and the ioctl()'s to dynamically add/remove targets. We
> > can't really mkdir(2) or rmdir(2) in sysfs so the ioctl()'s are needed.
> >
> > So may I suggest:
> >
> > Just lose *both* the sysfs and ioctl() interfaces and use _configfs_.
> > It is *precisely* the thing you need in your driver here -- the ability
> > to create / destroy kernel objects (or config_items in configfs lingo)
> > from _userspace_ via simple mkdir(2) and rmdir(2). And configfs
> > makes changing multiple configurable parameters atomically trivial
> > too, via rename(2) ... not to mention a sysfs+ioctls -> configfs
> > conversion would help your patchset lose some weight too :-)
>
> Stephen Hemminger previously advised me about the user interface such as
> the following messages.
>
> > Some other speculations:
> > 1. Would it be possible to add ioctl's to /dev/console? This would be more in
> >     keeping with older Unix style model.
> >
> > 2. Using sysfs makes sense if there is a device object that exists to
> >    add the sysfs attributes to.
> >
> > 3. Procfs is handy for summary type tables.
> >
> > 4. Netlink does feel like overkill for this. Although newer generic netlink
> >    makes it easier.
>
> So, I implemented ioctls to add/remove port like this patch on the tty driver.
> But I'm going to search configfs. Thank you for you information.

Hmm, I might've missed this thread, but my opinion on the
alternatives, fwiw:

1. I think adding new ioctl's to the kernel are generally disliked for
obvious reasons. Perhaps Stephen meant to add some generic
ioctl's above (and not separate ones specially implemented for
the dynamically reconfigurable netconsole driver)?

2. sysfs: In fact after reading Stephen's comment above I now noticed/
realized that we're introducing the misc device thing _specifically_ to
be able to attach kobjects for our use in the /sys/class/misc/
namespace? IMHO all such stuff (and the stuff we're adding to be able
to export the ioctl interface) would become unnecessary / redundant if
we switch to configfs, thus making this patchset quite small.

3. Again, as Stephen mentions, procfs interfaces are generally used
for different cases ... personally, I wouldn't even consider a procfs
interface for our case here :-)

4. Yup, netlink is clearly overkill. Also, I think that would require a
special application on the userspace side to talk over the netlink
socket, so we lose the ability to "echo > " from shell ... again,
netlink isn't _quite_ the thing for our case here, IMHO.

Please do consider configfs. Note that we'll have to lose the sysfs
symlink from your target's kobject to the kobject of the ethernet
device if we switch to configfs, but was that symlink needed for
some essential functionality or was it simply for informational
purpose? IMHO, this patchset only needs to bring in functionality
to be able to create, destroy, and modify netconsole targets at
run-time, and all these reconfiguration tasks would be handled
quite well by configfs, AFAICT.

Thanks again,
Satyam

PS: I really liked this patchset (and the idea behind it), and was
thinking of doing a configfs conversion myself, if you hadn't
replied :-) configfs has similar interfaces / structures as sysfs,
so it shouldn't really be too time-consuming.

Re: [RFC][PATCH -mm take5 6/7] add ioctls for adding/removing target

[Keiichi KII]

Hello Satyam,

> Hmm, I might've missed this thread, but my opinion on the
> alternatives, fwiw:
> 
> 1. I think adding new ioctl's to the kernel are generally disliked for
> obvious reasons. Perhaps Stephen meant to add some generic
> ioctl's above (and not separate ones specially implemented for
> the dynamically reconfigurable netconsole driver)?

You're right.
At first, I implemented ioctls to misc device because of using misc sysfs.
But, Andrew Morton said "Using an ioctl() against a miscdev is rather 
untypical for networking.". So, I implemented ioclts to tty_driver.

> Please do consider configfs. Note that we'll have to lose the sysfs
> symlink from your target's kobject to the kobject of the ethernet
> device if we switch to configfs, but was that symlink needed for
> some essential functionality or was it simply for informational
> purpose? IMHO, this patchset only needs to bring in functionality
> to be able to create, destroy, and modify netconsole targets at
> run-time, and all these reconfiguration tasks would be handled
> quite well by configfs, AFAICT.

It was for informational pupose. But, if we used symlink to the net_device 
kobject in sysfs, we could easily keep up with changing network device name by 
changing symbolic link.
In the case of configfs, Do we use config_item related to the network interface 
because the configfs doesn't have symlink that refers to net_device kobject
(e.g. "network_interface" in configfs, "network_interface" value is "eth0")?

I'm going to search configfs and modify interface to configfs.

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

Re: [RFC][PATCH -mm take5 6/7] add ioctls for adding/removing target

[Satyam Sharma]

Hi Keiichi,

> > Please do consider configfs. Note that we'll have to lose the sysfs
> > symlink from your target's kobject to the kobject of the ethernet
> > device if we switch to configfs, but was that symlink needed for
> > some essential functionality or was it simply for informational
> > purpose? IMHO, this patchset only needs to bring in functionality
> > to be able to create, destroy, and modify netconsole targets at
> > run-time, and all these reconfiguration tasks would be handled
> > quite well by configfs, AFAICT.
>
> It was for informational pupose. But, if we used symlink to the net_device
> kobject in sysfs, we could easily keep up with changing network device name by
> changing symbolic link.

In that case (because it is non-essential) we can drop that link entirely.
(This means the associated code for the temporary modify_list, the
extra mutex and the kasprintf() stuff can also be dropped too!) I guess
the netconsole_event() notifier could now become simply:

	if (event == NETDEV_CHANGENAME) {
		spin_lock_irqsave(&target_list_lock, ...);
		list_for_each_entry(nt, ..., &target_list, ...)
			if (nt->np.dev == dev)
				strcpy(nt->np.dev_name, dev->name);
		spin_unlock_irqrestore(&target_list_lock, ...);
	}

> In the case of configfs, Do we use config_item related to the network interface
> because the configfs doesn't have symlink that refers to net_device kobject
> (e.g. "network_interface" in configfs, "network_interface" value is "eth0")?

Yes, that's a good idea, this way we continue to give the information we
were giving previously. We could do this by defining another configfs
attribute of the target's config_item which could then be a proxy for
the char dev_name[IFNAMSIZ] member of the target's underlying netpoll
structure. (this attribute would naturally be read-only from userspace;
sort of similar to what you're doing for the local_mac attribute currently).

Please feel free to discuss anything else regarding this with me off-list,
if you want.

Thanks,
Satyam

[RFC][PATCH -mm take5 7/7] update documentation

[Keiichi KII]

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

update Documentation/networking/netconsole.txt
 - how to use sysfs for dynamic configurability
 - how to use ioctl for dynamic configurability

Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
---
Index: mm/Documentation/networking/netconsole.txt
===================================================================
--- mm.orig/Documentation/networking/netconsole.txt
+++ mm/Documentation/networking/netconsole.txt
@@ -4,6 +4,9 @@ started by Ingo Molnar <mingo@redhat.com
 
 Please send bug reports to Matt Mackall <mpm@selenic.com>
 
+1. Description
+--------------
+
 This module logs kernel printk messages over UDP allowing debugging of
 problem where disk logging fails and serial consoles are impractical.
 
@@ -13,6 +16,21 @@ the specified interface as soon as possi
 capture of early kernel panics, it does capture most of the boot
 process.
 
+This module equips the runtime configurability that can changes
+values(src/tgt IP address and port, tgt MAC address) by using sysfs
+and can add/remove logging agent by using ioctls.
+
+In order to use the runtime configurability, you have to enable
+CONFIG_NETCONSOLE_DYNCON. If you don't use one, you don't have to enable
+CONFIG_NETCONSOLE_DYNCON. By disabling this option, The kernel module
+size is smaller than the module enabled CONFIG_NETCONSOLE_DYNCON.
+
+2. Configuration
+----------------
+
+2.1 Module Parameter(sender side)
+---------------------------------
+
 It takes a string configuration parameter "netconsole" in the
 following format:
 
@@ -34,12 +52,21 @@ Examples:
 
  insmod netconsole netconsole=@/,@10.0.0.2/
 
+Or it also takes a semi-colon separated configuration parameter.
+In the case, you can send kerenl messages to multiple logging agents.
+
+ netconsole=<target1>;<target2>;<target3>
+
+ each target is the above configuration parameter.
+
+Examples:
+
+ netconsole="@/,6666@10.0.0.2/;@/,6666@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.
 
-The remote host can run either 'netcat -u -l -p <port>' or syslogd.
-
 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.
@@ -55,3 +82,83 @@ from IRQ contexts as well, and does not 
 sending packets. Due to these unique needs, configuration cannot
 be more automatic, and some fundamental limitations will remain:
 only IP networks, UDP packets and ethernet devices are supported.
+
+2.2 Receiver side
+-----------------
+
+The remote host can run either 'netcat -u -l -p <port>' or syslogd.
+
+3 Dynamic Configurability
+-------------------------
+
+You can make use of the dynamic configurability if
+CONFIG_NETCONSOLE_DYNCON is enabled. The operations are the following.
+
+3.1 /sys/class/misc/netconsole
+
+This entry, "netconsole", has the following attributes related to
+netconsole. 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/
+   | |--- enabled      [rw-r--r--] 0:disable 1:enable
+   | |--- id           [r--r--r--] unique port id
+   | |--- local_ip     [rw-r--r--] source IP to use
+   | |--- local_mac    [r--r--r--] source MAC address
+   | |--- local_port   [rw-r--r--] source port number for UDP packets
+   | |--- net:<netdev> [r--r--r--] symlink to net_dev: eth0,eth1,...
+   | |--- remote_ip    [rw-r--r--] port number for logging agent
+   | |--- remote_mac   [rw-r--r--] MAC address for logging agent
+   | +--- remote_port  [rw-r--r--] IP address for logging agent
+   |--- port2/
+   |--- port3/
+   ...
+
+If "enabled" attribute of certain port is '1', this port is used
+and the configurations of this port are uable to change.
+
+If "enabled" attribute of certain port is '0', this port isn't used
+and the configurations of this port are able to change.
+
+3.2 ioctls
+
+1. create device file
+
+First of all, we would like you to check the entry, "/proc/devices",
+in procfs because of creating device file for netconsole.
+If you can find the entry, "netcon" and that major number, create
+the device file using mknod command.
+
+2. send commands using ioctl system call
+
+You can send the following ioctl commands to the above device file.
+
+- NETCONSOLE_ADD_TARGET ioctl adds netconsole target
+
+In the case of module parameter,
+
+  netconsole=6665@10.0.0.1/eth0,6666@10.0.0.2/12:34:56:78:9a:bc
+
+In the case of ioclt command,
+
+  struct netconsole_request req = {
+          .netdev_name = "eth0",
+          .local_ip = inet_addr("10.0.0.1"),
+          .local_port = 6665,
+          .remote_ip = inet_addr("10.0.0.2"),
+          .remote_port = 6666,
+          .remote_mac = {0x12, 0x34, 0x56, 0x78, 0x9a, 0xbc},
+  };
+  ioctl(fd, NETCON_ADD_TARGET, &req)
+
+The above patterns mean the same thing.
+
+- NETCONSOLE_REMOVE_TARGET ioctl removes netconsole target.
+
+  ioctl(fd, NETCON_REMOVE_TARGET, &id)
+
+The above "id" is port number in /sys/class/misc/netconsole.
+For example, The "id" is 1 for /sys/class/misc/netconsole/port1.

-- 

Re: [RFC][PATCH -mm take5 7/7] update documentation

[Satyam Sharma]

Hi,

On 6/13/07, Keiichi KII <k-keiichi@bx.jp.nec.com> wrote:
> From: Keiichi KII <k-keiichi@bx.jp.nec.com>
>
> update Documentation/networking/netconsole.txt
>  - how to use sysfs for dynamic configurability
>  - how to use ioctl for dynamic configurability
>
> Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>

Ah, so here comes the documentation :-) Actually, I've seen a lot
of patchsets these days that prefer to submit the documentation
patch in the beginning, so that a reader / reviewer can get ready
about what to expect from the code.

Anyway, I really liked your submission. It makes netconsole
quite flexible and powerful, indeed.

Thanks,
Satyam