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

Kay Sievers
2007-10-31T02:02:15Z

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