Re: [patch] kernel sysfs events layer

[Greg KH <greg@kroah.com>]

On Mon, Sep 13, 2004 at 04:45:53PM +0200, Kay Sievers wrote:
> On Sat, Sep 11, 2004 at 09:53:00AM -0700, Greg KH wrote:
> > On Sat, Sep 11, 2004 at 12:09:35AM -0400, Robert Love wrote:
> > > > +/**
> > > > + * send_uevent - notify userspace by sending event trough netlink socket
> > > 
> > > s/trough/through/ ;-)
> > 
> > Heh, give something for the "spelling nit-pickers" to submit a patch
> > against :)
> > 
> > > We should probably add at least _some_ user.  The filesystem mount
> > > events are good, since we want to add those to HAL.
> > 
> > True, anyone want to send me a patch with a user of this?
> 
> Do we agree on the model that the signal is a simple verb and we keep
> only a small dictionary of _static_ signal strings and no fancy compositions?

I agree with this.  And because of that, we should enforce that, and
help prevent typos in the signals.  So, here's a patch that does just
that, making it a lot harder to mess up (although you still can, as
enumerated types aren't checked by the compiler...)  This patch booted
on my test box.

Anyone object to me adding this patch?  If not, I'll fix up Kay's patch
for mounting to use this interface and add both of them.

> And we should reserve the "add" and "remove" only for the hotplug events.

I don't know, the firmware objects already use "add" for an event.  I
didn't put a check in the kobject_uevent() calls to prevent the add and
remove, but now it's a lot easier to do so if you think it's necessary.

thanks,

greg k-h

-----------

kevent: force users to use a type, not a string to help prevent typos
and so that we all can keep track of the different event types much
easier.


===== drivers/base/firmware_class.c 1.13 vs edited =====
--- 1.13/drivers/base/firmware_class.c	2004-09-13 09:46:22 -07:00
+++ edited/drivers/base/firmware_class.c	2004-09-14 16:33:20 -07:00
@@ -420,7 +420,7 @@
 		add_timer(&fw_priv->timeout);
 	}
 
-	kobject_hotplug("add", &class_dev->kobj);
+	kobject_hotplug(&class_dev->kobj, KOBJ_ADD);
 	wait_for_completion(&fw_priv->completion);
 	set_bit(FW_STATUS_DONE, &fw_priv->status);
 
===== include/linux/kobject.h 1.30 vs edited =====
--- 1.30/include/linux/kobject.h	2004-09-13 12:58:51 -07:00
+++ edited/include/linux/kobject.h	2004-09-14 16:24:00 -07:00
@@ -236,27 +236,33 @@
 extern void subsys_remove_file(struct subsystem * , struct subsys_attribute *);
 
 
+enum kobject_action {
+	KOBJ_ADD	= 0x00,
+	KOBJ_REMOVE	= 0x01,
+	KOBJ_MAX_ACTION,
+};
+
 #ifdef CONFIG_HOTPLUG
-extern void kobject_hotplug(const char *action, struct kobject *kobj);
+extern void kobject_hotplug(struct kobject *kobj, enum kobject_action action);
 #else
-static inline void kobject_hotplug(const char *action, struct kobject *kobj) { }
+static inline void kobject_hotplug(struct kobject *kobj, enum kobject_action action) { }
 #endif
 
 
 #ifdef CONFIG_KOBJECT_UEVENT
-extern int kobject_uevent(const char *signal, struct kobject *kobj,
+extern int kobject_uevent(struct kobject *kobj, enum kobject_action action,
 			  struct attribute *attr);
 
-extern int kobject_uevent_atomic(const char *signal, struct kobject *kobj,
+extern int kobject_uevent_atomic(struct kobject *kobj, enum kobject_action action,
 				 struct attribute *attr);
 #else
-static inline int kobject_uevent(const char *signal, struct kobject *kobj,
+static inline int kobject_uevent(struct kobject *kobj, enum kobject_action action,
 				 struct attribute *attr)
 {
 	return 0;
 }
 
-static inline int kobject_uevent_atomic(const char *signal, struct kobject *kobj,
+static inline int kobject_uevent_atomic(struct kobject *kobj, enum kobject_action action,
 					struct attribute *attr)
 {
 	return 0;
===== lib/kobject.c 1.55 vs edited =====
--- 1.55/lib/kobject.c	2004-09-13 09:54:04 -07:00
+++ edited/lib/kobject.c	2004-09-14 16:20:21 -07:00
@@ -185,7 +185,7 @@
 		if (parent)
 			kobject_put(parent);
 	} else {
-		kobject_hotplug("add", kobj);
+		kobject_hotplug(kobj, KOBJ_ADD);
 	}
 
 	return error;
@@ -299,7 +299,7 @@
 
 void kobject_del(struct kobject * kobj)
 {
-	kobject_hotplug("remove", kobj);
+	kobject_hotplug(kobj, KOBJ_REMOVE);
 	sysfs_remove_dir(kobj);
 	unlink(kobj);
 }
===== lib/kobject_uevent.c 1.2 vs edited =====
--- 1.2/lib/kobject_uevent.c	2004-09-11 18:50:05 -07:00
+++ edited/lib/kobject_uevent.c	2004-09-14 16:28:21 -07:00
@@ -22,6 +22,20 @@
 #include <linux/kobject.h>
 #include <net/sock.h>
 
+/* these match up with the values for enum kobject_action */
+static char *actions[] = {
+	"add",		/* 0x00 */
+	"remove",	/* 0x01 */
+};
+
+static char *action_to_string(enum kobject_action action)
+{
+	if (action >= KOBJ_MAX_ACTION)
+		return NULL;
+	else
+		return actions[action];
+}
+
 #ifdef CONFIG_KOBJECT_UEVENT
 static struct sock *uevent_sock;
 
