From: Satyam Sharma <redacted>
[0/9] netconsole: Multiple targets and dynamic reconfigurability
This patchset is a rework of the original idea and patches posted by
Keiichi Kii and Takayoshi Kochi at: http://lkml.org/lkml/2007/6/13/72
[ I had initially thought about only modifying Keiichi's patches to
incorporate the suggested stuff, sysfs+ioctl->configfs conversion, etc,
but the changes became too much and I ultimately ended up rewriting the
patchset. This also gave the flexibility to make other changes possible.
Keiichi, Takayoshi, kindly review this, and if OK, please give your
Signed-off-by:'s to the patches herein. ]
This patchset depends upon the earlier netpoll fixlet and the configfs
cleanup patches. Diffed against 2.6.22-rc6-mm1.
[1/9] netconsole: Cleanups, codingstyle, prettyfication
[2/9] netconsole: Code simplification
[3/9] netconsole: Introduce netconsole_target
[4/9] netconsole: Introduce netconsole_netdev_notifier
[5/9] netconsole: Introduce dev_status member
[6/9] netconsole: Update documentation for multiple target support
[7/9] netconsole: Support multiple logging targets
[8/9] netconsole: Update documentation for dynamic reconfigurability
[9/9] netconsole: Support dynamic reconfiguration using configfs
Signed-off-by: Satyam Sharma <redacted>
About this patchset:
====================
What?
Support multiple remote logging targets in netconsole. Also, ability to
dynamically add or remove targets or modify parameters (IP, port, remote
MAC address) of targets at runtime, from userspace, using configfs.
Why?
From Keiichi Kii's original post:
[...] current netconsole is not flexible. For example, if you want to change
ip address for logging agent, in the case of built-in netconsole, you can't
change config except for changing boot parameter and rebooting your system,
or in the case of module netconsole, you need to remove it and reload
with different parameters. [...] and we have been losing serial console
port with PCs and Servers.
(... and especially laptops, I would add.)
The patchset is heavily commented and verbosely changelogged, so hope you
have as much fun reading and reviewing it as I had writing it :-)
Kindly review and comment.
Satyam
From: Satyam Sharma <redacted>
[6/9] netconsole: Update documentation for multiple target support
... and add a few useful general purpose tips as well while we're at it.
Signed-off-by: Satyam Sharma <redacted>
Cc: Keiichi Kii <redacted>
Cc: Takayoshi Kochi <redacted>
---
Documentation/networking/netconsole.txt | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
---
diff -ruNp a/Documentation/networking/netconsole.txt b/Documentation/networking/netconsole.txt
@@ -34,6 +34,12 @@ Examples: insmod netconsole netconsole=@/,@10.0.0.2/+It also supports logging to multiple remote agents by specifying+parameters for the multiple agents separated by semicolons and the+complete string enclosed in "quotes", thusly:++modprobe netconsole netconsole="@/,@10.0.0.2/;@/eth1,6892@10.0.0.3/"+ Built-in netconsole starts immediately after the TCP stack is initialized and attempts to bring up the supplied dev at the supplied address.
@@ -44,6 +50,18 @@ WARNING: the default target ethernet set ethernet address to send packets, which can cause increased load on other systems on the same ethernet segment.+TIP: some LAN switches may be configured to suppress ethernet broadcasts+so it is advised to explicitly specify the remote agents' MAC addresses+from the config parameters passed to netconsole.++TIP: to find out the MAC address of 10.0.0.2, you may try using:+ping -c 1 10.0.0.2 ; /sbin/arp -n | grep 10.0.0.2++TIP: in case the remote logging agent is on a separate LAN subnet than+the sender, it is suggested to try specifying the default gateway's+(you may use /sbin/route -n to find out) MAC address as the remote MAC+address instead.+ NOTE: the network device (eth1 in the above case) can run any kind of other network traffic, netconsole is not intrusive. Netconsole might cause slight delays in other traffic if the volume of kernel
From: Satyam Sharma <redacted>
[2/9] netconsole: Code simplification
(1) Extract netpoll_parse_options() out of option_setup(), and into
init_netconsole() itself. So "configured" variable is redundant and
can be removed.
(2) With this change, option_setup() is not required for modular netconsole.
(3) The (!np.dev) check in write_msg() is bogus (always false), because:
np.dev is set by netpoll_setup(), which is called by the target init
code in init_netconsole() _before_ register_console() => write_msg() cannot
be triggered unless netpoll_setup() returns with success. And that will not
happen if netpoll_setup() failed to set np.dev. Also np.dev cannot go from
under us while netconsole is loaded. This is because netpoll_setup() grabs
a reference for us on that dev. So let's remove the pointless check.
Signed-off-by: Satyam Sharma <redacted>
Cc: Keiichi Kii <redacted>
Cc: Takayoshi Kochi <redacted>
---
drivers/net/netconsole.c | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)
---
diff -ruNp a/drivers/net/netconsole.c b/drivers/net/netconsole.c
From: Satyam Sharma <redacted>
[9/9] netconsole: Support dynamic reconfiguration using configfs
This patch introduces support for dynamic reconfiguration (adding, removing
and/or modifying parameters of netconsole targets at runtime) using a
userspace interface exported via configfs.
Issues and brief design overview:
(1) Kernel-initiated creation / destruction of kernel objects is not
possible with configfs -- the lifetimes of the "config items" is managed
exclusively from userspace. But netconsole must support boot/module params
too, and these are parsed in kernel and hence netpolls must be setup from
the kernel. I initially thought of extending configfs with this functionality
(item creation/destroy by kernel subsystems) but Joel Becker pointed me to
the locking and refcounting complexities such a project would entail.
After more discussion, Joel suggested the idea to separately manage the
lifetimes of the two kinds of netconsole_target objects, those created while
parsing boot/module options and those created via configfs mkdir(2) from
userspace, in my driver itself. That adds a little bit of complexity and
redundancy (multiple item creation and destruction points) in this driver.
Note that the latter type (configfs-created netconsole_targets) can be
modified / updated / destroyed at runtime from userspace but the former
(param string created) cannot, as they are not exposed through our configfs
namespace. However, this is not really a problem and is similar to current
behaviour in any case.
(2) Enhancing the semantics of the "enabled" attribute. In the original
patchset submitted, "enabled" was just an on / off knob to switch logging
to particular targets on or off in the console->write() function itself.
But there is a problem. The original patchset had interface device name
as a read-only attribute and assumed ioctl()'s were to be used to create
new targets. This worked only because ioctl()'s deal with full structures
and we can fill out the structure members (including local interface) in
the special userspace program that implements that ioctl(). But configfs
does this with simple mkdir's that can be run off the shell -- and the
corresponding kernel object is created as result of the mkdir(2) call
chain in our driver subsystem. But at that point we don't really know
what interface device should the netpoll be attached.
So, let's solve it this way: newly-created netconsole_target's are not
automatically enabled, and their parameters are not parsed (and the
netpoll_setup() not called). Then, the user may set values to the
various attributes, ending with setting "1" to "enabled" itself. The
netpoll_setup() is then called on the set parameters in the context of
_this_ write(2) on the "enabled" attribute.
The upside of this solution is that existing netconsole_targets can be
reconfigured to be attached to newly-come-up interfaces, that might not
have even existed when netconsole was loaded, or even when the targets
were originally created. Also, we are able to completely get rid of the
custom ioctl()'s based solution.
(3) Ultra-paranoid configfs attribute show() and store() operations, with
sanity and input range checking, using only safe string primitives, and
compliant with the recommendations in Documentation/filesystems/sysfs.txt.
Signed-off-by: Satyam Sharma <redacted>
Cc: Keiichi Kii <redacted>
Cc: Takayoshi Kochi <redacted>
---
drivers/net/Kconfig | 10
drivers/net/netconsole.c | 601 +++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 596 insertions(+), 15 deletions(-)
---
diff -ruNp a/drivers/net/Kconfig b/drivers/net/Kconfig
@@ -41,6 +41,8 @@#include<linux/moduleparam.h>#include<linux/string.h>#include<linux/netpoll.h>+#include<linux/inet.h>+#include<linux/configfs.h>MODULE_AUTHOR("Maintainer: Matt Mackall <mpm@selenic.com>");MODULE_DESCRIPTION("Console driver for network interfaces");
@@ -69,6 +71,9 @@ static LIST_HEAD(target_list);/* This needs to be a spinlock because write_msg() cannot sleep */staticDEFINE_SPINLOCK(target_list_lock);+/* Target ids. Only for information, we don't care about wrap-wround */+staticatomic_tmonotonic_count=ATOMIC_INIT(0);+/**Whynonet_dev_is_up()innetdevice.h?Thekernelcouldlosealotof*weightifonlynetdevice.hhadthegoodsensetoexportsuchafunction.
@@ -82,25 +87,88 @@ static inline int net_dev_is_up(struct n/***structnetconsole_target-Representsaconfigurednetconsoletarget.*@list:Linksthistargetintothetarget_list.+*@item:Linksusintotheconfigfssubsystemhierarchy.+*@id:Uniqueidentifierforthistarget.+*Visiblefromuserspace(read-only).+*@enabled:On/offknobtoenable/disabletarget.+*Visiblefromuserspace(read-write).+*Notethatwemaintainastrict,reliable1:1correspondence+*betweenthisandwhethernetpollisactive/inactive.+*Also,otherparametersofatargetmaybemodifiedat+*runtimeonlywhenitisdisabled(enabled=0).*@dev_status:Trackswhethertheunderlyinginterfaceisupordown.+*Notvisiblefromuserspace.*@np:Thenetpollstructureforthistarget.+*Containstheotheruserspacevisibleparameters,namely:+*dev_name(read-write)+*local_ip(read-write)+*remote_ip(read-write)+*local_port(read-write)+*remote_port(read-write)+*local_mac(read-only)+*remote_mac(read-write)*/structnetconsole_target{structlist_headlist;+structconfig_itemitem;+intid;+intenabled;intdev_status;structnetpollnp;};+#ifdef CONFIG_NETCONSOLE_DYNAMIC++/*+*Targetsthatwerecreatedbyparsingtheboot/moduleoptionstring+*donotexistintheconfigfshierarchyandwillnevergoaway(and+*havezeroed-outconfig_itemmembers).Somaketheseano-opforthem.+*/+staticvoidnetconsole_target_get(structnetconsole_target*nt)+{+staticstructconfig_itemempty_item;/* Zeroed-out config_item */++if(memcmp(&nt->item,&empty_item,sizeof(structconfig_item)))+config_item_get(&nt->item);+}++staticvoidnetconsole_target_put(structnetconsole_target*nt)+{+staticstructconfig_itemempty_item;/* Zeroed-out config_item */++if(memcmp(&nt->item,&empty_item,sizeof(structconfig_item)))+config_item_put(&nt->item);+}++#else /* !CONFIG_NETCONSOLE_DYNAMIC */+/*-*Allocatenewtargetandsetupnetpollforit.+*Nodangeroftargetsgoingawayfromunderuswhendynamic+*reconfigurabilityisoff.+*/+staticvoidnetconsole_target_get(structnetconsole_target*nt)+{+}++staticvoidnetconsole_target_put(structnetconsole_target*nt)+{+}++#endif /* CONFIG_NETCONSOLE_DYNAMIC */++/*+*Allocatenewtarget(fromboot/moduleparam)andsetupnetpollforit.*Callermustthenaddthistotarget_listitself.*/-staticstructnetconsole_target*alloc_target(char*target_config)+staticstructnetconsole_target*alloc_param_target(char*target_config){interr=-ENOMEM;structnetconsole_target*nt;-/* Allocate and initialize with defaults */+/*+*Allocateandinitializewithdefaults.+*Notethatthesetargetsgettheirconfig_itemfieldszeroed-out.+*/nt=kzalloc(sizeof(*nt),GFP_KERNEL);if(!nt){printk(KERN_ERR"netconsole: failed to allocate memory\n");
@@ -132,15 +202,485 @@ fail:}/*-*Cleanupnetpollforgiventargetandfreeit.+*Cleanupnetpollforgiventarget(fromboot/moduleparam)andfreeit.*Callermusthaveremovedthisfromtarget_listalready.*/-staticvoidfree_target(structnetconsole_target*nt)+staticvoidfree_param_target(structnetconsole_target*nt){netpoll_cleanup(&nt->np);kfree(nt);}+#ifdef CONFIG_NETCONSOLE_DYNAMIC++/*+*Oursubsystemhierarchyis:+*+*/config/netconsole/+*|+*<target>/+*|id+*|enabled+*|dev_name+*|local_port+*|remote_port+*|local_ip+*|remote_ip+*|local_mac+*|remote_mac+*|+*<target>/...+*/++structnetconsole_target_attr{+structconfigfs_attributeattr;+ssize_t(*show)(structnetconsole_target*nt,+char*buf);+ssize_t(*store)(structnetconsole_target*nt,+constchar*buf,+size_tcount);+};++staticstructnetconsole_target*to_target(structconfig_item*item)+{+returnitem?+container_of(item,structnetconsole_target,item):+NULL;+}++/*+*Wrapperoversimple_strtol(base10)withsanityandrangechecking.+*Wereturn(signed)longonlybecausewemaywanttoreturnerrors.+*Thus,donotusethistoconvertnumbersthatareallowedtobenegative.+*/+staticlongstrtol10_check_range(constchar*cp,longmin,longmax)+{+longret;+char*p=(char*)cp;++WARN_ON(min<0);+WARN_ON(max<min);++ret=simple_strtol(p,&p,10);++if(*p&&(*p!='\n')){+printk(KERN_ERR"netconsole: invalid input\n");+return-EINVAL;+}+if((ret<min)||(ret>max)){+printk(KERN_ERR"netconsole: input %ld must be between "+"%ld and %ld\n",ret,min,max);+return-EINVAL;+}++returnret;+}++/*+*Attributeoperationsfornetconsole_target.+*/++staticssize_tshow_id(structnetconsole_target*nt,char*buf)+{+returnsnprintf(buf,PAGE_SIZE,"%d\n",nt->id);+}++staticssize_tshow_enabled(structnetconsole_target*nt,char*buf)+{+returnsnprintf(buf,PAGE_SIZE,"%d\n",nt->enabled);+}++staticssize_tshow_dev_name(structnetconsole_target*nt,char*buf)+{+returnsnprintf(buf,PAGE_SIZE,"%s\n",nt->np.dev_name);+}++staticssize_tshow_local_ip(structnetconsole_target*nt,char*buf)+{+returnsnprintf(buf,PAGE_SIZE,"%d.%d.%d.%d\n",+HIPQUAD(nt->np.local_ip));+}++staticssize_tshow_remote_ip(structnetconsole_target*nt,char*buf)+{+returnsnprintf(buf,PAGE_SIZE,"%d.%d.%d.%d\n",+HIPQUAD(nt->np.remote_ip));+}++staticssize_tshow_local_port(structnetconsole_target*nt,char*buf)+{+returnsnprintf(buf,PAGE_SIZE,"%d\n",nt->np.local_port);+}++staticssize_tshow_remote_port(structnetconsole_target*nt,char*buf)+{+returnsnprintf(buf,PAGE_SIZE,"%d\n",nt->np.remote_port);+}++staticssize_tshow_local_mac(structnetconsole_target*nt,char*buf)+{+returnsnprintf(buf,PAGE_SIZE,"%02x:%02x:%02x:%02x:%02x:%02x\n",+nt->np.local_mac[0],nt->np.local_mac[1],+nt->np.local_mac[2],nt->np.local_mac[3],+nt->np.local_mac[4],nt->np.local_mac[5]);+}++staticssize_tshow_remote_mac(structnetconsole_target*nt,char*buf)+{+returnsnprintf(buf,PAGE_SIZE,"%02x:%02x:%02x:%02x:%02x:%02x\n",+nt->np.remote_mac[0],nt->np.remote_mac[1],+nt->np.remote_mac[2],nt->np.remote_mac[3],+nt->np.remote_mac[4],nt->np.remote_mac[5]);+}++/*+*Thisone'sspecialandimportant--targetscreatedthroughthe+*configfsinterfacearenotenabled/netpollactivatedbydefault.+*Theuserisexpectedtosetthedesiredparametersfirst(which+*wouldenablehimtodynamicallyaddnewnetpolltargetsfornew+*networkinterfacesasandwhentheycomeup).+*/+staticssize_tstore_enabled(structnetconsole_target*nt,+constchar*buf,+size_tcount)+{+interr;+longenabled;+structnetpoll*np=&nt->np;++enabled=strtol10_check_range(buf,0,1);+if(enabled<0)+returnenabled;++if(enabled){/* 1 */++/*+*Skipnetpoll_parse_options()--alltheattributesare+*alreadyconfiguredinnt->npthroughconfigfs.Butat+*leastlet'sprinttheusefulstuffitusedtooutput:-)+*/+printk(KERN_INFO"%s: local port %d\n",+np->name,np->local_port);+printk(KERN_INFO"%s: local IP %d.%d.%d.%d\n",+np->name,HIPQUAD(np->local_ip));+printk(KERN_INFO"%s: interface %s\n",+np->name,np->dev_name);+printk(KERN_INFO"%s: remote port %d\n",+np->name,np->remote_port);+printk(KERN_INFO"%s: remote IP %d.%d.%d.%d\n",+np->name,HIPQUAD(np->remote_ip));+printk(KERN_INFO"%s: remote ethernet address "+"%02x:%02x:%02x:%02x:%02x:%02x\n",+np->name,+np->remote_mac[0],np->remote_mac[1],+np->remote_mac[2],np->remote_mac[3],+np->remote_mac[4],np->remote_mac[5]);++err=netpoll_setup(np);+if(err)+returnerr;++nt->dev_status=net_dev_is_up(np->dev);+printk(KERN_INFO"netconsole: network logging started\n");++}else{/* 0 */+netpoll_cleanup(np);+}++nt->enabled=enabled;++returnstrnlen(buf,count);+}++staticssize_tstore_dev_name(structnetconsole_target*nt,+constchar*buf,+size_tcount)+{+if(nt->enabled){+printk(KERN_ERR"netconsole: target (id %d) is enabled, "+"disable to update parameters\n",nt->id);+return-EINVAL;+}++strlcpy(nt->np.dev_name,buf,IFNAMSIZ);++returnstrnlen(buf,count);+}++staticssize_tstore_local_port(structnetconsole_target*nt,+constchar*buf,+size_tcount)+{+longlocal_port;+#define __U16_MAX ((__u16) ~0U)++if(nt->enabled){+printk(KERN_ERR"netconsole: target (id %d) is enabled, "+"disable to update parameters\n",nt->id);+return-EINVAL;+}++local_port=strtol10_check_range(buf,0,__U16_MAX);+if(local_port<0)+returnlocal_port;++nt->np.local_port=local_port;++returnstrnlen(buf,count);+}++staticssize_tstore_remote_port(structnetconsole_target*nt,+constchar*buf,+size_tcount)+{+longremote_port;+#define __U16_MAX ((__u16) ~0U)++if(nt->enabled){+printk(KERN_ERR"netconsole: target (id %d) is enabled, "+"disable to update parameters\n",nt->id);+return-EINVAL;+}++remote_port=strtol10_check_range(buf,0,__U16_MAX);+if(remote_port<0)+returnremote_port;++nt->np.remote_port=remote_port;++returnstrnlen(buf,count);+}++staticssize_tstore_local_ip(structnetconsole_target*nt,+constchar*buf,+size_tcount)+{+if(nt->enabled){+printk(KERN_ERR"netconsole: target (id %d) is enabled, "+"disable to update parameters\n",nt->id);+return-EINVAL;+}++nt->np.local_ip=ntohl(in_aton(buf));++returnstrnlen(buf,count);+}++staticssize_tstore_remote_ip(structnetconsole_target*nt,+constchar*buf,+size_tcount)+{+if(nt->enabled){+printk(KERN_ERR"netconsole: target (id %d) is enabled, "+"disable to update parameters\n",nt->id);+return-EINVAL;+}++nt->np.remote_ip=ntohl(in_aton(buf));++returnstrnlen(buf,count);+}++staticssize_tstore_remote_mac(structnetconsole_target*nt,+constchar*buf,+size_tcount)+{+u8remote_mac[ETH_ALEN];+char*p=(char*)buf;+inti;++if(nt->enabled){+printk(KERN_ERR"netconsole: target (id %d) is enabled, "+"disable to update parameters\n",nt->id);+return-EINVAL;+}++for(i=0;i<ETH_ALEN-1;i++){+remote_mac[i]=simple_strtoul(p,&p,16);+if(*p!=':')+gotoinvalid;+p++;+}+remote_mac[ETH_ALEN-1]=simple_strtoul(p,&p,16);+if(*p&&(*p!='\n'))+gotoinvalid;++memcpy(nt->np.remote_mac,remote_mac,ETH_ALEN);++returnstrnlen(buf,count);++invalid:+printk(KERN_ERR"netconsole: invalid input\n");+return-EINVAL;+}++/*+*Attributedefinitionsfornetconsole_target.+*/++#define NETCONSOLE_TARGET_ATTR_RO(_name) \+staticstructnetconsole_target_attrnetconsole_target_##_name=\+__CONFIGFS_ATTR(_name,S_IRUGO,show_##_name,NULL)++#define NETCONSOLE_TARGET_ATTR_RW(_name) \+staticstructnetconsole_target_attrnetconsole_target_##_name=\+__CONFIGFS_ATTR(_name,S_IRUGO|S_IWUSR,show_##_name,store_##_name)++NETCONSOLE_TARGET_ATTR_RO(id);+NETCONSOLE_TARGET_ATTR_RW(enabled);+NETCONSOLE_TARGET_ATTR_RW(dev_name);+NETCONSOLE_TARGET_ATTR_RW(local_port);+NETCONSOLE_TARGET_ATTR_RW(remote_port);+NETCONSOLE_TARGET_ATTR_RW(local_ip);+NETCONSOLE_TARGET_ATTR_RW(remote_ip);+NETCONSOLE_TARGET_ATTR_RO(local_mac);+NETCONSOLE_TARGET_ATTR_RW(remote_mac);++staticstructconfigfs_attribute*netconsole_target_attrs[]={+&netconsole_target_id.attr,+&netconsole_target_enabled.attr,+&netconsole_target_dev_name.attr,+&netconsole_target_local_port.attr,+&netconsole_target_remote_port.attr,+&netconsole_target_local_ip.attr,+&netconsole_target_remote_ip.attr,+&netconsole_target_local_mac.attr,+&netconsole_target_remote_mac.attr,+NULL,+};++/*+*Itemoperationsandtypefornetconsole_target.+*/++staticvoidnetconsole_target_release(structconfig_item*item)+{+kfree(to_target(item));+}++staticssize_tnetconsole_target_attr_show(structconfig_item*item,+structconfigfs_attribute*attr,+char*buf)+{+ssize_tret=-EINVAL;+structnetconsole_target*nt=to_target(item);+structnetconsole_target_attr*na=+container_of(attr,structnetconsole_target_attr,attr);++if(na->show)+ret=na->show(nt,buf);++returnret;+}++staticssize_tnetconsole_target_attr_store(structconfig_item*item,+structconfigfs_attribute*attr,+constchar*buf,+size_tcount)+{+ssize_tret=-EINVAL;+structnetconsole_target*nt=to_target(item);+structnetconsole_target_attr*na=+container_of(attr,structnetconsole_target_attr,attr);++if(na->store)+ret=na->store(nt,buf,count);++returnret;+}++staticstructconfigfs_item_operationsnetconsole_target_item_ops={+.release=netconsole_target_release,+.show_attribute=netconsole_target_attr_show,+.store_attribute=netconsole_target_attr_store,+};++staticstructconfig_item_typenetconsole_target_type={+.ct_attrs=netconsole_target_attrs,+.ct_item_ops=&netconsole_target_item_ops,+.ct_owner=THIS_MODULE,+};++/*+*Groupoperationsandtypefornetconsole_subsys.+*/++staticstructconfig_item*make_netconsole_target(structconfig_group*group,+constchar*name)+{+unsignedlongflags;+structnetconsole_target*nt;++/*+*Allocateandinitializewithdefaults.+*Notethat->enabledand->dev_statuswouldbezero.+*/+nt=kzalloc(sizeof(*nt),GFP_KERNEL);+if(!nt){+printk(KERN_ERR"netconsole: failed to allocate memory\n");+returnNULL;+}++nt->np.name="netconsole";+strlcpy(nt->np.dev_name,"eth0",IFNAMSIZ);+nt->np.local_port=6665;+nt->np.remote_port=6666;+memset(nt->np.remote_mac,0xff,ETH_ALEN);++/* Get an id, fill out config_item and attach us to the filesystem */+nt->id=atomic_inc_return(&monotonic_count);+config_item_init_type_name(&nt->item,name,&netconsole_target_type);++/* Adding ourselves, but netpoll is not active and we're disabled */+spin_lock_irqsave(&target_list_lock,flags);+list_add(&nt->list,&target_list);+spin_unlock_irqrestore(&target_list_lock,flags);++return&nt->item;+}++staticvoiddrop_netconsole_target(structconfig_group*group,+structconfig_item*item)+{+unsignedlongflags;+structnetconsole_target*nt=to_target(item);++spin_lock_irqsave(&target_list_lock,flags);+list_del(&nt->list);+spin_unlock_irqrestore(&target_list_lock,flags);++/*+*Thetargetmayhaveneverbeenenabled,orwasmanuallydisabled+*beforebeingremovedsonetpollmayhavealreadybeencleanedup.+*/+if(nt->enabled)+netpoll_cleanup(&nt->np);++config_item_put(&nt->item);+}++staticstructconfigfs_group_operationsnetconsole_subsys_group_ops={+.make_item=make_netconsole_target,+.drop_item=drop_netconsole_target,+};++staticstructconfig_item_typenetconsole_subsys_type={+.ct_group_ops=&netconsole_subsys_group_ops,+.ct_owner=THIS_MODULE,+};++/* The netconsole configfs subsystem */+staticstructconfigfs_subsystemnetconsole_subsys={+.su_group={+.cg_item={+.ci_namebuf="netconsole",+.ci_type=&netconsole_subsys_type,+},+},+};++#endif /* CONFIG_NETCONSOLE_DYNAMIC */+/* Handle network interface device notifications */staticintnetconsole_netdev_event(structnotifier_block*this,unsignedlongevent,
@@ -156,6 +696,7 @@ static int netconsole_netdev_event(strucspin_lock_irqsave(&target_list_lock,flags);list_for_each_entry(nt,&target_list,list){+netconsole_target_get(nt);if(nt->np.dev==dev){switch(event){caseNETDEV_UP:
@@ -172,6 +713,7 @@ static int netconsole_netdev_event(strucbreak;}}+netconsole_target_put(nt);}spin_unlock_irqrestore(&target_list_lock,flags);
From: Satyam Sharma <redacted>
[7/9] netconsole: Support multiple logging targets
This patch introduces support for multiple targets:
Let's keep this out of CONFIG_NETCONSOLE_DYNAMIC as well -- this is useful
even in the default case and (including the infrastructure introduced in
previous patches) doesn't really add too many bytes to module text. All the
complexity (and size) comes with the dynamic reconfigurability / userspace
interface patch, and so it's plausible users may want to keep this enabled
but that disabled (say to avoid a dep on CONFIG_CONFIGFS_FS).
Brief overview:
We maintain a target_list (and corresponding lock). Get rid of the static
"default_target" and introduce allocation and release functions for our
netconsole_target objects (but keeping sure to preserve previous behaviour
such as default values). During init_netconsole(), ";" is used as the
separator to identify multiple target specifications in the input boot/module
option string, and sent off to netpoll_parse_options(), and then finally
netpoll's are setup. During exit, the target_list is torn down and all items
released.
Signed-off-by: Satyam Sharma <redacted>
Cc: Keiichi Kii <redacted>
Cc: Takayoshi Kochi <redacted>
---
drivers/net/netconsole.c | 193 +++++++++++++++++++++++++++++++++++------------
1 file changed, 145 insertions(+), 48 deletions(-)
---
diff -ruNp a/drivers/net/netconsole.c b/drivers/net/netconsole.c
@@ -63,6 +63,12 @@ static int __init option_setup(char *opt__setup("netconsole=",option_setup);#endif+/* Linked list of all configured targets */+staticLIST_HEAD(target_list);++/* This needs to be a spinlock because write_msg() cannot sleep */+staticDEFINE_SPINLOCK(target_list_lock);+/**Whynonet_dev_is_up()innetdevice.h?Thekernelcouldlosealotof*weightifonlynetdevice.hhadthegoodsensetoexportsuchafunction.
@@ -75,52 +81,99 @@ static inline int net_dev_is_up(struct n/***structnetconsole_target-Representsaconfigurednetconsoletarget.+*@list:Linksthistargetintothetarget_list.*@dev_status:Trackswhethertheunderlyinginterfaceisupordown.*@np:Thenetpollstructureforthistarget.*/structnetconsole_target{+structlist_headlist;intdev_status;structnetpollnp;};-staticstructnetconsole_targetdefault_target={-.np={-.name="netconsole",-.dev_name="eth0",-.local_port=6665,-.remote_port=6666,-.remote_mac={0xff,0xff,0xff,0xff,0xff,0xff},-},-};+/*+*Allocatenewtargetandsetupnetpollforit.+*Callermustthenaddthistotarget_listitself.+*/+staticstructnetconsole_target*alloc_target(char*target_config)+{+interr=-ENOMEM;+structnetconsole_target*nt;++/* Allocate and initialize with defaults */+nt=kzalloc(sizeof(*nt),GFP_KERNEL);+if(!nt){+printk(KERN_ERR"netconsole: failed to allocate memory\n");+gotofail;+}++nt->np.name="netconsole";+strlcpy(nt->np.dev_name,"eth0",IFNAMSIZ);+nt->np.local_port=6665;+nt->np.remote_port=6666;+memset(nt->np.remote_mac,0xff,ETH_ALEN);++/* Parse parameters and setup netpoll */+err=netpoll_parse_options(&nt->np,target_config);+if(err)+gotofail;++err=netpoll_setup(&nt->np);+if(err)+gotofail;++nt->dev_status=net_dev_is_up(nt->np.dev);++returnnt;++fail:+kfree(nt);+returnERR_PTR(err);+}++/*+*Cleanupnetpollforgiventargetandfreeit.+*Callermusthaveremovedthisfromtarget_listalready.+*/+staticvoidfree_target(structnetconsole_target*nt)+{+netpoll_cleanup(&nt->np);+kfree(nt);+}/* Handle network interface device notifications */staticintnetconsole_netdev_event(structnotifier_block*this,unsignedlongevent,void*ptr){+unsignedlongflags;+structnetconsole_target*nt;structnet_device*dev=ptr;-structnetconsole_target*nt=&default_target;if(!(event==NETDEV_UP||event==NETDEV_DOWN||event==NETDEV_CHANGEADDR||event==NETDEV_CHANGENAME))-gotodone;+gotodone;-if(nt->np.dev==dev){-switch(event){-caseNETDEV_UP:-caseNETDEV_DOWN:-nt->dev_status=net_dev_is_up(nt->np.dev);-break;--caseNETDEV_CHANGEADDR:-memcpy(nt->np.local_mac,dev->dev_addr,ETH_ALEN);-break;--caseNETDEV_CHANGENAME:-strlcpy(nt->np.dev_name,dev->name,IFNAMSIZ);-break;+spin_lock_irqsave(&target_list_lock,flags);+list_for_each_entry(nt,&target_list,list){+if(nt->np.dev==dev){+switch(event){+caseNETDEV_UP:+caseNETDEV_DOWN:+nt->dev_status=net_dev_is_up(nt->np.dev);+break;++caseNETDEV_CHANGEADDR:+memcpy(nt->np.local_mac,dev->dev_addr,ETH_ALEN);+break;++caseNETDEV_CHANGENAME:+strlcpy(nt->np.dev_name,dev->name,IFNAMSIZ);+break;+}}}+spin_unlock_irqrestore(&target_list_lock,flags);done:returnNOTIFY_DONE;
From: Satyam Sharma <redacted>
[3/9] netconsole: Introduce netconsole_target
Introduce a wrapper structure over netpoll to represent targets
configured in netconsole. This will get extended with other members in
further patches.
Ok, so the original patchset did this along with (and inside the #ifdef)
of CONFIG_NETCONSOLE_DYNAMIC itself. I decided otherwise, and was able
to drastically cut down on the #ifdef-complexity of final netconsole.c.
Also, struct netconsole_target would be required for multiple targets
support also, and not just dynamic reconfigurability. Previously these
two things were coupled, but I want to de-link that. (more on this later)
Note that this patch in itself looks quite redundant / stupid, but it is
purposefully made this way, so further patches are much more readable.
Signed-off-by: Satyam Sharma <redacted>
Cc: Keiichi Kii <redacted>
Cc: Takayoshi Kochi <redacted>
---
drivers/net/netconsole.c | 34 ++++++++++++++++++++++++----------
1 file changed, 24 insertions(+), 10 deletions(-)
---
diff -ruNp a/drivers/net/netconsole.c b/drivers/net/netconsole.c
From: Satyam Sharma <redacted>
[5/9] netconsole: Introduce dev_status member
Introduce a new member in netconsole_target that tracks the status (up or
down) of the underlying interface network device that the specific logging
target netpoll is attached to.
We then join this up with the just-introduced net_device notifier, and
introduce NETDEV_UP and NETDEV_DOWN notifications. By disabling the target
when the corresponding local interface is down, we save the overhead of
unnecessarily disabling interrupts and calling into the netpoll stack in
console->write().
Signed-off-by: Satyam Sharma <redacted>
Cc: Keiichi Kii <redacted>
Cc: Takayoshi Kochi <redacted>
---
drivers/net/netconsole.c | 42 +++++++++++++++++++++++++++++++-----------
1 file changed, 31 insertions(+), 11 deletions(-)
---
diff -ruNp a/drivers/net/netconsole.c b/drivers/net/netconsole.c
@@ -3,6 +3,10 @@ started by Ingo Molnar <mingo@redhat.com 2.6 port and netpoll api by Matt Mackall <mpm@selenic.com>, Sep 9 2003 Please send bug reports to Matt Mackall <mpm@selenic.com>+and Satyam Sharma <satyam.sharma@gmail.com>++Introduction:+============= This module logs kernel printk messages over UDP allowing debugging of problem where disk logging fails and serial consoles are impractical.
@@ -13,6 +17,9 @@ the specified interface as soon as possi capture of early kernel panics, it does capture most of the boot process.+Sender and receiver configuration:+==================================+ It takes a string configuration parameter "netconsole" in the following format:
@@ -38,7 +45,7 @@ It also supports logging to multiple rem parameters for the multiple agents separated by semicolons and the complete string enclosed in "quotes", thusly:-modprobe netconsole netconsole="@/,@10.0.0.2/;@/eth1,6892@10.0.0.3/"+ modprobe netconsole netconsole="@/,@10.0.0.2/;@/eth1,6892@10.0.0.3/" Built-in netconsole starts immediately after the TCP stack is initialized and attempts to bring up the supplied dev at the supplied
@@ -46,6 +53,64 @@ address. The remote host can run either 'netcat -u -l -p <port>' or syslogd.+Dynamic reconfiguration:+========================++Dynamic reconfigurability is a useful addition to netconsole that enables+remote logging targets to be dynamically added, removed, or have their+parameters reconfigured at runtime from a configfs-based userspace interface.++To include this feature, select CONFIG_NETCONSOLE_DYNAMIC when building the+netconsole module (or kernel, if netconsole is built-in).++Some examples follow (configfs is mounted at the /config mountpoint, say).++To add a remote logging target (target names can be arbitrary):++ cd /config/netconsole/+ mkdir target1++Note that newly created targets have default parameter values (as mentioned+above) and are disabled by default -- they must first be enabled by writing+"1" to the "enabled" attribute (usually after setting parameters accordingly)+as described below.++To remove a target:++ rmdir /config/netconsole/othertarget/++The interface exposes these parameters of a netconsole target to userspace:++ id Unique identifier for this target (read-only)+ enabled Is this target currently enabled? (read-write)+ dev_name Local network interface name (read-only)+ local_ip Source IP address to use (read-write)+ remote_ip Remote agent's IP address (read-write)+ local_port Source UDP port to use (read-write)+ remote_port Remote agent's UDP port (read-write)+ local_mac Local interface's MAC address (read-only)+ remote_mac Remote agent's MAC address (read-write)++The "enabled" attribute is also used to control whether the parameters of+a target can be updated or not -- you can modify the parameters of only+disabled targets (i.e. if "enabled" is 0).++To update a target's parameters:++ cat enabled # check if enabled is 1+ echo 0 > enabled # disable the target (if required)+ echo eth2 > dev_name # set local interface+ echo 10.0.0.4 > remote_ip # update some parameter+ echo cb:a9:87:65:43:21 > remote_mac # update more parameters+ echo 1 > enabled # enable target again++Note that you can even update the local interface dynamically. This is+especially useful if you want to use interfaces that have newly come up+and didn't even exist when netconsole was loaded / initialized.++Miscellaneous notes:+====================+ WARNING: the default target ethernet setting uses the broadcast ethernet address to send packets, which can cause increased load on other systems on the same ethernet segment.
@@ -55,7 +120,8 @@ so it is advised to explicitly specify t from the config parameters passed to netconsole. TIP: to find out the MAC address of 10.0.0.2, you may try using:-ping -c 1 10.0.0.2 ; /sbin/arp -n | grep 10.0.0.2++ ping -c 1 10.0.0.2 ; /sbin/arp -n | grep 10.0.0.2 TIP: in case the remote logging agent is on a separate LAN subnet than the sender, it is suggested to try specifying the default gateway's
Hi Satyam,
At first, thank you for your helping.
I'm going to try/review your patches.
[ I had initially thought about only modifying Keiichi's patches to
incorporate the suggested stuff, sysfs+ioctl->configfs conversion, etc,
but the changes became too much and I ultimately ended up rewriting the
patchset. This also gave the flexibility to make other changes possible.
Keiichi, Takayoshi, kindly review this, and if OK, please give your
Signed-off-by:'s to the patches herein. ]
By the way, how can I add Signed-off-by:'s to your patches?
Should I add it to each patch and submit them to this list again?
Anybody could give me an advice?
--
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com
Hi Keiichi,
On Wed, 4 Jul 2007, Keiichi KII wrote:
[...]
At first, thank you for your helping.
I'm going to try/review your patches.
quoted
[ I had initially thought about only modifying Keiichi's patches to
incorporate the suggested stuff, sysfs+ioctl->configfs conversion, etc,
but the changes became too much and I ultimately ended up rewriting the
patchset. This also gave the flexibility to make other changes possible.
Keiichi, Takayoshi, kindly review this, and if OK, please give your
Signed-off-by:'s to the patches herein. ]
By the way, how can I add Signed-off-by:'s to your patches?
Should I add it to each patch and submit them to this list again?
Anybody could give me an advice?
I think you could just individually reply to all the mails and replace
the "Cc:" in front of your name + email ID with "Signed-off-by:" --
after reviewing & testing this, of course!
Thanks,
Satyam
[ Some more thoughts I had on this, for those interested. ]
+#ifdef CONFIG_NETCONSOLE_DYNAMIC
+
+/*
+ * Targets that were created by parsing the boot/module option string
+ * do not exist in the configfs hierarchy and will never go away (and
+ * have zeroed-out config_item members). So make these a no-op for them.
+ */
+static void netconsole_target_get(struct netconsole_target *nt)
+{
+ static struct config_item empty_item; /* Zeroed-out config_item */
+
+ if (memcmp(&nt->item, &empty_item, sizeof(struct config_item)))
+ config_item_get(&nt->item);
+}
+
+static void netconsole_target_put(struct netconsole_target *nt)
+{
+ static struct config_item empty_item; /* Zeroed-out config_item */
+
+ if (memcmp(&nt->item, &empty_item, sizeof(struct config_item)))
+ config_item_put(&nt->item);
+}
+
+#else /* !CONFIG_NETCONSOLE_DYNAMIC */
+
/*
- * Allocate new target and setup netpoll for it.
+ * No danger of targets going away from under us when dynamic
+ * reconfigurability is off.
+ */
+static void netconsole_target_get(struct netconsole_target *nt)
+{
+}
+
+static void netconsole_target_put(struct netconsole_target *nt)
+{
+}
+
+#endif /* CONFIG_NETCONSOLE_DYNAMIC */
This whole stuff (and calls to netconsole_target_get and _put from code
below) is not necessary. All the access to the netconsole_target objects
(that are present in the target_list) happens inside
spin_lock_irqsave(target_list_lock), so there's no need to do a
config_item_get() on these as there is no race window in which userspace
can delete (and thus release) these items from under us. Note that
drop_netconsole_target() removes target-to-delete from target_list
(under spin_lock_irqsave) _before_ free'ing the objects, and that both
_get() and _put() are balanced *inside* a spin_lock_irqsave /
spin_unlock_irqrestore pair -- so all this business is quite pointless.
I've kept this stuff in for now, but would like to get rid of it, really.
It's an unnecessary #ifdef-#else-#endif kludge. Comments?
+ if (nt->enabled) {
+ printk(KERN_ERR "netconsole: target (id %d) is enabled, "
+ "disable to update parameters\n", nt->id);
+ return -EINVAL;
+ }
[ Such code exists in all the store() operations for all attributes other
than "enabled" itself. ]
Anyway, the problem is that the error does not show up violently in the
shell. The write fails, of course, values are unchanged, and dmesg shows
the diagnostic outputted above -- but "echo" does not complain about
-EINVAL returning from write(2) for some reason, neither did -EACCES.
I guess "echo" needs to be fixed. BTW would -EACCES be more appropriate?
Satyam
From: Matt Mackall <hidden> Date: 2007-07-04 13:47:42
On Wed, Jul 04, 2007 at 04:37:49PM +0530, Satyam Sharma wrote:
From: Satyam Sharma <redacted>
[2/9] netconsole: Code simplification
(1) Extract netpoll_parse_options() out of option_setup(), and into
init_netconsole() itself. So "configured" variable is redundant and
can be removed.
(2) With this change, option_setup() is not required for modular netconsole.
How is this a simplification? You've taken code with no #ifdefs and
added one! Please have both modular and nonmodular continue to use the
same paths.
(3) The (!np.dev) check in write_msg() is bogus (always false), because:
np.dev is set by netpoll_setup(), which is called by the target init
code in init_netconsole() _before_ register_console() => write_msg() cannot
be triggered unless netpoll_setup() returns with success. And that will not
happen if netpoll_setup() failed to set np.dev. Also np.dev cannot go from
under us while netconsole is loaded. This is because netpoll_setup() grabs
a reference for us on that dev. So let's remove the pointless check.
This ought to be a separate patch.
--
Mathematics is the supreme nostalgia of our time.
From: Matt Mackall <hidden> Date: 2007-07-04 13:58:08
On Wed, Jul 04, 2007 at 04:38:04PM +0530, Satyam Sharma wrote:
From: Satyam Sharma <redacted>
[5/9] netconsole: Introduce dev_status member
Introduce a new member in netconsole_target that tracks the status (up or
down) of the underlying interface network device that the specific logging
target netpoll is attached to.
We then join this up with the just-introduced net_device notifier, and
introduce NETDEV_UP and NETDEV_DOWN notifications. By disabling the target
when the corresponding local interface is down, we save the overhead of
unnecessarily disabling interrupts and calling into the netpoll stack in
console->write().
Yuck.
+/*
+ * Why no net_dev_is_up() in netdevice.h? The kernel could lose a lot of
+ * weight if only netdevice.h had the good sense to export such a function.
+ * Oh well ...
+ */
+static inline int net_dev_is_up(struct net_device *net_dev)
+{
+ return ((net_dev->flags & IFF_UP) == IFF_UP);
+}
Why editorialize? Why not just add this to netdevice.h?
+ if (nt->dev_status) {
Why not simply call net_dev_is_up?
--
Mathematics is the supreme nostalgia of our time.
From: Matt Mackall <hidden> Date: 2007-07-04 14:00:35
On Wed, Jul 04, 2007 at 04:37:59PM +0530, Satyam Sharma wrote:
From: Satyam Sharma <redacted>
[4/9] netconsole: Introduce netconsole_netdev_notifier
To update fields of underlying netpoll structure at runtime on
corresponding NETDEV_CHANGEADDR or NETDEV_CHANGENAME notifications.
It's not obvious that we want to do that.
I can have a host with DHCP that uses netconsole to log to a private
network address. This breaks that.
--
Mathematics is the supreme nostalgia of our time.
From: Matt Mackall <hidden> Date: 2007-07-04 14:02:32
On Wed, Jul 04, 2007 at 04:38:09PM +0530, Satyam Sharma wrote:
From: Satyam Sharma <redacted>
[6/9] netconsole: Update documentation for multiple target support
... and add a few useful general purpose tips as well while we're at it.
The tips are fine and should go in their own patch at the beginning of
the series. The docs for multiple interfaces should go either in the
patch that adds that (ideally) or after.
--
Mathematics is the supreme nostalgia of our time.
On Wed, Jul 04, 2007 at 04:38:04PM +0530, Satyam Sharma wrote:
quoted
[...]
+/*
+ * Why no net_dev_is_up() in netdevice.h? The kernel could lose a lot of
+ * weight if only netdevice.h had the good sense to export such a function.
+ * Oh well ...
+ */
+static inline int net_dev_is_up(struct net_device *net_dev)
+{
+ return ((net_dev->flags & IFF_UP) == IFF_UP);
+}
Why editorialize? Why not just add this to netdevice.h?
Hmm, I wanted to keep all changes local to netconsole, but ...
ok, will do that.
quoted
+ if (nt->dev_status) {
Why not simply call net_dev_is_up?
Yes, I did consider just calling the function here instead of
introducing another member ... again, ok will do that ...
quoted
We then join this up with the just-introduced net_device notifier, and
introduce NETDEV_UP and NETDEV_DOWN notifications. By disabling the target
when the corresponding local interface is down, we save the overhead of
unnecessarily disabling interrupts and calling into the netpoll stack in
console->write().
On Wed, Jul 04, 2007 at 04:37:49PM +0530, Satyam Sharma wrote:
quoted
[...]
(2) With this change, option_setup() is not required for modular netconsole.
How is this a simplification? You've taken code with no #ifdefs and
added one! Please have both modular and nonmodular continue to use the
same paths.
No, modular and non-modular do *not* take the same paths even in the
present code -- note that __setup() is a not even defined for modules.
If anything, the paths become somewhat "similar" *after* this patch.
Basically the present code is quite funny / strange, I'll explain why:
(1) For modular netconsole, the:
"module_param_string(netconsole, config, 256, 0);"
at the top of the file is what (effectively) does a:
strlcpy(config, "whatever", 256);
for us.
Note that __setup(option_setup) would be discarded at the preprocessor
stage itself for a modular build.
Then, in init_netconsole(), we call netpoll_parse_options() through
option_setup() ... the string (already copied into "config" due to the
module_param_string as I mentioned above) is then parsed and the
netpoll structure populated.
Then we proceed to do the netpoll_setup().
(2) For built-in netconsole:
__setup(option_setup) ensures a pointer to the option_setup() function
is put into the .init.text section starting at __setup__start ... then,
obsolete_checksetup() calls option_setup() with the appropriate string
from the kernel's command line [note that all this is happening even
before init_netconsole() gets called during the initcall process].
Anyway, obsolete_checksetup() calls option_setup() ->
netpoll_parse_options() that parses the passed string options that
matches after "netconsole=" [_not_ our "config" variable, which is a
redundant/unused variable in the built-in case] and populates the "np"
netpoll structure.
Then comes the kernel initcalls stage. init_netconsole() is duly called
when its time comes, and sees strlen(config) == 0 (note that config
is static and never populated in the built-in case that used the
"netconsole=" boot option). So, options_setup() is *not* called. But
that's no problem, "np" has already been populated after all.
And then we proceed to netpoll_setup() as usual.
Anyway, what I'm saying is that the _present_ code has radically different
codepaths, and the proposed patch _is_ a simplification because it makes
them somewhat more similar, as follows:
1. For modular netconsole, module_param_string effectively does a
strcpy(config, "whatever", 256); for us.
2. For built-in case, __setup effectively does _the same_ for us.
And for *both* the cases, init_netconsole() will call the
netpoll_parse_options() for us. Somewhat more similar, and simpler
(at least for me -- taste varies, of course).
BTW, you *don't* really want the exact same codepaths for both built-in
and modular netconsole anyway, believe me. If that's what you want, just
get rid of the __setup use in netconsole -- module_param_string works
equally good for modules when they are modular *or* when they are
built-in.
HOWEVER, this will introduce a regression: people will have to specify
the netconsole target specification as "netconsole.netconsole=..." on
the kernel command line, and not simply as "netconsole=" as they have
always been doing.
Perhaps I should've explained this a bit more verbosely in the changelog ...
Satyam
On Wed, Jul 04, 2007 at 04:37:59PM +0530, Satyam Sharma wrote:
quoted
From: Satyam Sharma <redacted>
[4/9] netconsole: Introduce netconsole_netdev_notifier
To update fields of underlying netpoll structure at runtime on
corresponding NETDEV_CHANGEADDR or NETDEV_CHANGENAME notifications.
[...]
I can have a host with DHCP that uses netconsole to log to a private
network address. This breaks that.
Hmm, could you explain that a bit more?
(and the breakage that could happen?)
(1) Please note that NETDEV_CHANGEADDR is a response to SIOCSIFHWADDR,
i.e. when the _hardware_ or MAC address of the particular _local_
(ethernet) network interface device that the netpoll is attached to
(i.e. np->dev) is changed -- say using:
# ip link set eth0 set address 01:23:45:67:89:ab
This has nothing to do with DHCP / IP addresses, AFAICT.
(2) NETDEV_CHANGENAME is a response to SIOCSIFNAME, i.e. when the name
of the particular (ethernet) network interface device is changed,
say something like:
# ip link set eth0 name ieee80230
(which would rename "eth0" to "ieee80230")
So I'm not following how DHCP could pose problems for us (or private
network addresses of remote logging agent), sorry ...
On Wed, 4 Jul 2007, Matt Mackall wrote:
On Wed, Jul 04, 2007 at 04:37:59PM +0530, Satyam Sharma wrote:
quoted
[...]
To update fields of underlying netpoll structure at runtime on
corresponding NETDEV_CHANGEADDR or NETDEV_CHANGENAME notifications.
It's not obvious that we want to do that.
I'd want to retain this, if possible, please. Kindly note that configfs
makes all these attributes (that are members of struct netpoll) visible
from userspace -- so that user can both read and write to them. We
wouldn't want to display obsolete values for any of these parameters.
Say the interface was called "eth0" when the user / admin created the
target and "enabled" it. And say somewhere in between the interface
name got changed to "eth2". We wouldn't want the user to get obsolete/
incorrect information if / when he does a "cat dev_name" from
/config/netconsole/mytarget123/, for example.
Satyam
On Wed, Jul 04, 2007 at 04:38:09PM +0530, Satyam Sharma wrote:
quoted
From: Satyam Sharma <redacted>
[6/9] netconsole: Update documentation for multiple target support
... and add a few useful general purpose tips as well while we're at it.
The tips are fine and should go in their own patch at the beginning of
the series. The docs for multiple interfaces should go either in the
patch that adds that (ideally) or after.
Ok, will do that in the next iteration -- along with the change to get
rid of the redundant ->dev_status member, as you mentioned.
Thanks a lot for reviewing this, Matt.
Satyam
It's a small nit, but isn't the large if() just the degenerate
default case of the switch()? Why have it at all?
Joel
--
Life's Little Instruction Book #396
"Never give anyone a fruitcake."
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127
From: Joel Becker <hidden> Date: 2007-07-05 06:52:54
On Wed, Jul 04, 2007 at 04:38:19PM +0530, Satyam Sharma wrote:
+Some examples follow (configfs is mounted at the /config mountpoint, say).
configfs is generally expected to be mounted at
/sys/kernel/config, and it even creates that mountpoint for itself. The
documentation should probably be consistent with that.
Joel
--
"Also, all of life's big problems include the words 'indictment' or
'inoperable.' Everything else is small stuff."
- Alton Brown
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127
From: Joel Becker <hidden> Date: 2007-07-05 07:05:37
On Wed, Jul 04, 2007 at 05:55:15PM +0530, Satyam Sharma wrote:
On Wed, 4 Jul 2007, Keiichi KII wrote:
quoted
By the way, how can I add Signed-off-by:'s to your patches?
Should I add it to each patch and submit them to this list again?
Anybody could give me an advice?
I think you could just individually reply to all the mails and replace
the "Cc:" in front of your name + email ID with "Signed-off-by:" --
after reviewing & testing this, of course!
If the chain is not going through Keiichi, he should be
responding with "Acked-by:", not "Signed-off-by:". Then Satyam submits
the patches upstream with the Acked-by: line and his own Signed-off-by:
Joel
--
"Anything that is too stupid to be spoken is sung."
- Voltaire
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127
It's a small nit, but isn't the large if() just the degenerate
default case of the switch()? Why have it at all?
Yes, the large if() is redundant in this particular patch.
But in further patches, the switch() is enclosed inside a
spin_lock_irqsave() and list_for_each_entry(target_list) and so the
large if() upfront saves us from unnecessarily disabling interrupts
and iterating through the entire target_list in the case that it is
a notification for an event that we don't care about.
So I'll remove this if() from this patch and introduce it in the
later one itself, which is where it starts making some sense, anyway.
Satyam
On Wed, Jul 04, 2007 at 04:38:19PM +0530, Satyam Sharma wrote:
quoted
+Some examples follow (configfs is mounted at the /config mountpoint, say).
configfs is generally expected to be mounted at
/sys/kernel/config, and it even creates that mountpoint for itself. The
documentation should probably be consistent with that.
Ok, will do that in the next iteration -- I'll wait for a couple more
days for comments from others, and then post the v2 patches sometime
during the weekend.
Satyam
+ cat enabled # check if enabled is 1
+ echo 0 > enabled # disable the target (if required)
+ echo eth2 > dev_name # set local interface
I think that the above line should change from "echo eth2 > dev_name" to
"echo -n eth2 > dev_name" or the newline should be removed at store_dev_name().
The default behavior of echo(1) outputs the newline.
So, if we write appropriate network device name to dev_name,
the netpoll can't find net_device in netpoll_setup().
I don't have enough time now. So I will check your patches more specific
at the weekend.
Thanks
--
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com
Hi Keiichi,
On Thu, 5 Jul 2007, Keiichi KII wrote:
Hi Satyam,
quoted
+ cat enabled # check if enabled is 1
+ echo 0 > enabled # disable the target (if required)
+ echo eth2 > dev_name # set local interface
I think that the above line should change from "echo eth2 > dev_name" to
"echo -n eth2 > dev_name" or the newline should be removed at store_dev_name().
The default behavior of echo(1) outputs the newline.
So, if we write appropriate network device name to dev_name,
the netpoll can't find net_device in netpoll_setup().
Yes, you're right. I'll fix this in store_dev_name() itself, it isn't
right to limit users to only -n option. Thanks for pointing this out.
[ BTW I also noticed a typo in that file just above your extract that
mentions dev_name as a read-only attribute! That also must be fixed ... ]
I don't have enough time now. So I will check your patches more specific
at the weekend.
Ok, thanks a lot. I'll wait for your comments / testing results over the
weekend, before sending out the next iteration.
BTW I did some testing myself, and have found another *embarrassing* bug:
if netconsole is loaded _without_ specifying any "netconsole=" parameter,
module is still kept loaded, but on unloading configfs_unregister_...()
obviously panics! :-) This should've been found by me earlier, just that
I never tested without specifying the parameter to modprobe :-(
Thanks,
Satyam
[...]
BTW I did some testing myself, and have found another *embarrassing* bug:
if netconsole is loaded _without_ specifying any "netconsole=" parameter,
module is still kept loaded, but on unloading configfs_unregister_...()
obviously panics! :-) This should've been found by me earlier, just that
I never tested without specifying the parameter to modprobe :-(
Basically that check upfront in init_netconsole() checking if the boot/
module param was provided now becomes unnecessary -- with dynamic
reconfigurability, a user might plausibly want to load netconsole without
specifying any params, but add targets later ...
Satyam
From: Stephen Hemminger <hidden> Date: 2007-07-05 15:18:57
On Wed, 4 Jul 2007 08:56:42 -0500
Matt Mackall [off-list ref] wrote:
On Wed, Jul 04, 2007 at 04:38:04PM +0530, Satyam Sharma wrote:
quoted
From: Satyam Sharma <redacted>
[5/9] netconsole: Introduce dev_status member
Introduce a new member in netconsole_target that tracks the status (up or
down) of the underlying interface network device that the specific logging
target netpoll is attached to.
We then join this up with the just-introduced net_device notifier, and
introduce NETDEV_UP and NETDEV_DOWN notifications. By disabling the target
when the corresponding local interface is down, we save the overhead of
unnecessarily disabling interrupts and calling into the netpoll stack in
console->write().
Yuck.
quoted
+/*
+ * Why no net_dev_is_up() in netdevice.h? The kernel could lose a lot of
+ * weight if only netdevice.h had the good sense to export such a function.
+ * Oh well ...
+ */
+static inline int net_dev_is_up(struct net_device *net_dev)
+{
+ return ((net_dev->flags & IFF_UP) == IFF_UP);
+}
Why editorialize? Why not just add this to netdevice.h?
quoted
+ if (nt->dev_status) {
Why not simply call net_dev_is_up?
The flags field values are really BSD legacy-ish stuff and should not be
used internally. IFF_UP etc, do not have the necessary atomic properties.
Use netif_running() instead.
From: Joel Becker <hidden> Date: 2007-07-07 06:02:01
On Wed, Jul 04, 2007 at 04:38:24PM +0530, Satyam Sharma wrote:
struct netconsole_target {
struct list_head list;
+ struct config_item item;
+ int id;
+ int enabled;
int dev_status;
struct netpoll np;
};
If you're trying to be good with your CONFIG_NETCONSOLE_DYNAMIC
ifdefs, you probably want to ifdef the item. You'll save space when
NETCONSOLE_DYNAMIC is off.
+#ifdef CONFIG_NETCONSOLE_DYNAMIC
+
+/*
+ * Targets that were created by parsing the boot/module option string
+ * do not exist in the configfs hierarchy and will never go away (and
+ * have zeroed-out config_item members). So make these a no-op for them.
+ */
+static void netconsole_target_get(struct netconsole_target *nt)
+{
+ static struct config_item empty_item; /* Zeroed-out config_item */
+
+ if (memcmp(&nt->item, &empty_item, sizeof(struct config_item)))
+ config_item_get(&nt->item);
+}
I was going to point out that you could merely check
config_item_name(&nt->item) != NULL, because a valid configfs object has
a name and your zeroed object does not. But your followup email
suggests you'll be removing this code anyway.
I don't, off the top of my head, see a problem with removing the
_get/_put cycle, because you do have them under the spinlock. Things
should behave correctly. However, the _get/_put pair is "cleaner", in
that it expresses the relationship and doesn't add a special case of "I
happen to know this is safe".
Perhaps an indent would be clearer, but that's a tiny nitpick.
/*
- * Neither the netdev notifier, nor the console have been
- * registered so far. Nobody's racing us, so skip the lock.
+ * Neither the netdev notifier, not the configfs subsystem and
+ * nor the console have been registered so far. Nobody's racing us,
+ * so skip the lock.
Once again, while you know you can skip the lock, it's unclear
without the comment. Perhaps using the lock makes it explicitly
"correct"? Food for thought.
quoted hunk
@@ -251,6 +796,17 @@ static int __init init_netconsole(void) if (err) goto fail;+#ifdef CONFIG_NETCONSOLE_DYNAMIC+ config_group_init(&netconsole_subsys.su_group);+ mutex_init(&netconsole_subsys.su_mtx);++ err = configfs_register_subsystem(&netconsole_subsys);+ if (err) {+ unregister_netdevice_notifier(&netconsole_netdev_notifier);+ goto fail;+ }+#endif /* CONFIG_NETCONSOLE_DYNAMIC */
and this to an dynamic_fini() function. Basically, do what you
did with _get()/_put(). This keeps the ifdef up above and out of the
functions themselves.
#ifdef CONFIG_NETCONSOLE_DYNAMIC
static int __init dynamic_netconsole_init(void)
{
config_group_init(&netconsole_subsys.su_group);
mutex_init(&netconsole_subsys.su_mtx);
return configfs_register_subsystem(&netconsole_subsys);
}
#else
static int __init dynamic_netconsole_init(void)
{
return 0;
}
#endif
Joel
--
Life's Little Instruction Book #3
"Watch a sunrise at least once a year."
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127
On Wed, Jul 04, 2007 at 04:38:24PM +0530, Satyam Sharma wrote:
quoted
struct netconsole_target {
struct list_head list;
+ struct config_item item;
+ int id;
+ int enabled;
int dev_status;
struct netpoll np;
};
If you're trying to be good with your CONFIG_NETCONSOLE_DYNAMIC
ifdefs, you probably want to ifdef the item. You'll save space when
NETCONSOLE_DYNAMIC is off.
Hmm, I had thought about this, but thought people might not like
another #ifdef. But then this kind of thing is very idiomatic throughout
the kernel, so ok, I'll do this.
[ BTW that .id field also looks like it can be gotten rid of altogether.
I was using it to print informational / error messages in the store()
operations below, but I guess I'll just use config_item_name() there
as well. ]
quoted
+#ifdef CONFIG_NETCONSOLE_DYNAMIC
+
+/*
+ * Targets that were created by parsing the boot/module option string
+ * do not exist in the configfs hierarchy and will never go away (and
+ * have zeroed-out config_item members). So make these a no-op for them.
+ */
+static void netconsole_target_get(struct netconsole_target *nt)
+{
+ static struct config_item empty_item; /* Zeroed-out config_item */
+
+ if (memcmp(&nt->item, &empty_item, sizeof(struct config_item)))
+ config_item_get(&nt->item);
+}
I was going to point out that you could merely check
config_item_name(&nt->item) != NULL, because a valid configfs object has
a name and your zeroed object does not.
Ok.
[ BTW: not related to dynamic netconsole / issue at hand, but ... I
just noticed in fs/configfs/item.c that ci_name is set to point to
embedded ci_namebuf array in config_item_set_name (if name was small
enough to be set in ci_namebuf itself), which is called from
config_item_init_type_name(). So, this keeps ci_name and ci_namebuf
in sync for such items.
However, for items that are statically initialized (often the
group->cg_item members of subsystems or default groups) we often simply
set ci_namebuf and then call config_item_init() -- say via
config_group_init(), like I've done with the netconsole subsystem in this
patch -- but config_item_init() does not set ci_name for such items to
ci_namebuf. This means the ci_name member of _initialized_ config_items
with their names in ci_namebuf is left un-initialized (NULL, actually,
because the subsys / default group would likely be static).
This doesn't really affect dynamic netconsole, because all config_items
that will be checked in the get() / put() check above are targets that
were initialized with config_item_init_type_name(), but something to
think about for perhaps other users?
Perhaps users who want to statically initialize their config_items must
be asked to just use ci_name and avoid ci_namebuf altogether? ]
I don't, off the top of my head, see a problem with removing the
_get/_put cycle, because you do have them under the spinlock. Things
should behave correctly. However, the _get/_put pair is "cleaner", in
that it expresses the relationship and doesn't add a special case of "I
happen to know this is safe".
Right. We shouldn't special case (at least not without adding a comment
why that would be right) and we never know what might happen to the code
at some later day. So let's keep the get() / put() pair.
quoted
+ /*
+ * Skip netpoll_parse_options() -- all the attributes are
+ * already configured in nt->np through configfs. But at
+ * least let's print the useful stuff it used to output :-)
+ */
+ printk(KERN_INFO "%s: local port %d\n",
+ np->name, np->local_port);
+ printk(KERN_INFO "%s: local IP %d.%d.%d.%d\n",
+ np->name, HIPQUAD(np->local_ip));
+ printk(KERN_INFO "%s: interface %s\n",
+ np->name, np->dev_name);
+ printk(KERN_INFO "%s: remote port %d\n",
+ np->name, np->remote_port);
+ printk(KERN_INFO "%s: remote IP %d.%d.%d.%d\n",
+ np->name, HIPQUAD(np->remote_ip));
+ printk(KERN_INFO "%s: remote ethernet address "
+ "%02x:%02x:%02x:%02x:%02x:%02x\n",
+ np->name,
+ np->remote_mac[0], np->remote_mac[1],
+ np->remote_mac[2], np->remote_mac[3],
+ np->remote_mac[4], np->remote_mac[5]);
Shouldn't you break this out into a function so that both places
can use it?
Hmm, netpoll_parse_options() currently prints these separately as and
how it walks through the input string parsing it. But changing that to
just print all these out together at the end if / when the parse is
successful seems better than what it's doing right now. I'll do this too.
Perhaps an indent would be clearer, but that's a tiny nitpick.
Yes, an indent would be good there.
quoted
/*
- * Neither the netdev notifier, nor the console have been
- * registered so far. Nobody's racing us, so skip the lock.
+ * Neither the netdev notifier, not the configfs subsystem and
+ * nor the console have been registered so far. Nobody's racing us,
+ * so skip the lock.
Once again, while you know you can skip the lock, it's unclear
without the comment. Perhaps using the lock makes it explicitly
"correct"? Food for thought.
Yes. That special-casing was really ugly and unnecessary -- we just need
to hold the lock around list_add(). I was going to set that right in the
next iteration myself.
quoted
@@ -251,6 +796,17 @@ static int __init init_netconsole(void) if (err) goto fail;+#ifdef CONFIG_NETCONSOLE_DYNAMIC+ config_group_init(&netconsole_subsys.su_group);+ mutex_init(&netconsole_subsys.su_mtx);++ err = configfs_register_subsystem(&netconsole_subsys);+ if (err) {+ unregister_netdevice_notifier(&netconsole_netdev_notifier);+ goto fail;+ }+#endif /* CONFIG_NETCONSOLE_DYNAMIC */
and this to an dynamic_fini() function. Basically, do what you
did with _get()/_put(). This keeps the ifdef up above and out of the
functions themselves.
#ifdef CONFIG_NETCONSOLE_DYNAMIC
static int __init dynamic_netconsole_init(void)
{
config_group_init(&netconsole_subsys.su_group);
mutex_init(&netconsole_subsys.su_mtx);
return configfs_register_subsystem(&netconsole_subsys);
}
#else
static int __init dynamic_netconsole_init(void)
{
return 0;
}
#endif
Right, this looks neater and is again the idiom followed throughout the
kernel.
Thanks,
Satyam
Hi Stephen,
On Thu, 5 Jul 2007, Stephen Hemminger wrote:
On Wed, 4 Jul 2007 08:56:42 -0500
Matt Mackall [off-list ref] wrote:
[...]
quoted
quoted
+ if (nt->dev_status) {
Why not simply call net_dev_is_up?
The flags field values are really BSD legacy-ish stuff and should not be
used internally. IFF_UP etc, do not have the necessary atomic properties.
Use netif_running() instead.
Ok, thanks for letting me know about that. I'll make the change.
Satyam
From: Joel Becker <hidden> Date: 2007-07-07 08:19:43
On Sat, Jul 07, 2007 at 01:18:45PM +0530, Satyam Sharma wrote:
However, for items that are statically initialized (often the
group->cg_item members of subsystems or default groups) we often simply
set ci_namebuf and then call config_item_init() -- say via
config_group_init(), like I've done with the netconsole subsystem in this
patch -- but config_item_init() does not set ci_name for such items to
ci_namebuf. This means the ci_name member of _initialized_ config_items
with their names in ci_namebuf is left un-initialized (NULL, actually,
because the subsys / default group would likely be static).
Configfs notices subsystems and default groups and handles this.
See configfs_register_subsystem() and create_default_group() in
fs/configfs/dir.c
Right. We shouldn't special case (at least not without adding a comment
why that would be right) and we never know what might happen to the code
at some later day. So let's keep the get() / put() pair.
If you're keeping them, don't do the "empty_item", check the
name.
Otherwise, the changes you describe sound good.
Joel
--
"I have never let my schooling interfere with my education."
- Mark Twain
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127
BTW I did some testing myself, and have found another *embarrassing* bug:
if netconsole is loaded _without_ specifying any "netconsole=" parameter,
module is still kept loaded, but on unloading configfs_unregister_...()
obviously panics! :-) This should've been found by me earlier, just that
I never tested without specifying the parameter to modprobe :-(
I tested your patches on the x86 architecture and report the results.
It is no problem except for the above problem.
I think that it is useful to use configfs(especially, mkdir and rmdir
because of adding/removing netconsole_target).
I am going to test the your next iteration patches on x86 and IA64
architecutures.
Thanks
--
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com
[4/9] netconsole: Introduce netconsole_netdev_notifier
To update fields of underlying netpoll structure at runtime on
corresponding NETDEV_CHANGEADDR or NETDEV_CHANGENAME notifications.
Signed-off-by: Satyam Sharma <redacted>
Cc: Keiichi Kii <redacted>
Cc: Takayoshi Kochi <redacted>
Should we avoid the automatic update of netconsole_target until we can notice all
necessary members of netconsole_target updated automatically?
I had thought about automatic updating of the netconsole_target(i.e. local IP address
and network device).
But I thought that the netconsole(netpoll) can only be related to a network device
because the user of netconsole is free to be able to set local IP address and
MAC address of netconsole and it is difficult to monitor change of local IP address.
So, my patches only updates the network device automatically by using a symbolic link
to the network device in sysfs when the event of NETDEV_CHANGEADDR occurs.
The symbolic link to the network device in sysfs has the same meaning as following change
of MAC address and hardware.
(but I hadn't known the changing ability of MAC address using ip(8) command,
thanks for your info)
Or I propose updating netconsole_taget informations when the network device up/down.
The MAC address and network interface name can be changed when
the network device is disabled.
But, in the case, we must think of the case of changing IP address
because we can change IP address without disabling the network device and
can't hook the event of changing IP address.
How do you think?
Thanks
--
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com
After more discussion, Joel suggested the idea to separately manage the
lifetimes of the two kinds of netconsole_target objects, those created while
parsing boot/module options and those created via configfs mkdir(2) from
userspace, in my driver itself. That adds a little bit of complexity and
redundancy (multiple item creation and destruction points) in this driver.
Note that the latter type (configfs-created netconsole_targets) can be
modified / updated / destroyed at runtime from userspace but the former
(param string created) cannot, as they are not exposed through our configfs
namespace. However, this is not really a problem and is similar to current
behaviour in any case.
Maybe you should add the above description into
Documentation/networking/netconsole.txt.
The users of netconsole applied these patches may think that they can change
parameters of netconsole_target(param string created at the time of parsing
boot/options).
And I'm going to search the another solution of this problem and read the
code of configfs.
Thanks
--
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com
Hi Keiichi,
On Sun, 8 Jul 2007, KII Keiichi wrote:
Hi Satyam,
quoted
After more discussion, Joel suggested the idea to separately manage the
lifetimes of the two kinds of netconsole_target objects, those created while
parsing boot/module options and those created via configfs mkdir(2) from
userspace, in my driver itself. That adds a little bit of complexity and
redundancy (multiple item creation and destruction points) in this driver.
Note that the latter type (configfs-created netconsole_targets) can be
modified / updated / destroyed at runtime from userspace but the former
(param string created) cannot, as they are not exposed through our configfs
namespace. However, this is not really a problem and is similar to current
behaviour in any case.
Maybe you should add the above description into
Documentation/networking/netconsole.txt.
The users of netconsole applied these patches may think that they can change
parameters of netconsole_target(param string created at the time of parsing
boot/options).
Yes, you're absolutely right. This definitely needs to be mentioned in the
documenation -- I'll take care of this in the next iteration.
Satyam
Eek, looks like some whitespace correction leaked in here. I'll take care
not to produce the bad whitespace in the first place the first time this
line comes in.
quoted
+ spin_lock_irqsave(&target_list_lock, flags);
+ list_for_each_entry(nt, &target_list, list) {
+ if (nt->np.dev == dev) {
+ switch (event) {
+ case NETDEV_UP:
+ case NETDEV_DOWN:
+ nt->dev_status = net_dev_is_up(nt->np.dev);
+ break;
+
+ case NETDEV_CHANGEADDR:
+ memcpy(nt->np.local_mac, dev->dev_addr, ETH_ALEN);
The above line is over 80 characters.
Hmm, yes, but only by 2 columns. It's a simple and readable line
in its present form, so I'm not sure I'd want to be an extremist
and cut it into two ...
Satyam
[...]
Should we avoid the automatic update of netconsole_target until we can notice all
necessary members of netconsole_target updated automatically?
I had thought about automatic updating of the netconsole_target(i.e. local IP address
and network device).
But I thought that the netconsole(netpoll) can only be related to a network device
because the user of netconsole is free to be able to set local IP address and
MAC address of netconsole and it is difficult to monitor change of local IP address.
[...]
But, in the case, we must think of the case of changing IP address
because we can change IP address without disabling the network device and
can't hook the event of changing IP address.
Well, updating the local _IP_ address (for a particular interface) as
and when it changes would also be good ... it is another piece of info
exposed (read-write) to userspace through configfs, so makes sense to
keep it uptodate. But note that the two event notifications that we
care about currently are simply to keep up with changes in local _MAC_
address and interface name.
Regarding local_ip, the present behaviour is:
(1) If the user did not specify any local IP address (that is left that
field empty or zero), netpoll_setup() automatically fills it up with the
IP of the local network interface device that it is attached to: from
net_device->ip_ptr->ifa_list->ifa_local
(2) If user specified some local IP address, netpoll_setup() blindly
copies that into np->local_ip and that would later get used when
constructing the eth/ip/udp packet to be sent (containing the logging
data), as can be seen in netpoll_send_udp(). If the local IP changes
at some point after netpoll_setup(), nothing updates np->local_ip
accordingly.
So this patch just continues preserving the current behaviour, but yes,
it could make sense to trap protocol address changes and update the
local_ip field too when that happens (as long as it doesn't break
anything, as Matt warned earlier). I'm not aware of any existing
netdevice notifier that'll do this, so I'll look around (or trace
an ifconfig(8) / ip(8) to see what actually happens).
Or I propose updating netconsole_taget informations when the network device up/down.
The MAC address and network interface name can be changed when
the network device is disabled.
Actually, what happens is that if you: (1) disable / down the interface,
(2) change name / MAC address while it was down, and then (3) bring it
back up, then on coming back up, *both* the NETDEV_CHANGE{NAME,ADDR} and
the NETDEV_UP notifier chains are called out. If you just brought it
down and back up without setting any params, the other chains would not
be called out, but NETDEV_UP would. In this case, it's unnecessary to
update the params, because nothing got changed. And if something did
get changed, the corresponding chain would get called anyway, so it's
clearly best to update only the appropriate params, only when the
corresponding event notification comes along.
Satyam
Hi Keiichi,
On Sun, 8 Jul 2007, KII Keiichi wrote:
[...]
I tested your patches on the x86 architecture and report the results.
It is no problem except for the above problem.
I think that it is useful to use configfs(especially, mkdir and rmdir because
of adding/removing netconsole_target).
I am going to test the your next iteration patches on x86 and IA64
architecutures.
Thanks a lot for reviewing and testing this. I'll send out the next
iteration by tommorrow.
Thanks,
Satyam
But, in the case, we must think of the case of changing IP address
because we can change IP address without disabling the network device and
can't hook the event of changing IP address.
Well, updating the local _IP_ address (for a particular interface) as
and when it changes would also be good ... it is another piece of info
exposed (read-write) to userspace through configfs, so makes sense to
keep it uptodate.
[...] (as long as it doesn't break
anything, as Matt warned earlier). I'm not aware of any existing
netdevice notifier that'll do this, so I'll look around (or trace
an ifconfig(8) / ip(8) to see what actually happens).
I think inetaddr_chain / register_inetaddr_notifier() are what I'm
looking for. I'll see if this does the job for us without breaking
anything else.
Satyam