proposal for dynamic configurable netconsole
Subsystems (5)
cover proposal for dynamic configurable netconsole
From: Keiichi KII <[email protected]>
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.
---
RFC PATCH 1/7 marking __init
From: Keiichi KII <[email protected]>
This patch contains the following cleanups.
- add __init for initialization functions(option_setup() and init_netconsole()).
---
drivers/net/netconsole.c +2 -2
@@ -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
Quoted 9 lines · Keiichi KII
From: Keiichi KII <[email protected]>
This patch contains the following cleanups.
- add__initfor initializationfunctions(option_setup() and init_netconsole()).Acked-by Matt Mackall <[email protected]>Signed-off-by Keiichi KII <[email protected]>Signed-off-by Takayoshi Kochi <[email protected]>
Dave, please take this one. Not sure what to do about the rest, sigh.
Signature · 1 line
Mathematics is the supreme nostalgia of our time.
RFC PATCH 2/7 support multiple logging
In reply to Keiichi KIIFrom: Keiichi KII <[email protected]>
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.
---
drivers/net/netconsole.c +123 -20
@@ -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);
drivers/net/Kconfig +10 -0
@@ -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
Hi Keiichi,
drivers/net/netconsole.c static struct netpoll np = { · line 75
Quoted 8 lines · Keiichi KII
drivers/net/netconsole.c +8
-0+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.
drivers/net/netconsole.c static struct netpoll np = { · line 115
Quoted 5 lines
drivers/net/netconsole.c +5
-0+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.
drivers/net/netconsole.c static struct console netconsole = { · line 181
Quoted 5 lines
drivers/net/netconsole.c +2
-1+#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.
drivers/net/netconsole.c static struct console netconsole = {
Quoted 13 lines
drivers/net/netconsole.c +6
-4-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() andspin_unlock_irqrestore() around this code here,
and lose it from the remove_target() ...
drivers/net/netconsole.c static struct console netconsole = { · line 208
Quoted 7 lines
drivers/net/netconsole.c +7
-0+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().
drivers/net/netconsole.c static struct console netconsole = { · line 212
Quoted 4 lines
drivers/net/netconsole.c +2
-0+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.
Quoted 4 lines
drivers/net/netconsole.c +0
-4- err =netpoll_setup (&np); -if (err) -return err; -
drivers/net/netconsole.c static struct console netconsole = { · line 222
Quoted 10 lines
drivers/net/netconsole.c +10
-0+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 3/7 add interface for netconsole using sysfs
In reply to Keiichi KIIFrom: Keiichi KII <[email protected]>
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/
...
---
drivers/net/netconsole.c +206 -1
@@ -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 <[email protected]>" );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 netconskfree (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_confilist_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
Hi,
Quoted 21 lines · Keiichi KII
From: Keiichi KII <[email protected]>
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 ofnetconsole(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/
[...]
Quoted 1 line
+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?
Quoted 1 line
+static
ssize_tshow_target_attr(struct kobject *kobj,
[...]
drivers/net/netconsole.c struct netconsole_target { · line 221
Quoted 4 lines
drivers/net/netconsole.c +4
-0+if (na->show) +return na->show (nt, buffer); +else +return -EIO ;
and
Quoted 1 line
+static
ssize_tstore_target_attr(struct kobject *kobj,
[...]
drivers/net/netconsole.c struct netconsole_target { · line 235
Quoted 4 lines
drivers/net/netconsole.c +4
-0+if (na->store) +return na->store (nt, buffer, count); +else +return -EIO ;
EIO isn't quite appropriate for the above two cases. EPERM?
drivers/net/netconsole.c static void remove_target(struct netcons · line 276
Quoted 3 lines
drivers/net/netconsole.c +3
-0+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.
drivers/net/netconsole.c static void cleanup_netconsole(void) · line 391
Quoted 8 lines
drivers/net/netconsole.c +3
-1@@ -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.
drivers/net/netconsole.c static int __init init_netconsole(void) · line 412
Quoted 9 lines
drivers/net/netconsole.c +9
-0+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
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.
Quoted 4 lines
Quoted 1 line
+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
Signature · 3 lines
Keiichi KII NEC Corporation OSS Platform Development Division E-mail: [email protected]
RFC PATCH 4/7 using symlink for the net_device
In reply to Keiichi KIIFrom: Keiichi KII <[email protected]>
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/
...
---
drivers/net/netconsole.c +83 -1
@@ -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
Hi,
Quoted 2 lines · Keiichi KII
[...]
+staticDECLARE_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. ]
Review
Quoted 2 lines
(quoted diff) +2
-0+ down(&netdev_change_sem); + up(&netdev_change_sem);
So these become mutex_lock and mutex_unlock.
Quoted 17 lines
drivers/net/netconsole.c +17
-0+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); +
drivers/net/netconsole.c static int add_target(char* target_confi · line 379
Quoted 10 lines
drivers/net/netconsole.c +10
-0+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.
drivers/net/netconsole.c static int add_target(char* target_confi · line 387
Quoted 8 lines
drivers/net/netconsole.c +8
-0+ } +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 ...
Quoted 1 line
+
strcpy(nt->np.dev_name, dev->name);
... you'll have move this up.
drivers/net/netconsole.c static int add_target(char* target_confi · line 401
Quoted 13 lines
drivers/net/netconsole.c +13
-0+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
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)
Quoted 12 lines · Keiichi KII
Quoted 2 lines
[...]
+staticDECLARE_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_lockbecause it's defined just below the "target_list". It's not equally obvious at first glance what is protected by thenetdev_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):
Quoted 11 lines
drivers/net/netconsole.c static int add_target(char* target_confi · line 363
Quoted 11 lines
drivers/net/netconsole.c +11
-0+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.
Quoted 7 lines
drivers/net/netconsole.c static int add_target(char* target_confi
Quoted 7 lines
drivers/net/netconsole.c +6
-0+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.
Quoted 5 lines
drivers/net/netconsole.c static int add_target(char* target_confi
Quoted 5 lines
drivers/net/netconsole.c +4
-0+list_for_each_entry (nt, &modify_target_list, list) { + [...] + } +up (&netdev_change_sem);
So does this.
Quoted 4 lines
drivers/net/netconsole.c static int add_target(char* target_confi · line 397
Quoted 4 lines
drivers/net/netconsole.c +4
-0+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.
Quoted 8 lines
Quoted 4 lines
drivers/net/netconsole.c +4
-0+ } + +return NOTIFY_DONE ; +}drivers/net/netconsole.c +0
-0@@ -239,12 +240,14 @@ static void remove_target(struct netcons {unsigned long flags;
drivers/net/netconsole.c static void remove_target(struct netcons · line 251
Quoted 10 lines
drivers/net/netconsole.c +2
-0+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.
drivers/net/netconsole.c static struct kobj_type target_ktype = { · line 281
Quoted 5 lines
drivers/net/netconsole.c +5
-0+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?
drivers/net/netconsole.c static struct kobj_type target_ktype = { · line 288
Quoted 7 lines
drivers/net/netconsole.c +7
-0+if (!name) { +printk (KERN_ERR "netconsole: kmalloc() failed!\n" ); +return NULL ; + } + +return name; +}
And this doesn't want to be a separate function either.
drivers/net/netconsole.c static struct kobj_type target_ktype = { · line 301
Quoted 13 lines
static int
setup_target_sysfs(struct netconsole_target *nt){drivers/net/netconsole.c +7
-1+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
Hello Satyam,
Quoted 4 lines
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_listwould be nice? (BTWtemporary_listwould be a better name for that, IMO)
All right, my patches are short of comments. So, I will add comments to the ambiguous codes.
Quoted 16 lines
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_CHANGENAMEnotifier ] [ioctl(NETCON_REMOVE_TARGET)]
netconsole_event()
move fromtarget_listto temp list
work on temp listkobject_unregister() -> release_target() -> remove_target()move back to
target_list
Quoted 8 lines
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 (renamingnetdev_change_semtonetdev_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?
Quoted 7 lines
drivers/net/netconsole.c static struct kobj_type target_ktype = { · line 281
Quoted 5 lines
drivers/net/netconsole.c +5
-0+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.
Quoted 14 lines
drivers/net/netconsole.c static struct kobj_type target_ktype = { · line 301
Quoted 13 lines
static int
setup_target_sysfs(struct netconsole_target *nt){drivers/net/netconsole.c +7
-1+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 ;
Quoted 2 lines
Just call
kasprintf()directly, why the obfuscation?
I drew upon dev_change_name() method in net/core/dev.c.
Thanks
Signature · 3 lines
Keiichi KII NEC Corporation OSS Platform Development Division E-mail: [email protected]
Re: [RFC][PATCH -mm take5 4/7] using symlink for the net_device
Hi,
Quoted 35 lines · Keiichi KII
Hello Satyam,
Quoted 4 lines
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_listwould be nice? (BTWtemporary_listwould be a better name for that, IMO)All right, my patches are short of comments. So, I will add comments to the ambiguous codes.
Quoted 16 lines
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_CHANGENAMEnotifier ] [ioctl(NETCON_REMOVE_TARGET)]
netconsole_event()
move fromtarget_listto temp list
work on temp listkobject_unregister() -> release_target() -> remove_target()move back to
target_listQuoted 8 lines
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 (renamingnetdev_change_semtonetdev_changename_mtx):
Quoted 4 lines
After the target moves from
target_listtotemporary_list, thekobject_unregister()of possible raced target isn't called inioctl(NETCON_REMOVE_TARGET)because thetarget_listdoesn'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.
Quoted 11 lines
Quoted 7 lines
drivers/net/netconsole.c static struct kobj_type target_ktype = { · line 281
Quoted 5 lines
drivers/net/netconsole.c +5
-0+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 innet/core/dev.c. Thedevice_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
Hello Satyam,
Quoted 9 lines
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.
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
drivers/net/netconsole.c static struct kobj_type target_ktype = { · line 281
Quoted 5 lines
drivers/net/netconsole.c +5
-0+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?
Quoted 8 lines
Quoted 3 lines
Because I drew upon
dev_change_name()method innet/core/dev.c. Thedevice_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 overkasprintf()like themake_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
Signature · 3 lines
Keiichi KII NEC Corporation OSS Platform Development Division E-mail: [email protected]
Re: [RFC][PATCH -mm take5 4/7] using symlink for the net_device
In reply to Satyam SharmaHello Satyam,
Quoted 5 lines
Sorry, but we're not covering from the error condition fully here. Note that later you merge the temporary
modify_target_listentirely back into thetarget_list... which would still contain these erroneous nodes. A full cleanup (kobject_unregisterthe entry, and thenlist_delfrommodify_target_list) is required here, before continuing.
I will fix this. If the error occurs, I think so that we need to cleanup completely.
Quoted 4 lines
Quoted 1 line
+
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
Signature · 3 lines
Keiichi KII NEC Corporation OSS Platform Development Division E-mail: [email protected]
Re: [RFC][PATCH -mm take5 4/7] using symlink for the net_device
Hi Keiichi,
Quoted 10 lines · Keiichi KII
Hello Satyam,
Quoted 5 lines
Sorry, but we're not covering from the error condition fully here. Note that later you merge the temporary
modify_target_listentirely back into thetarget_list... which would still contain these erroneous nodes. A full cleanup (kobject_unregisterthe entry, and thenlist_delfrommodify_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.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
+
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 5/7 switch function of netpoll
In reply to Keiichi KIIFrom: Keiichi KII <[email protected]>
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/
...
---
drivers/net/netconsole.c +42 -2
@@ -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 nstatic 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(structstatic 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 netstatic 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 *cofor (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
Hi Keiichi,
Quoted 27 lines · Keiichi KII
From: Keiichi KII <[email protected]>
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 <[email protected]>Signed-off-by Takayoshi Kochi <[email protected]>---
Index:mm/drivers/net/netconsole.c
===================================================================
drivers/net/netconsole.c struct netconsole_target { · line 76
Quoted 6 lines
drivers/net/netconsole.c +1
-0@@ -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 6/7 add ioctls for adding/removing target
In reply to Keiichi KIIFrom: Keiichi KII <[email protected]>
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.
---
/*
* 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 <[email protected]>
* 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;
}fs/compat_ioctl.c +8 -0
@@ -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
include/linux/netconsole.h new file +18 -0
@@ -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 */
drivers/net/netconsole.c +94 -0
130 lines not on this page · raw message
Re: [RFC][PATCH -mm take5 6/7] add ioctls for adding/removing target
Hi Keiichi,
Quoted 7 lines · Keiichi KII
From: Keiichi KII <[email protected]>
We add ioctls for adding/removing target. If we use
NETCONSOLE_ADD_TARGETioctl, we can dynamically add netconsole target. If we useNETCONSOLE_REMOVE_TARGETioctl, 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
Hello Satyam,
Quoted 12 lines
*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:
Quoted 7 lines
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 (orconfig_itemsin 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.
Quoted 11 lines
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
Signature · 3 lines
Keiichi KII NEC Corporation OSS Platform Development Division E-mail: [email protected]
Re: [RFC][PATCH -mm take5 6/7] add ioctls for adding/removing target
Hi again Keiichi,
Quoted 32 lines · Keiichi KII
Hello Satyam,
Quoted 12 lines
*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:
Quoted 7 lines
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 (orconfig_itemsin 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.
Quoted 7 lines
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.
Quoted 7 lines
Quoted 4 lines
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
Hello Satyam,
Quoted 7 lines
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.
Quoted 8 lines
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
Signature · 3 lines
Keiichi KII NEC Corporation OSS Platform Development Division E-mail: [email protected]
Re: [RFC][PATCH -mm take5 6/7] add ioctls for adding/removing target
Hi Keiichi,
Quoted 9 lines
Quoted 8 lines
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.
Quoted 3 lines
It was for informational pupose. But, if we used symlink to the
net_devicekobject 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, ...);
}Quoted 3 lines
In the case of configfs, Do we use
config_itemrelated to the network interface because the configfs doesn't have symlink that refers tonet_devicekobject (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 7/7 update documentation
In reply to Keiichi KIIFrom: Keiichi KII <[email protected]>
update Documentation/networking/netconsole.txt
- how to use sysfs for dynamic configurability
- how to use ioctl for dynamic configurability
---
Documentation/networking/netconsole.txt +109 -2
@@ -4,6 +4,9 @@ started by Ingo Molnar <[email protected] Please send bug reports to Matt Mackall <[email protected]> +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="@/,[email protected]/;@/,[email protected]/" + 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, + + [email protected]/eth0,[email protected]/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
Hi,
Quoted 7 lines · Keiichi KII
From: Keiichi KII <[email protected]>
update
Documentation/networking/netconsole.txt
- how to use sysfs for dynamic configurability
- how to use ioctl for dynamic configurabilitySigned-off-by Keiichi KII <[email protected]>
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