lkml.org 
[lkml]   [2007]   [Jun]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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/

    \
     
     \ /
      Last update: 2007-06-13 17:33    [W:2.223 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site