sysfs: add sysfs_create/remove_groups()

PATCH In review Greg Kroah-Hartman 3 participants 6 messages Aug 21, 2013 – Aug 22, 2013 Driver core

lore

Share & tools

lore

Greg Kroah-Hartman

author maintainer

2013-08-21 20:35 UTC #0 lore raw

PATCH 1/1 sysfs: add sysfs_create/remove_groups()

From: Greg Kroah-Hartman <[email protected]>

These functions are being open-coded in 3 different places in the driver core, and other driver subsystems will want to start doing this as well, so move it to the sysfs core to keep it all in one place, where we know it is written properly.

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 drivers/base/bus.c    |   23 +---------------------
 drivers/base/core.c   |   22 +--------------------
 drivers/base/driver.c |   22 +--------------------
 fs/sysfs/group.c      |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sysfs.h |    4 +++
 5 files changed, 62 insertions(+), 61 deletions(-)

drivers/base/bus.c +2 ~~-21~~

@@ -884,32 +884,13 @@ static void bus_remove_attrs(struct bus_
 static int bus_add_groups(struct bus_type *bus,
 			  const struct attribute_group **groups)
 {
-	int error = 0;
-	int i;
-
-	if (groups) {
-		for (i = 0; groups[i]; i++) {
-			error = sysfs_create_group(&bus->p->subsys.kobj,
-						   groups[i]);
-			if (error) {
-				while (--i >= 0)
-					sysfs_remove_group(&bus->p->subsys.kobj,
-							   groups[i]);
-				break;
-			}
-		}
-	}
-	return error;
+	return sysfs_create_groups(&bus->p->subsys.kobj, groups);
 }
 
 static void bus_remove_groups(struct bus_type *bus,
 			      const struct attribute_group **groups)
 {
-	int i;
-
-	if (groups)
-		for (i = 0; groups[i]; i++)
-			sysfs_remove_group(&bus->p->subsys.kobj, groups[i]);
+	sysfs_remove_groups(&bus->p->subsys.kobj, groups);
 }
 
 static void klist_devices_get(struct klist_node *n)

drivers/base/core.c +2 ~~-20~~

@@ -493,31 +493,13 @@ static void device_remove_bin_attributes
 
 int device_add_groups(struct device *dev, const struct attribute_group **groups)
 {
-	int error = 0;
-	int i;
-
-	if (groups) {
-		for (i = 0; groups[i]; i++) {
-			error = sysfs_create_group(&dev->kobj, groups[i]);
-			if (error) {
-				while (--i >= 0)
-					sysfs_remove_group(&dev->kobj,
-							   groups[i]);
-				break;
-			}
-		}
-	}
-	return error;
+	return sysfs_create_groups(&dev->kobj, groups);
 }
 
 void device_remove_groups(struct device *dev,
 			  const struct attribute_group **groups)
 {
-	int i;
-
-	if (groups)
-		for (i = 0; groups[i]; i++)
-			sysfs_remove_group(&dev->kobj, groups[i]);
+	sysfs_remove_groups(&dev->kobj, groups);
 }
 
 static int device_add_attrs(struct device *dev)

drivers/base/driver.c +2 ~~-20~~

@@ -126,31 +126,13 @@ EXPORT_SYMBOL_GPL(driver_remove_file);
 int driver_add_groups(struct device_driver *drv,
 		      const struct attribute_group **groups)
 {
-	int error = 0;
-	int i;
-
-	if (groups) {
-		for (i = 0; groups[i]; i++) {
-			error = sysfs_create_group(&drv->p->kobj, groups[i]);
-			if (error) {
-				while (--i >= 0)
-					sysfs_remove_group(&drv->p->kobj,
-							   groups[i]);
-				break;
-			}
-		}
-	}
-	return error;
+	return sysfs_create_groups(&drv->p->kobj, groups);
 }
 
 void driver_remove_groups(struct device_driver *drv,
 			  const struct attribute_group **groups)
 {
-	int i;
-
-	if (groups)
-		for (i = 0; groups[i]; i++)
-			sysfs_remove_group(&drv->p->kobj, groups[i]);
+	sysfs_remove_groups(&drv->p->kobj, groups);
 }
 
 /**

fs/sysfs/group.c +52 -0

64 lines not on this page · raw message

include/linux/sysfs.h +4 -0

@@ -215,10 +215,14 @@ void sysfs_delete_link(struct kobject *d
 
 int __must_check sysfs_create_group(struct kobject *kobj,
 				    const struct attribute_group *grp);
+int __must_check sysfs_create_groups(struct kobject *kobj,
+				     const struct attribute_group **groups);
 int sysfs_update_group(struct kobject *kobj,
 		       const struct attribute_group *grp);
 void sysfs_remove_group(struct kobject *kobj,
 			const struct attribute_group *grp);
+void sysfs_remove_groups(struct kobject *kobj,
+			 const struct attribute_group **groups);
 int sysfs_add_file_to_group(struct kobject *kobj,
 			const struct attribute *attr, const char *group);
 void sysfs_remove_file_from_group(struct kobject *kobj,

Randy Dunlap

2013-08-21 20:38 UTC #1 lore raw

Quoted 8 lines · Greg Kroah-Hartman

fs/sysfs/group.c +1 -0

@@ -131,6 +131,39 @@ int sysfs_create_group(struct kobject *k
 }
 
 /**
+ * sysfs_create_groups - given a directory kobject, create a bunch of attribute groups

fs/sysfs/group.c int sysfs_create_group(struct kobject *k · line 144

Quoted 10 lines

fs/sysfs/group.c +10 -0

+ * @kobj:	The kobject to create the group on
+ * @groups:	The attribute groups to create, NULL terminated
+ *
+ * This function creates a bunch of attribute groups.  If an error occurs when
+ * creating a group, all previously created groups will be removed, unwinding
+ * everything back to the original state when this function was called.
+ * It will explicitly warn and error if any of the attribute files being
+ * created already exist.
+ *
+ * Returns 0 on success or error.

ambiguous. How about:
Returns 0 on success or error code on error.

Quoted 20 lines

fs/sysfs/group.c +20 -0

+ */
+int sysfs_create_groups(struct kobject *kobj,
+			const struct attribute_group **groups)
+{
+	int error = 0;
+	int i;
+
+	if (!groups)
+		return 0;
+
+	for (i = 0; groups[i]; i++) {
+		error = sysfs_create_group(kobj, groups[i]);
+		if (error) {
+			while (--i >= 0)
+				sysfs_remove_group(kobj, groups[i]);
+			break;
+		}
+	}
+	return error;
+}

Signature · 1 line

~Randy

Greg Kroah-Hartman

author maintainer

2013-08-21 20:44 UTC #2 lore raw

Quoted 7 lines · Randy Dunlap

Quoted 6 lines · Greg Kroah-Hartman
fs/sysfs/group.c +0 -0
@@ -131,6 +131,39 @@ int sysfs_create_group(struct kobject *k
 }
 

Quoted 15 lines

fs/sysfs/group.c int sysfs_create_group(struct kobject *k · line 144

Quoted 12 lines

fs/sysfs/group.c +11 -0

 /**
+ * sysfs_create_groups - given a directory kobject, create a bunch of attribute groups
+ * @kobj:	The kobject to create the group on
+ * @groups:	The attribute groups to create, NULL terminated
+ *
+ * This function creates a bunch of attribute groups.  If an error occurs when
+ * creating a group, all previously created groups will be removed, unwinding
+ * everything back to the original state when this function was called.
+ * It will explicitly warn and error if any of the attribute files being
+ * created already exist.
+ *
+ * Returns 0 on success or error.

ambiguous. How about:
Returns 0 on success or error code on error.

Ok, I also forgot to export these symbols, so I'll go do a v2 of the patch, thanks for the review.

greg k-h

Greg Kroah-Hartman

author maintainer

2013-08-21 20:47 UTC #3 lore raw

PATCH 1/1 sysfs: add sysfs_create/remove_groups()

In reply to Randy Dunlap

From: Greg Kroah-Hartman <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
v2: - export new functions so modules can use them
- return value better documented thanks to Randy.

 drivers/base/bus.c    |   23 +--------------------
 drivers/base/core.c   |   22 +-------------------
 drivers/base/driver.c |   22 +-------------------
 fs/sysfs/group.c      |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sysfs.h |    4 +++
 5 files changed, 64 insertions(+), 61 deletions(-)

drivers/base/bus.c +2 ~~-21~~

@@ -884,32 +884,13 @@ static void bus_remove_attrs(struct bus_
 static int bus_add_groups(struct bus_type *bus,
 			  const struct attribute_group **groups)
 {
-	int error = 0;
-	int i;
-
-	if (groups) {
-		for (i = 0; groups[i]; i++) {
-			error = sysfs_create_group(&bus->p->subsys.kobj,
-						   groups[i]);
-			if (error) {
-				while (--i >= 0)
-					sysfs_remove_group(&bus->p->subsys.kobj,
-							   groups[i]);
-				break;
-			}
-		}
-	}
-	return error;
+	return sysfs_create_groups(&bus->p->subsys.kobj, groups);
 }
 
 static void bus_remove_groups(struct bus_type *bus,
 			      const struct attribute_group **groups)
 {
-	int i;
-
-	if (groups)
-		for (i = 0; groups[i]; i++)
-			sysfs_remove_group(&bus->p->subsys.kobj, groups[i]);
+	sysfs_remove_groups(&bus->p->subsys.kobj, groups);
 }
 
 static void klist_devices_get(struct klist_node *n)

drivers/base/core.c +2 ~~-20~~

@@ -493,31 +493,13 @@ static void device_remove_bin_attributes
 
 int device_add_groups(struct device *dev, const struct attribute_group **groups)
 {
-	int error = 0;
-	int i;
-
-	if (groups) {
-		for (i = 0; groups[i]; i++) {
-			error = sysfs_create_group(&dev->kobj, groups[i]);
-			if (error) {
-				while (--i >= 0)
-					sysfs_remove_group(&dev->kobj,
-							   groups[i]);
-				break;
-			}
-		}
-	}
-	return error;
+	return sysfs_create_groups(&dev->kobj, groups);
 }
 
 void device_remove_groups(struct device *dev,
 			  const struct attribute_group **groups)
 {
-	int i;
-
-	if (groups)
-		for (i = 0; groups[i]; i++)
-			sysfs_remove_group(&dev->kobj, groups[i]);
+	sysfs_remove_groups(&dev->kobj, groups);
 }
 
 static int device_add_attrs(struct device *dev)

drivers/base/driver.c +2 ~~-20~~

@@ -126,31 +126,13 @@ EXPORT_SYMBOL_GPL(driver_remove_file);
 int driver_add_groups(struct device_driver *drv,
 		      const struct attribute_group **groups)
 {
-	int error = 0;
-	int i;
-
-	if (groups) {
-		for (i = 0; groups[i]; i++) {
-			error = sysfs_create_group(&drv->p->kobj, groups[i]);
-			if (error) {
-				while (--i >= 0)
-					sysfs_remove_group(&drv->p->kobj,
-							   groups[i]);
-				break;
-			}
-		}
-	}
-	return error;
+	return sysfs_create_groups(&drv->p->kobj, groups);
 }
 
 void driver_remove_groups(struct device_driver *drv,
 			  const struct attribute_group **groups)
 {
-	int i;
-
-	if (groups)
-		for (i = 0; groups[i]; i++)
-			sysfs_remove_group(&drv->p->kobj, groups[i]);
+	sysfs_remove_groups(&drv->p->kobj, groups);
 }
 
 /**

fs/sysfs/group.c +54 -0

66 lines not on this page · raw message

include/linux/sysfs.h +4 -0

@@ -215,10 +215,14 @@ void sysfs_delete_link(struct kobject *d
 
 int __must_check sysfs_create_group(struct kobject *kobj,
 				    const struct attribute_group *grp);
+int __must_check sysfs_create_groups(struct kobject *kobj,
+				     const struct attribute_group **groups);
 int sysfs_update_group(struct kobject *kobj,
 		       const struct attribute_group *grp);
 void sysfs_remove_group(struct kobject *kobj,
 			const struct attribute_group *grp);
+void sysfs_remove_groups(struct kobject *kobj,
+			 const struct attribute_group **groups);
 int sysfs_add_file_to_group(struct kobject *kobj,
 			const struct attribute *attr, const char *group);
 void sysfs_remove_file_from_group(struct kobject *kobj,

Anthony Foiani

2013-08-22 05:03 UTC #4 lore raw

Quoted 11 lines · Greg Kroah-Hartman

+ * sysfs_create_groups - given a directory kobject, create a bunch of attribute groups
+ * @kobj: The kobject to create the group on
+ * @groups: The attribute groups to create, NULL terminated
+ *
+ * This function creates a bunch of attribute groups. If an error occurs when
+ * creating a group, all previously created groups will be removed, unwinding
+ * everything back to the original state when this function was called.
+ * It will explicitly warn and error if any of the attribute files being
+ * created already exist.
+ *
+ * Returns 0 on success or error code from sysfs_create_groups on error.

"... from sysfs_create_group on error"
^^^^^ (note singular, not plural)

Otherwise it's a bit of a tautology...

Maybe rename to "sysfs_(create|remove)_multiple_groups" (or ..._many_groups) to avoid this kind of one-char difference?

Quoted 6 lines

+ * sysfs_remove_groups - remove a list of groups
+ *
+ * kobj: The kobject for the groups to be removed from
+ * groups: NULL terminated list of groups to be removed
+ *
+ * If groups is not NULL, the all groups will be removed from the kobject

"If groups is not NULL, remove listed groups from the kobject"

(I also played around with using pointer arithmetic instead of array accesses for these two functions; it cut a few lines of code, but not enough to bother, I don't think.)

Thanks,
Tony

Greg Kroah-Hartman

author maintainer

2013-08-22 16:22 UTC #5 lore raw

Quoted 14 lines · Anthony Foiani

Quoted 11 lines · Greg Kroah-Hartman
+ * sysfs_create_groups - given a directory kobject, create a bunch of attribute groups
+ * @kobj: The kobject to create the group on
+ * @groups: The attribute groups to create, NULL terminated
+ *
+ * This function creates a bunch of attribute groups. If an error occurs when
+ * creating a group, all previously created groups will be removed, unwinding
+ * everything back to the original state when this function was called.
+ * It will explicitly warn and error if any of the attribute files being
+ * created already exist.
+ *
+ * Returns 0 on success or error code from sysfs_create_groups on error.

Quoted 4 lines

"... from sysfs_create_group on error"
^^^^^ (note singular, not plural)
Otherwise it's a bit of a tautology...

Ah, thanks, will fix that up.

Quoted 2 lines

Maybe rename to "sysfs_(create|remove)_multiple_groups" (or ..._many_groups) to avoid this kind of one-char difference?

Nah, terse is good :)

Quoted 8 lines

Quoted 6 lines
+ * sysfs_remove_groups - remove a list of groups
+ *
+ * kobj: The kobject for the groups to be removed from
+ * groups: NULL terminated list of groups to be removed
+ *
+ * If groups is not NULL, the all groups will be removed from the kobject
"If groups is not NULL, remove listed groups from the kobject"

Nice, will go fix it up now, thanks.

Quoted 3 lines

(I also played around with using pointer arithmetic instead of array accesses for these two functions; it cut a few lines of code, but not enough to bother, I don't think.)

Yeah, it's better to not be "tricky" so that others can review the code easier, and the end result would be the same.

thanks for the review.

greg k-h