[PATCH] sysfs: add filter function to groups

[PATCH] sysfs: add filter function to groups

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

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.

The kzalloc in kernel/params.c is to stop the system crashing on boot
with a bogus filter function pointer.

Signed-off-by: James Bottomley <James.Bottomley@SteelEye.com>

----

There's one final thing; if you're OK with this, can you ack it and let
me take it through the SCSI tree?  we're building functionality that
will depend on this.

Thanks,

James

Index: BUILD-2.6/fs/sysfs/group.c
===================================================================
--- BUILD-2.6.orig/fs/sysfs/group.c	2007-10-28 17:27:04.000000000 -0500
+++ BUILD-2.6/fs/sysfs/group.c	2007-10-29 00:55:49.000000000 -0500
@@ -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->filter_show &&
+		    grp->filter_show(kobj, 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->filter_show &&
+		    grp->filter_show(kobj, 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
===================================================================
--- BUILD-2.6.orig/include/linux/sysfs.h	2007-10-28 17:20:06.000000000 -0500
+++ BUILD-2.6/include/linux/sysfs.h	2007-10-29 00:37:18.000000000 -0500
@@ -32,6 +32,7 @@ struct attribute {
 
 struct attribute_group {
 	const char		*name;
+	int			(*filter_show)(struct kobject *, int);
 	struct attribute	**attrs;
 };
 
Index: BUILD-2.6/kernel/params.c
===================================================================
--- BUILD-2.6.orig/kernel/params.c	2007-10-29 01:18:43.000000000 -0500
+++ BUILD-2.6/kernel/params.c	2007-10-29 01:18:49.000000000 -0500
@@ -472,7 +472,7 @@ param_sysfs_setup(struct module_kobject 
 			sizeof(mp->grp.attrs[0]));
 	size[1] = (valid_attrs + 1) * sizeof(mp->grp.attrs[0]);
 
-	mp = kmalloc(size[0] + size[1], GFP_KERNEL);
+	mp = kzalloc(size[0] + size[1], GFP_KERNEL);
 	if (!mp)
 		return ERR_PTR(-ENOMEM);
 

Re: [PATCH] sysfs: add filter function to groups

[Kay Sievers]

On Mon, 2007-10-29 at 10:16 -0500, James Bottomley wrote:
> 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.
 
> 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?

>  struct attribute_group {
>  	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?

Thanks,
Kay

Re: [PATCH] sysfs: add filter function to groups

[James Bottomley]

On Mon, 2007-10-29 at 17:54 +0100, Kay Sievers wrote:
> On Mon, 2007-10-29 at 10:16 -0500, James Bottomley wrote:
> > 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.
>  
> > 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?

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.

> >  struct attribute_group {
> >  	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.

>   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?

how about (*attribute_is_visible)?

James

Re: [PATCH] sysfs: add filter function to groups

[Cornelia Huck]

On Mon, 29 Oct 2007 11:57:51 -0500,
James Bottomley <James.Bottomley@SteelEye.com> wrote:

> On Mon, 2007-10-29 at 17:54 +0100, Kay Sievers wrote:
> > On Mon, 2007-10-29 at 10:16 -0500, James Bottomley wrote:
> > > 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?
> 
> 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();
}

> 
> > >  struct attribute_group {
> > >  	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.
> 
> >   int (*filter_show)(struct kobject *kobj, struct attribute *attr);

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

> > 
> > 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)?

Re: [PATCH] sysfs: add filter function to groups

[James Bottomley]

On Mon, 2007-10-29 at 18:18 +0100, Cornelia Huck wrote:
> On Mon, 29 Oct 2007 11:57:51 -0500,
> James Bottomley <James.Bottomley@SteelEye.com> wrote:
> 
> > On Mon, 2007-10-29 at 17:54 +0100, Kay Sievers wrote:
> > > On Mon, 2007-10-29 at 10:16 -0500, James Bottomley wrote:
> > > > 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?
> > 
> > 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();
> }

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.

> > 
> > > >  struct attribute_group {
> > > >  	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.
> > 
> > >   int (*filter_show)(struct kobject *kobj, struct attribute *attr);
> 
> I'd agree that using the attribute is better in this function.
> 
> > > 
> > > 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.

James

Re: [PATCH] sysfs: add filter function to groups

[Jeff Garzik]

James Bottomley wrote:
> 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?

	Jeff

Re: [PATCH] sysfs: add filter function to groups

[James Bottomley]

On Mon, 2007-10-29 at 13:27 -0400, Jeff Garzik wrote:
> James Bottomley wrote:
> > 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?

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.

James

Re: [PATCH] sysfs: add filter function to groups

[Cornelia Huck]

On Mon, 29 Oct 2007 12:29:27 -0500,
James Bottomley <James.Bottomley@SteelEye.com> wrote:

> On Mon, 2007-10-29 at 13:27 -0400, Jeff Garzik wrote:
> > James Bottomley wrote:
> > > 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?
> 
> 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.

But visibility always comes with creation or deletion of attributes?

Perhaps what we want is to move knowledge of the attributes to the
kobject (when attributes are created/deleted), while we decide on
visibilty of the attributes (creation/deletion of sysfs entries) based
on a filter (where visibility may change, while the attribute still
exists).

Re: [PATCH] sysfs: add filter function to groups

[Cornelia Huck]

On Mon, 29 Oct 2007 12:24:06 -0500,
James Bottomley <James.Bottomley@SteelEye.com> wrote:

> > 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.

I meant doing it in the core. You still have one group for all cases,
but immediately before creating the attributes, the core checks back
which ones it should create. (Of course, that doesn't solve your
problems if you dynamically want to change availability of attributes
later on. You would need a different mechanism for that.)

Re: [PATCH] sysfs: add filter function to groups

[Kay Sievers]

On Mon, 2007-10-29 at 11:57 -0500, James Bottomley wrote:
> On Mon, 2007-10-29 at 17:54 +0100, Kay Sievers wrote:
> > On Mon, 2007-10-29 at 10:16 -0500, James Bottomley wrote:

> > >  struct attribute_group {
> > >  	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.

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.

Kay

Re: [PATCH] sysfs: add filter function to groups

[James Bottomley]

On Mon, 2007-10-29 at 18:27 +0100, Kay Sievers wrote:
> On Mon, 2007-10-29 at 11:57 -0500, James Bottomley wrote:
> > On Mon, 2007-10-29 at 17:54 +0100, Kay Sievers wrote:
> > > On Mon, 2007-10-29 at 10:16 -0500, James Bottomley wrote:
> 
> > > >  struct attribute_group {
> > > >  	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.
> 
> 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.

James

Re: [PATCH] sysfs: add filter function to groups

[Kay Sievers]

On Mon, 2007-10-29 at 12:28 -0500, James Bottomley wrote:
> On Mon, 2007-10-29 at 18:27 +0100, Kay Sievers wrote:
> > On Mon, 2007-10-29 at 11:57 -0500, James Bottomley wrote:
> > > On Mon, 2007-10-29 at 17:54 +0100, Kay Sievers wrote:
> > > > On Mon, 2007-10-29 at 10:16 -0500, James Bottomley wrote:
> > 
> > > > >  struct attribute_group {
> > > > >  	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.
> > 
> > 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.

Having both parameters sounds fine. Otherwise, if the code is spread
around several files, the attribute arrays would need to be global to
handle the callbacks, which isn't too nice.

Thanks,
Kay

Re: [PATCH] sysfs: add filter function to groups

[Stefan Richter]

James Bottomley wrote:
>> >  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)(...);
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.
-- 
Stefan Richter
-=====-=-=== =-=- ===-=
http://arcgraph.de/sr/

Re: [PATCH] sysfs: add filter function to groups

[James Bottomley]

On Mon, 2007-10-29 at 18:58 +0100, Stefan Richter wrote:
> James Bottomley wrote:
> >> >  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)(...);
> 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.

Actually, as long as it's descriptive, I really don't care about the
name.  Can we all agree on "is_visible"?

James

Re: [PATCH] sysfs: add filter function to groups

[James Bottomley]

On Mon, 2007-10-29 at 18:58 +0100, Stefan Richter wrote:
> James Bottomley wrote:
> >> >  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)(...);

OK, so is this latest revision acceptable to everyone?

James

Index: BUILD-2.6/fs/sysfs/group.c
===================================================================
--- BUILD-2.6.orig/fs/sysfs/group.c	2007-10-28 17:27:04.000000000 -0500
+++ BUILD-2.6/fs/sysfs/group.c	2007-10-30 12:35:47.000000000 -0500
@@ -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
===================================================================
--- BUILD-2.6.orig/include/linux/sysfs.h	2007-10-28 17:20:06.000000000 -0500
+++ BUILD-2.6/include/linux/sysfs.h	2007-10-30 13:24:06.000000000 -0500
@@ -32,6 +32,8 @@ struct attribute {
 
 struct attribute_group {
 	const char		*name;
+	int			(*is_visible)(struct kobject *,
+					      struct attribute *, int);
 	struct attribute	**attrs;
 };
 
Index: BUILD-2.6/kernel/params.c
===================================================================
--- BUILD-2.6.orig/kernel/params.c	2007-10-29 01:18:43.000000000 -0500
+++ BUILD-2.6/kernel/params.c	2007-10-29 01:18:49.000000000 -0500
@@ -472,7 +472,7 @@ param_sysfs_setup(struct module_kobject 
 			sizeof(mp->grp.attrs[0]));
 	size[1] = (valid_attrs + 1) * sizeof(mp->grp.attrs[0]);
 
-	mp = kmalloc(size[0] + size[1], GFP_KERNEL);
+	mp = kzalloc(size[0] + size[1], GFP_KERNEL);
 	if (!mp)
 		return ERR_PTR(-ENOMEM);
 

Re: [PATCH] sysfs: add filter function to groups

[Stefan Richter]

James Bottomley wrote:
> OK, so is this latest revision acceptable to everyone?

No complaint from me.  (I'm more or less by accident in this thread
anyway.  Once this feature is available in mainline, I may have use for
it in drivers/firewire/ though.)  Thanks,
-- 
Stefan Richter
-=====-=-=== =-=- ====-
http://arcgraph.de/sr/

Re: [PATCH] sysfs: add filter function to groups

[Kay Sievers]

On Tue, 2007-10-30 at 13:25 -0500, James Bottomley wrote:
> On Mon, 2007-10-29 at 18:58 +0100, Stefan Richter wrote:
> > James Bottomley wrote:
> > >> >  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)(...);
> 
> OK, so is this latest revision acceptable to everyone?

Acked-by: Kay Sievers <kay.sievers@vrfy.org>

Best,
Kay

Re: [PATCH] sysfs: add filter function to groups

[Mark M. Hoffman]

Hi James:

* James Bottomley <James.Bottomley@SteelEye.com> [2007-10-30 13:25:43 -0500]:
> On Mon, 2007-10-29 at 18:58 +0100, Stefan Richter wrote:
> > James Bottomley wrote:
> > >> >  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)(...);
> 
> 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...

> Index: BUILD-2.6/fs/sysfs/group.c
> ===================================================================
> --- BUILD-2.6.orig/fs/sysfs/group.c	2007-10-28 17:27:04.000000000 -0500
> +++ BUILD-2.6/fs/sysfs/group.c	2007-10-30 12:35:47.000000000 -0500
> @@ -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
> ===================================================================
> --- BUILD-2.6.orig/include/linux/sysfs.h	2007-10-28 17:20:06.000000000 -0500
> +++ BUILD-2.6/include/linux/sysfs.h	2007-10-30 13:24:06.000000000 -0500
> @@ -32,6 +32,8 @@ struct attribute {
>  
>  struct attribute_group {
>  	const char		*name;
> +	int			(*is_visible)(struct kobject *,
> +					      struct attribute *, int);
>  	struct attribute	**attrs;
>  };

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.

commit 5b5bc08ae31519b7012d7fc23ff73e0c6474de53
Author: Mark M. Hoffman <mhoffman@lightlink.com>
Date:   Sun Oct 21 11:49:57 2007 -0400

    sysfs: allow attribute (group) lists to be const
    
    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.
    
    Signed-off-by: Mark M. Hoffman <mhoffman@lightlink.com>

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index d197237..b217a8e 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -17,71 +17,84 @@
 
 
 static void remove_files(struct sysfs_dirent *dir_sd,
-			 const struct attribute_group *grp)
+			 const struct attribute * const * attrs)
 {
-	struct attribute *const* attr;
+	const struct attribute *const* attr;
 
-	for (attr = grp->attrs; *attr; attr++)
+	for (attr = attrs; *attr; attr++)
 		sysfs_hash_and_remove(dir_sd, (*attr)->name);
 }
 
 static int create_files(struct sysfs_dirent *dir_sd,
-			const struct attribute_group *grp)
+			const struct attribute * const * attrs)
 {
-	struct attribute *const* attr;
+	const struct attribute *const* attr;
 	int error = 0;
 
-	for (attr = grp->attrs; *attr && !error; attr++)
+	for (attr = attrs; *attr && !error; attr++)
 		error = sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
 	if (error)
-		remove_files(dir_sd, grp);
+		remove_files(dir_sd, attrs);
 	return error;
 }
 
-
-int sysfs_create_group(struct kobject * kobj, 
-		       const struct attribute_group * grp)
+int sysfs_create_attr_group(struct kobject * kobj,
+		const char *name, const struct attribute * const * attrs)
 {
 	struct sysfs_dirent *sd;
 	int error;
 
 	BUG_ON(!kobj || !kobj->sd);
 
-	if (grp->name) {
-		error = sysfs_create_subdir(kobj, grp->name, &sd);
+	if (name) {
+		error = sysfs_create_subdir(kobj, name, &sd);
 		if (error)
 			return error;
 	} else
 		sd = kobj->sd;
 	sysfs_get(sd);
-	error = create_files(sd, grp);
-	if (error) {
-		if (grp->name)
-			sysfs_remove_subdir(sd);
-	}
+	error = create_files(sd, attrs);
+	if (error && name)
+		sysfs_remove_subdir(sd);
+
 	sysfs_put(sd);
 	return error;
 }
 
-void sysfs_remove_group(struct kobject * kobj, 
-			const struct attribute_group * grp)
+int sysfs_create_group (struct kobject * kobj,
+	const struct attribute_group *grp)
+{
+	return sysfs_create_attr_group(kobj, grp->name,
+			(const struct attribute * const *)grp->attrs);
+}
+
+void sysfs_remove_attr_group(struct kobject * kobj,
+		const char *name, const struct attribute * const * attrs)
 {
 	struct sysfs_dirent *dir_sd = kobj->sd;
 	struct sysfs_dirent *sd;
 
-	if (grp->name) {
-		sd = sysfs_get_dirent(dir_sd, grp->name);
+	if (name) {
+		sd = sysfs_get_dirent(dir_sd, name);
 		BUG_ON(!sd);
 	} else
 		sd = sysfs_get(dir_sd);
 
-	remove_files(sd, grp);
-	if (grp->name)
+	remove_files(sd, attrs);
+	if (name)
 		sysfs_remove_subdir(sd);
 
 	sysfs_put(sd);
 }
 
+void sysfs_remove_group(struct kobject *kobj,
+			const struct attribute_group *grp)
+{
+	return sysfs_remove_attr_group(kobj, grp->name,
+			(const struct attribute * const *)grp->attrs);
+}
 
+EXPORT_SYMBOL_GPL(sysfs_create_attr_group);
+EXPORT_SYMBOL_GPL(sysfs_remove_attr_group);
 EXPORT_SYMBOL_GPL(sysfs_create_group);
 EXPORT_SYMBOL_GPL(sysfs_remove_group);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 149ab62..5066d50 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -101,10 +101,18 @@ int __must_check sysfs_create_link(struct kobject *kobj, struct kobject *target,
 				   const char *name);
 void sysfs_remove_link(struct kobject *kobj, const char *name);
 
+int __must_check sysfs_create_attr_group(struct kobject *,
+			const char * name, const struct attribute * const * attrs);
+
 int __must_check sysfs_create_group(struct kobject *kobj,
-				    const struct attribute_group *grp);
+					const struct attribute_group *grp);
+
+void sysfs_remove_attr_group(struct kobject *,
+			const char * name, const struct attribute * const * attrs);
+
 void sysfs_remove_group(struct kobject *kobj,
-			const struct attribute_group *grp);
+					const struct attribute_group *grp);
+
 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,
@@ -190,8 +198,19 @@ static inline int sysfs_create_group(struct kobject *kobj,
 	return 0;
 }
 
-static inline void sysfs_remove_group(struct kobject *kobj,
-				      const struct attribute_group *grp)
+static inline int sysfs_create_attr_group(struct kobject *k,
+			const char * name, const struct attribute * const * attrs)
+{
+	return 0;
+}
+
+static inline void sysfs_remove_group(struct kobject * k, const struct attribute_group * g)
+{
+	;
+}
+
+static inline void sysfs_remove_attr_group(struct kobject * k,
+			const char * name, const struct attribute * const * attrs)
 {
 	;
 }

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

Re: [PATCH] sysfs: add filter function to groups

[Kay Sievers]

On Oct 31, 2007 1:40 AM, Mark M. Hoffman <mhoffman@lightlink.com> wrote:
> * James Bottomley <James.Bottomley@SteelEye.com> [2007-10-30 13:25:43 -0500]:
> > On Mon, 2007-10-29 at 18:58 +0100, Stefan Richter wrote:
> > > James Bottomley wrote:
> > > >> >  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)(...);
> >
> > 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?

> 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.

> 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.

> 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.

>     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?

Thanks,
Kay

Re: [PATCH] sysfs: add filter function to groups

[Mark M. Hoffman]

Hi Kay:

* Kay Sievers <kay.sievers@vrfy.org> [2007-10-31 03:01:56 +0100]:
> On Oct 31, 2007 1:40 AM, Mark M. Hoffman <mhoffman@lightlink.com> wrote:
> > * James Bottomley <James.Bottomley@SteelEye.com> [2007-10-30 13:25:43 -0500]:
> > > On Mon, 2007-10-29 at 18:58 +0100, Stefan Richter wrote:
> > > > James Bottomley wrote:
> > > > >> >  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)(...);
> > >
> > > 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?

