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