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

From: Satyam Sharma
Date: Wed Jun 13 2007 - 11:31:08 EST

Hi Keiichi,

On 6/13/07, Keiichi KII <k-keiichi@xxxxxxxxxxxxx> 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;
+ 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;
+ 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;
+ }
+ 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 ...

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at