Right, it's not.  That phrase was coined by the previous maintainer, Jean, to
describe a specific mechanism which makes for smaller code in drivers/hwmon.

> > 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.

Given my patch, he could also just add sysfs_create_attr_group_filtered(...).
This would affect current users even less than what you suggest because it
wouldn't bloat the struct that they all use.

> > 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.

Again, I don't see how it is better to bloat a struct with many users instead
of just adding a function for the few that need the extra functionality.  My
patch also requires 0 change for everyone else.

> > 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.

BTW: I hope you're not resisting based on the prospect of deprecating those
two functions.  I don't really have time to push such a huge patch either.
It wouldn't hurt just to keep them.

> >     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?

Conceptual correctness, for one, but also it allows hwmon drivers to move as
much as 1K each out of the data section and into text.  This is useful for the
embedded XIP scenario.

And please don't underestimate conceptual correctness: at least hwmon (and
probably many other users) really *rely* on the core not to modify the contents
of struct attribute_group (specifically, the data to which its members point).

Without the proper const qualifiers, this is not externally guaranteed.

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

Re: [PATCH] sysfs: add filter function to groups

[Greg KH]

On Tue, Oct 30, 2007 at 01:25:43PM -0500, James Bottomley wrote:
> On Mon, 2007-10-29 at 18:58 +0100, Stefan Richter wrote:
> > James Bottomley wrote:
> > >> >  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)(...);
> 
> OK, so is this latest revision acceptable to everyone?
> 
> James
> 
> Index: BUILD-2.6/fs/sysfs/group.c
> ===================================================================
> --- BUILD-2.6.orig/fs/sysfs/group.c	2007-10-28 17:27:04.000000000 -0500
> +++ BUILD-2.6/fs/sysfs/group.c	2007-10-30 12:35:47.000000000 -0500
> @@ -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);

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

