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

["Satyam Sharma" <satyam.sharma@gmail.com>]

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/