Re: [PATCH] sysfs: add filter function to groups
Hi Kay:
* Kay Sievers <[EMAIL PROTECTED]> [2007-10-31 03:01:56 +0100]:
> On Oct 31, 2007 1:40 AM, Mark M. Hoffman <[EMAIL PROTECTED]> wrote:
> > * James Bottomley <[EMAIL PROTECTED]> [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
[EMAIL PROTECTED]
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sysfs: add filter function to groups
Hi James:
* James Bottomley <[EMAIL PROTECTED]> [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.0 -0500
> +++ BUILD-2.6/fs/sysfs/group.c2007-10-30 12:35:47.0 -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.0
> -0500
> +++ BUILD-2.6/include/linux/sysfs.h 2007-10-30 13:24:06.0 -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 <[EMAIL PROTECTED]>
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