> -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);

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?

thanks,

greg k-h

Re: [PATCH] sysfs: add filter function to groups

[Cornelia Huck]

On Tue, 30 Oct 2007 20:55:06 -0700,
Greg KH <greg@kroah.com> wrote:

> On Tue, Oct 30, 2007 at 01:25:43PM -0500, James Bottomley wrote:
> > Index: BUILD-2.6/fs/sysfs/group.c
> > ===================================================================
> > --- BUILD-2.6.orig/fs/sysfs/group.c	2007-10-28 17:27:04.000000000 -0500
> > +++ BUILD-2.6/fs/sysfs/group.c	2007-10-30 12:35:47.000000000 -0500
> > @@ -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);
> 
> Hm, doesn't this break for the zillions of attribute groups that do not
> have the is_visible function set?
> 
> > -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);
> 
> 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?

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(...);
}

Re: [PATCH] sysfs: add filter function to groups

[Stefan Richter]

Cornelia Huck wrote:
> Greg KH <greg@kroah.com> wrote:
>> On Tue, Oct 30, 2007 at 01:25:43PM -0500, James Bottomley wrote:
>>> +	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(...);
> }

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

-- 
Stefan Richter
-=====-=-=== =-=- =====
http://arcgraph.de/sr/

Re: [PATCH] sysfs: add filter function to groups

