sysfs: add filter function to groups

In the SCSI transport classes (and soon to be in the AEN event subsystem) we have a lot of need for a grouping that doesn't include all files in the group. We basically want to show capability by which file is present. A classic example of this is the SPI transport class connected to the 53c700 card. It's incapable of doing all of the modern LVD functions, so we don't show any of those capabilities in its sysfs directory. However, we have a lot of horrible logic to generate separate per host groupings of attributes for this. We would be able to use the standard sysfs group attributes *if* there were a way of filtering them so that certain attributes didn't appear.

This patch is a first pass at adding a filter function to the group attributes, just to see how the idea flies. If everyone's OK with this, I think the next thing that we might do is add bitmap functions (so every bit in the bitmap has a name, but also might not appear) to groups.

Quoted 10 lines · James Bottomley

In the SCSI transport classes (and soon to be in the AEN event subsystem) we have a lot of need for a grouping that doesn't include all files in the group. We basically want to show capability by which file is present. A classic example of this is the SPI transport class connected to the 53c700 card. It's incapable of doing all of the modern LVD functions, so we don't show any of those capabilities in its sysfs directory. However, we have a lot of horrible logic to generate separate per host groupings of attributes for this. We would be able to use the standard sysfs group attributes *if* there were a way of filtering them so that certain attributes didn't appear.

Sounds fine.

Quoted 5 lines

This patch is a first pass at adding a filter function to the group attributes, just to see how the idea flies. If everyone's OK with this, I think the next thing that we might do is add bitmap functions (so every bit in the bitmap has a name, but also might not appear) to groups.

Bitmaps in the attribute groups?

