lkml.org 
[lkml]   [2007]   [Jul]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH -mm 9/9] netconsole: Support dynamic reconfiguration using configfs
On Wed, Jul 04, 2007 at 04:38:24PM +0530, Satyam Sharma wrote:
> struct netconsole_target {
> struct list_head list;
> + struct config_item item;
> + int id;
> + int enabled;
> int dev_status;
> struct netpoll np;
> };

If you're trying to be good with your CONFIG_NETCONSOLE_DYNAMIC
ifdefs, you probably want to ifdef the item. You'll save space when
NETCONSOLE_DYNAMIC is off.

> +#ifdef CONFIG_NETCONSOLE_DYNAMIC
> +
> +/*
> + * Targets that were created by parsing the boot/module option string
> + * do not exist in the configfs hierarchy and will never go away (and
> + * have zeroed-out config_item members). So make these a no-op for them.
> + */
> +static void netconsole_target_get(struct netconsole_target *nt)
> +{
> + static struct config_item empty_item; /* Zeroed-out config_item */
> +
> + if (memcmp(&nt->item, &empty_item, sizeof(struct config_item)))
> + config_item_get(&nt->item);
> +}

I was going to point out that you could merely check
config_item_name(&nt->item) != NULL, because a valid configfs object has
a name and your zeroed object does not. But your followup email
suggests you'll be removing this code anyway.
I don't, off the top of my head, see a problem with removing the
_get/_put cycle, because you do have them under the spinlock. Things
should behave correctly. However, the _get/_put pair is "cleaner", in
that it expresses the relationship and doesn't add a special case of "I
happen to know this is safe".

> + * Our subsystem hierarchy is:
> + *
> + * /config/netconsole/
> + * |
> + * <target>/
> + * | id
> + * | enabled

Your configfs bits seem to be pretty straightforward.

> + /*
> + * Skip netpoll_parse_options() -- all the attributes are
> + * already configured in nt->np through configfs. But at
> + * least let's print the useful stuff it used to output :-)
> + */
> + printk(KERN_INFO "%s: local port %d\n",
> + np->name, np->local_port);
> + printk(KERN_INFO "%s: local IP %d.%d.%d.%d\n",
> + np->name, HIPQUAD(np->local_ip));
> + printk(KERN_INFO "%s: interface %s\n",
> + np->name, np->dev_name);
> + printk(KERN_INFO "%s: remote port %d\n",
> + np->name, np->remote_port);
> + printk(KERN_INFO "%s: remote IP %d.%d.%d.%d\n",
> + np->name, HIPQUAD(np->remote_ip));
> + printk(KERN_INFO "%s: remote ethernet address "
> + "%02x:%02x:%02x:%02x:%02x:%02x\n",
> + np->name,
> + np->remote_mac[0], np->remote_mac[1],
> + np->remote_mac[2], np->remote_mac[3],
> + np->remote_mac[4], np->remote_mac[5]);

Shouldn't you break this out into a function so that both places
can use it?

> +#define NETCONSOLE_TARGET_ATTR_RO(_name) \
> +static struct netconsole_target_attr netconsole_target_##_name = \
> +__CONFIGFS_ATTR(_name, S_IRUGO, show_##_name, NULL)
> +
> +#define NETCONSOLE_TARGET_ATTR_RW(_name) \
> +static struct netconsole_target_attr netconsole_target_##_name = \
> +__CONFIGFS_ATTR(_name, S_IRUGO | S_IWUSR, show_##_name, store_##_name)

Perhaps an indent would be clearer, but that's a tiny nitpick.

> /*
> - * Neither the netdev notifier, nor the console have been
> - * registered so far. Nobody's racing us, so skip the lock.
> + * Neither the netdev notifier, not the configfs subsystem and
> + * nor the console have been registered so far. Nobody's racing us,
> + * so skip the lock.

Once again, while you know you can skip the lock, it's unclear
without the comment. Perhaps using the lock makes it explicitly
"correct"? Food for thought.

> @@ -251,6 +796,17 @@ static int __init init_netconsole(void)
> if (err)
> goto fail;
>
> +#ifdef CONFIG_NETCONSOLE_DYNAMIC
> + config_group_init(&netconsole_subsys.su_group);
> + mutex_init(&netconsole_subsys.su_mtx);
> +
> + err = configfs_register_subsystem(&netconsole_subsys);
> + if (err) {
> + unregister_netdevice_notifier(&netconsole_netdev_notifier);
> + goto fail;
> + }
> +#endif /* CONFIG_NETCONSOLE_DYNAMIC */

I'd abstract this to a dynamic_init() function.

> +
> +#ifdef CONFIG_NETCONSOLE_DYNAMIC
> + configfs_unregister_subsystem(&netconsole_subsys);
> +#endif /* CONFIG_NETCONSOLE_DYNAMIC */
> +

and this to an dynamic_fini() function. Basically, do what you
did with _get()/_put(). This keeps the ifdef up above and out of the
functions themselves.

#ifdef CONFIG_NETCONSOLE_DYNAMIC
static int __init dynamic_netconsole_init(void)
{
config_group_init(&netconsole_subsys.su_group);
mutex_init(&netconsole_subsys.su_mtx);
return configfs_register_subsystem(&netconsole_subsys);
}
#else
static int __init dynamic_netconsole_init(void)
{
return 0;
}
#endif


Joel

--

Life's Little Instruction Book #3

"Watch a sunrise at least once a year."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127
-
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-07-07 08:05    [W:0.188 / U:0.376 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site