@@ -60,11 +74,12 @@
 	return netlink_broadcast(uevent_sock, skb, 0, 1, gfp_mask);
 }
 
-static int do_kobject_uevent(const char *signal, struct kobject *kobj,
+static int do_kobject_uevent(struct kobject *kobj, enum kobject_action action, 
 			     struct attribute *attr, int gfp_mask)
 {
 	char *path;
 	char *attrpath;
+	char *signal;
 	int len;
 	int rc = -ENOMEM;
 
@@ -72,6 +87,10 @@
 	if (!path)
 		return -ENOMEM;
 
+	signal = action_to_string(action);
+	if (!signal)
+		return -EINVAL;
+
 	if (attr) {
 		len = strlen(path);
 		len += strlen(attr->name) + 2;
@@ -97,17 +116,17 @@
  * @kobj: struct kobject that the event is happening to
  * @attr: optional struct attribute the event belongs to
  */
-int kobject_uevent(const char *signal, struct kobject *kobj,
+int kobject_uevent(struct kobject *kobj, enum kobject_action action,
 		   struct attribute *attr)
 {
-	return do_kobject_uevent(signal, kobj, attr, GFP_KERNEL);
+	return do_kobject_uevent(kobj, action, attr, GFP_KERNEL);
 }
 EXPORT_SYMBOL_GPL(kobject_uevent);
 
-int kobject_uevent_atomic(const char *signal, struct kobject *kobj,
+int kobject_uevent_atomic(struct kobject *kobj, enum kobject_action action,
 			  struct attribute *attr)
 {
-	return do_kobject_uevent(signal, kobj, attr, GFP_ATOMIC);
+	return do_kobject_uevent(kobj, action, attr, GFP_ATOMIC);
 }
 
 EXPORT_SYMBOL_GPL(kobject_uevent_atomic);
@@ -149,7 +168,7 @@
  * @action: action that is happening (usually "ADD" or "REMOVE")
  * @kobj: struct kobject that the action is happening to
  */
-void kobject_hotplug(const char *action, struct kobject *kobj)
+void kobject_hotplug(struct kobject *kobj, enum kobject_action action)
 {
 	char *argv [3];
 	char **envp = NULL;
@@ -159,6 +178,7 @@
 	int retval;
 	char *kobj_path = NULL;
 	char *name = NULL;
+	char *action_string;
 	u64 seq;
 	struct kobject *top_kobj = kobj;
 	struct kset *kset;
@@ -183,6 +203,10 @@
 
 	pr_debug ("%s\n", __FUNCTION__);
 
+	action_string = action_to_string(action);
+	if (!action_string)
+		return;
+
 	envp = kmalloc(NUM_ENVP * sizeof (char *), GFP_KERNEL);
 	if (!envp)
 		return;
@@ -208,7 +232,7 @@
 	scratch = buffer;
 
 	envp [i++] = scratch;
-	scratch += sprintf(scratch, "ACTION=%s", action) + 1;
+	scratch += sprintf(scratch, "ACTION=%s", action_string) + 1;
 
 	kobj_path = kobject_get_path(kobj, GFP_KERNEL);
 	if (!kobj_path)
@@ -242,7 +266,7 @@
 	pr_debug ("%s: %s %s %s %s %s %s %s\n", __FUNCTION__, argv[0], argv[1],
 		  envp[0], envp[1], envp[2], envp[3], envp[4]);
 
-	send_uevent(action, kobj_path, buffer, scratch - buffer, GFP_KERNEL);
+	send_uevent(action_string, kobj_path, buffer, scratch - buffer, GFP_KERNEL);
 
 	if (!hotplug_path[0])
 		goto exit;