Hi Keiichi,
On 6/13/07, Keiichi KII <[EMAIL PROTECTED]> wrote:
+struct netconsole_target {
+ struct list_head list;
+ int id;
+ struct netpoll np;
+};
+
+static LIST_HEAD(target_list);
+static DEFINE_SPINLOCK(target_list_lock);
Some description of the struct netconsole_target / target_list
would be nice. For instance, here I tried to read from the code
what the "int id" member is all about, but noticed that it is
only being set during add_target(), and never read / used
anywhere else later. You should just remove it, then.
+ if (netpoll_setup(&new_target->np)) {
+ printk(KERN_ERR "netconsole: can't setup netpoll:%s\n",
+ target_config);
+ kfree(new_target);
+ retval = -EINVAL;
Please prefer something like "err = netpoll_setup(...);"
and then "if (err) { printk(); kfree(); return err; }" here.
netpoll_setup() returns perfectly reasonable error codes,
we must be returning the same back up.
+#ifndef MODULE
static int __init option_setup(char *opt)
{
- configured = !netpoll_parse_options(&np, opt);
+ strncpy(config, opt, 256);
strlcpy would be safer than strncpy here, IMHO.
-static int __init init_netconsole(void)
+static void cleanup_netconsole(void)
{
- int err;
+#ifdef CONFIG_NETCONSOLE_DYNCON
+ struct netconsole_target *nt, *tmp;
- if(strlen(config))
- option_setup(config);
+ unregister_console(&netconsole);
+ list_for_each_entry_safe(nt, tmp, &target_list, list)
+ remove_target(nt);
You should be doing the spin_lock_irqsave() and
spin_unlock_irqrestore() around this code here,
and lose it from the remove_target() ...
+static int __init init_netconsole(void)
+{
+ char *tmp = config;
+#ifdef CONFIG_NETCONSOLE_DYNCON
+ char *p;
+
+ register_console(&netconsole);
register_console() needs to be _after_ netpoll_setup().
+ if (!strlen(config)) {
+ printk(KERN_ERR "netconsole: not configured\n");
return 0;
}
You could move this out of the #ifdef. It's applicable to both
DYNCON and !DYNCON cases.
- err = netpoll_setup(&np);
- if (err)
- return err;
-
+ while ((p = strsep(&tmp, ";")) != NULL)
+ if (add_target(p)) {
+ printk(KERN_ERR
+ "netconsole: can't add target to netconsole\n");
+ cleanup_netconsole();
+ return -EINVAL;
+ }
+#else
+ if (!strlen(config) ||
+ netpoll_parse_options(&np, tmp) || netpoll_setup(&np)) {
Joining these 3 expressions (statements, actually) together
is so gross :-)
Please do them separately, like the previous code did. That way
you could preserve the error code from netpoll_setup() too ...
Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