[Cornelia Huck]

On Wed, 31 Oct 2007 10:52:35 +0100,
Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> Cornelia Huck wrote:
> > Greg KH <greg@kroah.com> wrote:
> >> On Tue, Oct 30, 2007 at 01:25:43PM -0500, James Bottomley wrote:
> >>> +	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(...);
> > }
> 
> 		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.

Re: [PATCH] sysfs: add filter function to groups

[Stefan Richter]

Cornelia Huck wrote:
> On Wed, 31 Oct 2007 10:52:35 +0100,
> Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
>> 		if (!grp->is_visible ||
>> 		    grp->is_visible(kobj, *attr, i))
>> 			add or remove();
>>
> 
> Hm, I find that a bit harder to parse...

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

However, how beautiful the implementation of static void remove_files()
and static int create_files() looks doesn't matter.  What's important is
that

 struct attribute_group {
 	const char		*name;
+	int			(*is_visible)(struct kobject *,
+					      struct attribute *, int);
 	struct attribute	**attrs;
 };

makes sense to users --- because this is the API.

[BTW, like most of the existing driver core APIs, there are kerneldoc
comments missing here.]

> 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.
-- 
Stefan Richter
-=====-=-=== =-=- =====
http://arcgraph.de/sr/

Re: [PATCH] sysfs: add filter function to groups