include/linux/sysfs.h struct attribute { · line 35

Quoted 3 lines

struct attribute_group {

include/linux/sysfs.h +1 -0

 	const char		*name;
+	int			(*filter_show)(struct kobject *, int);

Are you sure that you want to return an array index here, instead of the actual attribute? Like:

  int (*filter_show)(struct kobject *kobj, struct attribute *attr);

The names "show" and "store" are the ususal file-operation names, and we are not filtering a "show" here, right? Maybe "create", or "export", or something else might be a better name?

Quoted 7 lines · Kay Sievers

Quoted 5 lines · James Bottomley

This patch is a first pass at adding a filter function to the group attributes, just to see how the idea flies. If everyone's OK with this, I think the next thing that we might do is add bitmap functions (so every bit in the bitmap has a name, but also might not appear) to groups.

Quoted 1 line

Bitmaps in the attribute groups?

Actually, no ... that would spoil our one group for all devices rule. So they would be a set of helper functions for manipulating bitmaps, but the bitmap would have to be in separate storage elsewhere.

Quoted 7 lines

include/linux/sysfs.h struct attribute { · line 35

Quoted 4 lines

struct attribute_group {

include/linux/sysfs.h +1 -0

 	const char		*name;
+	int			(*filter_show)(struct kobject *, int);

Are you sure that you want to return an array index here, instead of the actual attribute? Like:

Actually, it returns a true/false value indicating whether the given attribute should be displayed.

Quoted 1 line

  int (*filter_show)(struct kobject *kobj, struct attribute *attr);

Quoted 4 lines

The names "show" and "store" are the ususal file-operation names, and we are not filtering a "show" here, right? Maybe "create", or "export", or something else might be a better name?

how about (*attribute_is_visible)?

Quoted 8 lines · James Bottomley

Quoted 7 lines · Kay Sievers

Quoted 5 lines · James Bottomley

This patch is a first pass at adding a filter function to the group attributes, just to see how the idea flies. If everyone's OK with this, I think the next thing that we might do is add bitmap functions (so every bit in the bitmap has a name, but also might not appear) to groups.

Quoted 5 lines

Quoted 1 line

Bitmaps in the attribute groups?

Actually, no ... that would spoil our one group for all devices rule. So they would be a set of helper functions for manipulating bitmaps, but the bitmap would have to be in separate storage elsewhere.

Can you determine which subset of the attributes you want just before actually creating the group? Then you could do something like:

create_group(grp, kobj)

{
	grp->update_creation_mask(kobj);
	actually_create_attrs();
}

Quoted 8 lines

Quoted 7 lines

include/linux/sysfs.h struct attribute { · line 35

Quoted 4 lines

struct attribute_group {

include/linux/sysfs.h +1 -0

 	const char		*name;
+	int			(*filter_show)(struct kobject *, int);

Are you sure that you want to return an array index here, instead of the actual attribute? Like:

Quoted 4 lines

Actually, it returns a true/false value indicating whether the given attribute should be displayed.

Quoted 1 line

  int (*filter_show)(struct kobject *kobj, struct attribute *attr);

I'd agree that using the attribute is better in this function.

Quoted 1 line

Quoted 1 line

Quoted 5 lines

Quoted 3 lines

The names "show" and "store" are the ususal file-operation names, and we are not filtering a "show" here, right? Maybe "create", or "export", or something else might be a better name?

how about (*attribute_is_visible)?

Well, you don't only stop the visibility, but the creation of the attribute, so perhaps (*creation_filter)?

visibility and creation are the same thing, aren't they? An invisible attribute doesn't appear in the sysfs directory, so it's equivalent to the file for it not being created.

Quoted 3 lines · James Bottomley

visibility and creation are the same thing, aren't they? An invisible attribute doesn't appear in the sysfs directory, so it's equivalent to the file for it not being created.

What about the case where it's visible at creation time, but then needs to be made selectively invisible later on?

That implies either a remove operation or dentry checks on each lookup?

Quoted 6 lines · Jeff Garzik

Quoted 3 lines · James Bottomley

visibility and creation are the same thing, aren't they? An invisible attribute doesn't appear in the sysfs directory, so it's equivalent to the file for it not being created.

Quoted 4 lines

What about the case where it's visible at creation time, but then needs to be made selectively invisible later on?

That implies either a remove operation or dentry checks on each lookup?

Yes, that comes with the bitmap manipulation code. There will be a way to add and remove runtime visibility. I just wanted to get the basic concept agreed to first.

Quoted 8 lines

Can you determine which subset of the attributes you want just before actually creating the group? Then you could do something like:

create_group(grp, kobj)

{
	grp->update_creation_mask(kobj);
	actually_create_attrs();
}

That's actually what we currently do (at least in hand coded form) in the current transport classes. However, it leads to one separate group for each attached class. With the filter approach, we only need one constructed group for every transport class.

Quoted 8 lines · Kay Sievers

include/linux/sysfs.h struct attribute { · line 35

Quoted 3 lines · James Bottomley

struct attribute_group {

include/linux/sysfs.h +1 -0

 	const char		*name;
+	int			(*filter_show)(struct kobject *, int);

Are you sure that you want to return an array index here, instead of the actual attribute? Like:

Actually, it returns a true/false value indicating whether the given attribute should be displayed.

Quoted 10 lines · James Bottomley

Quoted 8 lines · Kay Sievers

include/linux/sysfs.h struct attribute { · line 35

Quoted 3 lines · James Bottomley

struct attribute_group {

include/linux/sysfs.h +1 -0

 	const char		*name;
+	int			(*filter_show)(struct kobject *, int);

Are you sure that you want to return an array index here, instead of the actual attribute? Like:

Quoted 2 lines

Actually, it returns a true/false value indicating whether the given attribute should be displayed.

It isn't about the return value of the function, that's fine. You call back with the index number (int) of the array of attributes, instead of passing the attribute pointer (struct attribute *attr) back to ask the device for the attribute to create.

Quoted 11 lines · Kay Sievers

Quoted 10 lines · James Bottomley

Quoted 8 lines · Kay Sievers

include/linux/sysfs.h struct attribute { · line 35

Quoted 3 lines · James Bottomley

struct attribute_group {

include/linux/sysfs.h +1 -0

 	const char		*name;
+	int			(*filter_show)(struct kobject *, int);

Are you sure that you want to return an array index here, instead of the actual attribute? Like:

Quoted 7 lines

Quoted 2 lines

Actually, it returns a true/false value indicating whether the given attribute should be displayed.

It isn't about the return value of the function, that's fine. You call back with the index number (int) of the array of attributes, instead of passing the attribute pointer (struct attribute *attr) back to ask the device for the attribute to create.

For bitmaps, the int is what we will want. I can add both to the prototype if that will make you happy? so people searching on struct attribute and not relying on the array construction order can use the code as well.

Quoted 3 lines

include/linux/sysfs.h struct attribute { · line 35

Quoted 3 lines

struct attribute_group {

include/linux/sysfs.h +1 -0

 	const char		*name;
+	int			(*filter_show)(struct kobject *, int);

Actually, it returns a true/false value indicating whether the given attribute should be displayed.

Quoted 6 lines · James Bottomley

Quoted 3 lines

include/linux/sysfs.h struct attribute { · line 35

Quoted 3 lines

struct attribute_group {

include/linux/sysfs.h +1 -0

 	const char		*name;
+	int			(*filter_show)(struct kobject *, int);

Actually, it returns a true/false value indicating whether the given attribute should be displayed.

How about this:

	int (*is_visible)(...);
or
	bool (*shall_be_shown)(...);
or
	bool (*should_be_displayed)(...);

or whatever, so that it indicates that this function merely answers a question, but doesn't filter nor show anything.

Quoted 6 lines · James Bottomley

Quoted 3 lines

Review

Quoted 3 lines

struct attribute_group {

(quoted diff) +1 -0

 	const char		*name;
+	int			(*filter_show)(struct kobject *, int);

Actually, it returns a true/false value indicating whether the given attribute should be displayed.

How about this:

	int (*is_visible)(...);

OK, so is this latest revision acceptable to everyone?

Quoted 5 lines · Stefan Richter

Quoted 3 lines · James Bottomley

Quoted 3 lines

Quoted 3 lines

struct attribute_group {

(quoted diff) +1 -0

 	const char		*name;
+	int			(*filter_show)(struct kobject *, int);

Quoted 6 lines

Quoted 2 lines

Actually, it returns a true/false value indicating whether the given attribute should be displayed.

How about this:

	int (*is_visible)(...);

OK, so is this latest revision acceptable to everyone?

Quoted 5 lines · Stefan Richter

Quoted 3 lines · James Bottomley

Quoted 3 lines

Quoted 3 lines

struct attribute_group {

(quoted diff) +1 -0

 	const char		*name;
+	int			(*filter_show)(struct kobject *, int);

Quoted 6 lines

Quoted 2 lines

Actually, it returns a true/false value indicating whether the given attribute should be displayed.

How about this:

	int (*is_visible)(...);

OK, so is this latest revision acceptable to everyone?

Index: BUILD-2.6/fs/sysfs/group.c
===================================================================

fs/sysfs/group.c +16 -10

@@ -16,25 +16,31 @@
 #include "sysfs.h"
 
 
-static void remove_files(struct sysfs_dirent *dir_sd,
+static void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
 			 const struct attribute_group *grp)
 {
 	struct attribute *const* attr;
+	int i;
 
-	for (attr = grp->attrs; *attr; attr++)
-		sysfs_hash_and_remove(dir_sd, (*attr)->name);
+	for (i = 0, attr = grp->attrs; *attr; i++, attr++)
+		if (grp->is_visible &&
+		    grp->is_visible(kobj, *attr, i))
+			sysfs_hash_and_remove(dir_sd, (*attr)->name);
 }
 
-static int create_files(struct sysfs_dirent *dir_sd,
+static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
 			const struct attribute_group *grp)
 {
 	struct attribute *const* attr;
-	int error = 0;
+	int error = 0, i;
 
-	for (attr = grp->attrs; *attr && !error; attr++)
-		error = sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
+	for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++)
+		if (grp->is_visible &&
+		    grp->is_visible(kobj, *attr, i))
+			error |=
+				sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
 	if (error)
-		remove_files(dir_sd, grp);
+		remove_files(dir_sd, kobj, grp);
 	return error;
 }
 

@@ -54,7 +60,7 @@ int sysfs_create_group(struct kobject * 
 	} else
 		sd = kobj->sd;
 	sysfs_get(sd);
-	error = create_files(sd, grp);
+	error = create_files(sd, kobj, grp);
 	if (error) {
 		if (grp->name)
 			sysfs_remove_subdir(sd);

@@ -75,7 +81,7 @@ void sysfs_remove_group(struct kobject *
 	} else
 		sd = sysfs_get(dir_sd);
 
-	remove_files(sd, grp);
+	remove_files(sd, kobj, grp);
 	if (grp->name)
 		sysfs_remove_subdir(sd);
 

Index: BUILD-2.6/include/linux/sysfs.h
===================================================================

Quoted 11 lines · James Bottomley

Quoted 10 lines · Stefan Richter

Quoted 6 lines · James Bottomley

Quoted 3 lines

Quoted 3 lines

 struct attribute_group {
       const char              *name;

+ int (*filter_show)(struct kobject *, int);

Actually, it returns a true/false value indicating whether the given attribute should be displayed.

How about this:

Quoted 3 lines

Quoted 1 line

    int (*is_visible)(...);

OK, so is this latest revision acceptable to everyone?

I've just been hacking around in this area a bit, for a completely different
reason: there are literally 1000's of attributes in drivers/hwmon/*.c that really want to be const, but which cannot be due to the current API. I was about to propose some patches that move in a different direction...

IMHO the fundamental problem is struct attribute_group itself. This structure is nothing but a convenience for packaging the arguments to sysfs_create_group and sysfs_remove_group.

Those functions should take the contents of that
struct as direct arguments.

I haven't finished the patch series to implement
this, but since I noticed your patch I thought I'd better speak up now. Here's the first... the idea is to eventually deprecate sysfs_[create|remove]_group() altogether.

The current declaration of struct attribute_group prevents long lists of attributes from being marked const. Ideally, the second argument to the sysfs_create_group() and sysfs_remove_group() functions would be marked "deep" const, but C has no such construct. This patch provides a parallel set of functions with the desired decoration.

Quoted 14 lines · Mark M. Hoffman

Quoted 13 lines · James Bottomley

Quoted 11 lines · Stefan Richter

Quoted 6 lines · James Bottomley

Quoted 3 lines

Quoted 3 lines

 struct attribute_group {
       const char              *name;

+ int (*filter_show)(struct kobject *, int);

Actually, it returns a true/false value indicating whether the given attribute should be displayed.

How about this:

    int (*is_visible)(...);

Quoted 6 lines

Quoted 1 line

OK, so is this latest revision acceptable to everyone?

I've just been hacking around in this area a bit, for a completely different
reason: there are literally 1000's of attributes in drivers/hwmon/*.c that really want to be const, but which cannot be due to the current API. I was about to propose some patches that move in a different direction...

That isn't related to "dynamic attributes", right?

Quoted 3 lines

IMHO the fundamental problem is struct attribute_group itself. This structure is nothing but a convenience for packaging the arguments to sysfs_create_group and sysfs_remove_group.

That "problem" is actually a good thing. If you look at the change rate of the internal kernel API, it saves us so much trouble. Like in this case, James can just add a callback without caring about any (almost :)) of the current users.

Quoted 2 lines

Those functions should take the contents of that
struct as direct arguments.

I think we should move in the opposite direction. You are right, it isn't neccessarily pretty, but having encapsulations like this saves us a lot of trouble while interacting with so many other people and extending API's all the time. It's a trade, and it's a good one, if you need to maintain code that has so many callers, and so many architectures, you can't even check that you don't break them.

Quoted 4 lines

I haven't finished the patch series to implement
this, but since I noticed your patch I thought I'd better speak up now. Here's the first... the idea is to eventually deprecate sysfs_[create|remove]_group() altogether.

Again, I don't think, that we want to get rid of the struct container housing all the parameters and beeing open for future extensions without changing all the callers.

Quoted 5 lines

The current declaration of struct attribute_group prevents long lists of attributes from being marked const. Ideally, the second argument to the sysfs_create_group() and sysfs_remove_group() functions would be marked "deep" const, but C has no such construct. This patch provides a parallel set of functions with the desired decoration.

What do we get out of this constification compared to the current code?

Quoted 11 lines · Stefan Richter

Quoted 6 lines · James Bottomley

Quoted 3 lines

Review

Quoted 3 lines

struct attribute_group {

(quoted diff) +1 -0

 	const char		*name;
+	int			(*filter_show)(struct kobject *, int);

Actually, it returns a true/false value indicating whether the given attribute should be displayed.

How about this:

	int (*is_visible)(...);

OK, so is this latest revision acceptable to everyone?

James

Index: BUILD-2.6/fs/sysfs/group.c
===================================================================

fs/sysfs/group.c +1 -1

@@ -16,25 +16,31 @@
 #include "sysfs.h"
 
 
-static void remove_files(struct sysfs_dirent *dir_sd,
+static void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
 			 const struct attribute_group *grp)

fs/sysfs/group.c +1 -1

-static int create_files(struct sysfs_dirent *dir_sd,
+static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
 			const struct attribute_group *grp)
 {
 	struct attribute *const* attr;

Quoted 11 lines · James Bottomley

Index: BUILD-2.6/fs/sysfs/group.c
===================================================================

fs/sysfs/group.c +1 -1

@@ -16,25 +16,31 @@
 #include "sysfs.h"
 
 
-static void remove_files(struct sysfs_dirent *dir_sd,
+static void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
 			 const struct attribute_group *grp)

fs/sysfs/group.c line 28

Quoted 10 lines

fs/sysfs/group.c +5 -2

 {
 	struct attribute *const* attr;
+	int i;
 
-	for (attr = grp->attrs; *attr; attr++)
-		sysfs_hash_and_remove(dir_sd, (*attr)->name);
+	for (i = 0, attr = grp->attrs; *attr; i++, attr++)
+		if (grp->is_visible &&
+		    grp->is_visible(kobj, *attr, i))
+			sysfs_hash_and_remove(dir_sd, (*attr)->name);

Hm, doesn't this break for the zillions of attribute groups that do not have the is_visible function set?

Quoted 5 lines

fs/sysfs/group.c +1 -1

-static int create_files(struct sysfs_dirent *dir_sd,
+static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
 			const struct attribute_group *grp)
 {
 	struct attribute *const* attr;

fs/sysfs/group.c line 41

Quoted 10 lines

fs/sysfs/group.c +6 -3

-	int error = 0;
+	int error = 0, i;
 
-	for (attr = grp->attrs; *attr && !error; attr++)
-		error = sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
+	for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++)
+		if (grp->is_visible &&
+		    grp->is_visible(kobj, *attr, i))
+			error |=
+				sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);

Same problem here, if grp->is_visible is not set, sysfs_add_file() would never be called, right?

Other than the logic problem (I think), I have no issue with this idea at all. Care to redo this so it works?

Quoted 7 lines · Greg KH

fs/sysfs/group.c line 28

Quoted 4 lines · James Bottomley

fs/sysfs/group.c +4 -0

+	for (i = 0, attr = grp->attrs; *attr; i++, attr++)
+		if (grp->is_visible &&
+		    grp->is_visible(kobj, *attr, i))
+			sysfs_hash_and_remove(dir_sd, (*attr)->name);

Hm, doesn't this break for the zillions of attribute groups that do not have the is_visible function set?

Would it make more sense then to turn the meaning of the callback around?

for (...) {
	if (grp->mask_out && grp->mask_out(kobj, *attr, i))
		continue;
	error |= sysfs_add_file(...);
}

Quoted 8 lines · Cornelia Huck

Quoted 7 lines · Greg KH

fs/sysfs/group.c line 28

Quoted 4 lines · James Bottomley

fs/sysfs/group.c +4 -0

+	for (i = 0, attr = grp->attrs; *attr; i++, attr++)
+		if (grp->is_visible &&
+		    grp->is_visible(kobj, *attr, i))
+			sysfs_hash_and_remove(dir_sd, (*attr)->name);

Hm, doesn't this break for the zillions of attribute groups that do not have the is_visible function set?

...

Quoted 8 lines

Would it make more sense then to turn the meaning of the callback around?

for (...) {
	if (grp->mask_out && grp->mask_out(kobj, *attr, i))
		continue;
	error |= sysfs_add_file(...);
}

		if (!grp->is_visible ||
		    grp->is_visible(kobj, *attr, i))
			add or remove();

Quoted 4 lines · Stefan Richter

		if (!grp->is_visible ||
		    grp->is_visible(kobj, *attr, i))
			add or remove();

Hm, I find that a bit harder to parse...

mask_out() would also imply that the common use case is to have all attributes in the group created and that you need to take action to have an attribute not created.

Quoted 3 lines

mask_out() would also imply that the common use case is to have all attributes in the group created and that you need to take action to have an attribute not created.

Here you have a point. But James has a point too when he says:
| We basically want to show capability by which file is present.

Anyway, /if/ the reverse logic is preferred, it shouldn't be called "mask_out()" but rather "is_masked()" or "is_hidden()" or the like.

OSame problem here, if grp->is_visible is not set, sysfs_add_file() would never be called, right?

Other than the logic problem (I think), I have no issue with this idea at all. Care to redo this so it works?

Quoted 5 lines · Greg KH

OSame problem here, if grp->is_visible is not set, sysfs_add_file() would never be called, right?

Other than the logic problem (I think), I have no issue with this idea at all. Care to redo this so it works?

It's a fair cop govenor ... new patch attached.

Quoted 8 lines · James Bottomley

Quoted 5 lines · Greg KH

OSame problem here, if grp->is_visible is not set, sysfs_add_file() would never be called, right?

Other than the logic problem (I think), I have no issue with this idea at all. Care to redo this so it works?

It's a fair cop govenor ... new patch attached.

Much better, thanks :)

I have no problem with this, if you want to take this in your tree, please do and feel free to add:

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

to it, or I can take it in mine.

Quoted 16 lines · Greg KH

Quoted 8 lines · James Bottomley

Quoted 5 lines · Greg KH

OSame problem here, if grp->is_visible is not set, sysfs_add_file() would never be called, right?

Other than the logic problem (I think), I have no issue with this idea at all. Care to redo this so it works?

It's a fair cop govenor ... new patch attached.

Much better, thanks :)

I have no problem with this, if you want to take this in your tree, please do and feel free to add:

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

Quoted 1 line

to it, or I can take it in mine.

Actually, I think I have a use for it and the follow on patch (coming soon) to do attribute bitmaps in the SCSI tree, so I'd like to add your ack and take it through this tree.