Re: [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.

	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