Re: [PATCH] Permissions don't stick on ConfigFS attributes

[Daniel Phillips <phillips@istop.com>]

On Saturday 20 August 2005 13:33, Greg KH wrote:
> On Sat, Aug 20, 2005 at 01:23:29PM +1000, Daniel Phillips wrote:
> > On Saturday 20 August 2005 13:01, Greg KH wrote:
> > > On Sat, Aug 20, 2005 at 10:50:51AM +1000, Daniel Phillips wrote:
> > > > So: Integrate with sysfs.
> > >
> > > No, don't.  Do you think that Joel would not have already worked with
> > > the sysfs people prior to submitting this?  No, he did, and we all
> > > agreed that it should be kept separate.
> >
> > Would you care to recap the reasoning, please?
>
> They do two different things, and people interact with them in two
> different ways.

There is only one essential difference: the user can create nodes in configfs.  
Otherwise, the user does the same with both: display things and tweak things.  
The big difference is in your mind, not the user's mind.  And look above, 
you're saying "don't patch configfs, copy sysfs instead" and in the next 
breath you're saying "but oh they're completely different".

> > > > Terminology skew.  It is a very bad idea to call your configfs files
> > > > "attributes".
> > >
> > > That's what sysfs calls its files.  They used the same naming scheme
> > > there.  This is nothing that a user ever cares about or sees.
> >
> > It's wrrrrronnnggg.  The best you can defend this with is "it's
> > entrenched".
>
> Will a user ever see the word "attribute"?

For example:

   http://linuxcommand.org/man_pages/udev8.html

> Also, these files represent 
> attributes of the main object in which they are attached to.  Hence the
> name.

Yes, regrettable.  "Properties" would be tasteful and avoid the collision.

> > > > Memory requirements.  ConfigFS pins way too much kernel memory for
> > > > inodes and dentries.
> > >
> > > configfs is not going to have that many nodes at all in memory
> > > (compared to sysfs), so I don't think this is a big problem.
> >
> > The current bloat is unconscionable, for the amount of data that is
> > carried. Are you arguing against fixing it?  And what makes you think
> > configfs will never have lots of nodes?
>
> Doesn't it currently work the same way as sysfs with the backing store
> being created on the fly?

I haven't look at sysfs yet, but the backing store, which is just a tree of 
configfs_dirents that point at the corresponding dentries, is created on the 
fly.  Currently, directories are all pinned while only files are transient.  
There is no good reason for this as far as I can see, directories can be 
transient too.

What I did notice is that my fix for permissions is wrong.  I plugged the 
updated permissions back into the descriptor for all attributes of a config 
item - not what we want, the update needs to be restricted to just one 
attribute.  The right fix is to store the updated permissions in the 
configfs_dirent, which already has the field, it is just not used 
consistently.  See, the code is trying to say something to us, I think the 
message is "read me!"

> If not, it should be pretty simple to convert 
> over if people are really worried about memory consumption, but again,
> why don't we see how many nodes are really used on most systems (don't
> want to add more complexity and kernel code if you never really need
> it.)

It will not be hard to fix, the framework seems to be there already.  As far 
as complexity goes, it is trivial and the payoff is significant.  Cleaning up 
a bunch of little stylistic things will have a bigger effect on readability.

Regards,

Daniel