[Cornelia Huck]

On Wed, 31 Oct 2007 11:37:36 +0100,
Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> > 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.

Currently, if you register an attribute group, all of the attributes
will show up. I find it more intuitive to have to block some attributes
explicitely if I want to have more control, but the original logic is
fine with me as well, if most people prefer that :)

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

Sure. is_masked() would be fine.

Re: [PATCH] sysfs: add filter function to groups

[James Bottomley]

On Tue, 2007-10-30 at 20:55 -0700, Greg KH wrote:
> 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.

James

---
Index: BUILD-2.6/fs/sysfs/group.c
===================================================================
--- BUILD-2.6.orig/fs/sysfs/group.c	2007-10-28 17:27:04.000000000 -0500
+++ BUILD-2.6/fs/sysfs/group.c	2007-10-31 09:29:14.000000000 -0500
@@ -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
===================================================================
--- BUILD-2.6.orig/include/linux/sysfs.h	2007-10-28 17:20:06.000000000 -0500
+++ BUILD-2.6/include/linux/sysfs.h	2007-10-30 13:24:06.000000000 -0500
@@ -32,6 +32,8 @@ struct attribute {
 
 struct attribute_group {
 	const char		*name;
+	int			(*is_visible)(struct kobject *,
+					      struct attribute *, int);
 	struct attribute	**attrs;
 };
 
Index: BUILD-2.6/kernel/params.c
===================================================================
--- BUILD-2.6.orig/kernel/params.c	2007-10-29 01:18:43.000000000 -0500
+++ BUILD-2.6/kernel/params.c	2007-10-29 01:18:49.000000000 -0500
@@ -472,7 +472,7 @@ param_sysfs_setup(struct module_kobject 
 			sizeof(mp->grp.attrs[0]));
 	size[1] = (valid_attrs + 1) * sizeof(mp->grp.attrs[0]);
 
-	mp = kmalloc(size[0] + size[1], GFP_KERNEL);
+	mp = kzalloc(size[0] + size[1], GFP_KERNEL);
 	if (!mp)
 		return ERR_PTR(-ENOMEM);
 

Re: [PATCH] sysfs: add filter function to groups

[Greg KH]

On Wed, Oct 31, 2007 at 09:38:04AM -0500, James Bottomley wrote:
> On Tue, 2007-10-30 at 20:55 -0700, Greg KH wrote:
> > 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 <gregkh@suse.de>

to it, or I can take it in mine.

thanks,

greg k-h

Re: [PATCH] sysfs: add filter function to groups

[James Bottomley]

On Wed, 2007-10-31 at 10:29 -0700, Greg KH wrote:
> On Wed, Oct 31, 2007 at 09:38:04AM -0500, James Bottomley wrote:
> > On Tue, 2007-10-30 at 20:55 -0700, Greg KH wrote:
> > > 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 <gregkh@suse.de>
> 
> 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.

Thanks,

James

Re: [PATCH] sysfs: add filter function to groups

[Greg KH]

On Sun, Nov 04, 2007 at 08:12:02AM -0600, James Bottomley wrote:
> On Wed, 2007-10-31 at 10:29 -0700, Greg KH wrote:
> > On Wed, Oct 31, 2007 at 09:38:04AM -0500, James Bottomley wrote:
> > > On Tue, 2007-10-30 at 20:55 -0700, Greg KH wrote:
> > > > 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 <gregkh@suse.de>
> > 
> > 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.

That's fine.  Can you CC: me on the patch that uses this code so I can
see how well it works out?

thanks,

greg k-h