LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* configfs: return value for drop_item()/make_item()?
@ 2007-01-15 10:55 Michael Noisternig
  2007-01-16  8:30 ` Joel Becker
  0 siblings, 1 reply; 8+ messages in thread
From: Michael Noisternig @ 2007-01-15 10:55 UTC (permalink / raw)
  To: linux-kernel

Hi again,

I've posted this before but without getting any replies. Please, 
somebody either give a (short) reply to this or simply explain why they 
think this is OT or not worth answering...

Here's the issue... the configfs system can prevent a user from 
_creating_ sub-directories in a certain directory (by not supplying 
struct configfs_group_operations->make_item()/make_group()) but it 
cannot prevent him from _removing_ sub-directories because struct 
configfs_group_operations->drop_item() is void.

Similarly, struct configfs_group_operations->make_item() does not permit 
to return an error code, and as such it is impossible to 'check' the 
type of sub-directory a user wants to create (with returning a 
meaningful error code). This had already, unsuccessfully, been brought 
up before on 2006-08-29 by J. Berg (see 
http://marc.theaimsgroup.com/?l=linux-kernel&m=115692319227688&w=2).

Please give some arguments why you think 
configfs_group_operations->drop_item() should remain void.

Thank you very much in advance!

Here's the problem I ran into, explaining further...

(1)
Say the user creates one object, let's say as objects/myobj1/. This 
object is dependent on some (shared) parameters which the user created 
under params/myparams1/. Now while myobj1/ is 'active', I don't want to 
let the user remove myparams1/. I can prevent this by making the user 
create a symlink(2) in the objects/myobj1/ directory to myparams1/, i.e. 
objects/myobj1/params/ -> ../../params/myparams1/, to denote its use. 
Now - if I have read the documentation correctly - the user cannot 
remove myparams1/ without removing the params/ link first. So fine, so good.

(2)
Next the user may create several objects which may be dependent on 
several params objects. Now I can solve this by creating a default group 
for each object, i.e. on myobj1 creation there is objects/myobj1/params/ 
automatically. In that directory the user may create symlink(2)s to 
several params/*/ dirs. Fine again.

(3)
Now what I want is the list of params an object uses to be an ordered 
list. I cannot do this because there is no intrinsic order in a 
filesystem. I can get the order by instead having an attribute called 
param_list which contains the ordered list of all params to use, e.g.
 > cat param_list
  myparams2
  myparams4
  myparams1
However, this way I don't have any way to prevent the user from removing 
params because configfs_group_operations->drop_item() is void and does 
not allow me to return an error.

So what do you think would be an appropriate solution for the problem?

1)
Change configfs' configfs_group_operations->drop_item() to return an 
error code, so the user can be prevented from removing a directory in use.

2)
Keep a reference on each configfs object (e.g. params) in use, so when 
the user removes it from configfs it is still present in memory. This, 
however, requires to rename each according entry in every affected 
param_list to <params removed> or similar... not a very user-friendly 
way to deal with the situation.

3)
Change configfs so a user cannot remove directories with additional 
references on it.

4)
Trace back every object that relies on the params object about to be 
removed, and delete the according params entry from the object's 
params_list automatically... a solution with potentially unexpected side 
effects.

Thank you again for any and every feedback!!!

PS: Please CC me on your replies, I'm not a LKML subscriber.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: configfs: return value for drop_item()/make_item()?
  2007-01-15 10:55 configfs: return value for drop_item()/make_item()? Michael Noisternig
@ 2007-01-16  8:30 ` Joel Becker
  2007-01-16 16:42   ` Michael Noisternig
  0 siblings, 1 reply; 8+ messages in thread
From: Joel Becker @ 2007-01-16  8:30 UTC (permalink / raw)
  To: Michael Noisternig; +Cc: linux-kernel

On Mon, Jan 15, 2007 at 11:55:20AM +0100, Michael Noisternig wrote:
> I've posted this before but without getting any replies. Please, 
> somebody either give a (short) reply to this or simply explain why they 
> think this is OT or not worth answering...

	I'm sorry if I missed your previous post.  I've never ignored a
configfs post on purpose :-)

> Here's the issue... the configfs system can prevent a user from 
> _creating_ sub-directories in a certain directory (by not supplying 
> struct configfs_group_operations->make_item()/make_group()) but it 
> cannot prevent him from _removing_ sub-directories because struct 
> configfs_group_operations->drop_item() is void.

	This is intentional.  The entire point of configfs is to have
the lifetime controlled from userspace.  If the kernel can pin things
however it likes, we lose that property.

> Similarly, struct configfs_group_operations->make_item() does not permit 
> to return an error code, and as such it is impossible to 'check' the 
> type of sub-directory a user wants to create (with returning a 
> meaningful error code). This had already, unsuccessfully, been brought 
> up before on 2006-08-29 by J. Berg (see 
> http://marc.theaimsgroup.com/?l=linux-kernel&m=115692319227688&w=2).

	I don't quite see how this link relates, but perhaps I'm not
understanding your entire question.  Currently, ->make_item() doesn't
allow an error code because that would involve the ERR_PTR() construct.
While that construct is quite useful, I'm always wary of adding
complexity.  Thus, the tradeoff would have to be worth it.
	What would you like to signify with a ->make_item() error code?
I don't see how it 'checks' a type of sub-directory.  A particular
config_group can only create a particular type of sub-directory, so I'm
confused.  Maybe some more explanation of what you are trying to do will
help me understand.  Then I can see how configfs fits with your needs
and where it may not.

> Please give some arguments why you think 
> configfs_group_operations->drop_item() should remain void.

	Very simply, the lifetime must be in the control of userspace.
That's precisely the point of configfs.  If you'd like the kernel to
control the lifetime, I suspect sysfs will be more to your liking.
configfs is not meant to replace sysfs, merely to coexist for things
that benefit from the configfs model and not the sysfs model.
	(More on the model and your problem near the bottom)

> (1)
> Say the user creates one object, let's say as objects/myobj1/. This 
> object is dependent on some (shared) parameters which the user created 
> under params/myparams1/. Now while myobj1/ is 'active', I don't want to 
> let the user remove myparams1/. I can prevent this by making the user 
> create a symlink(2) in the objects/myobj1/ directory to myparams1/, i.e. 
> objects/myobj1/params/ -> ../../params/myparams1/, to denote its use. 
> Now - if I have read the documentation correctly - the user cannot 
> remove myparams1/ without removing the params/ link first. So fine, so good.

	This will certainly work.

> (2)
> Next the user may create several objects which may be dependent on 
> several params objects. Now I can solve this by creating a default group 
> for each object, i.e. on myobj1 creation there is objects/myobj1/params/ 
> automatically. In that directory the user may create symlink(2)s to 
> several params/*/ dirs. Fine again.
> 
> (3)
> Now what I want is the list of params an object uses to be an ordered 
> list. I cannot do this because there is no intrinsic order in a 
> filesystem. I can get the order by instead having an attribute called 
> param_list which contains the ordered list of all params to use, e.g.

	Why does it need to be ordered?  Also, why can't the symlinks
have a naming convention of order?  Sure, someone hacking at it can
break that, but your tool that really does the work can create
objects/myobj1/params/param1 -> myparams2.  So the listing does have
meaning.  Just one possibility.

> However, this way I don't have any way to prevent the user from removing 
> params because configfs_group_operations->drop_item() is void and does 
> not allow me to return an error.

	Um, with the symlinks pinning the params, they'll not be
removed.  Unless you were creating a param_list without creating the
symlinks.

> 2)
> Keep a reference on each configfs object (e.g. params) in use, so when 
> the user removes it from configfs it is still present in memory. This, 
> however, requires to rename each according entry in every affected 
> param_list to <params removed> or similar... not a very user-friendly 
> way to deal with the situation.

	There are two ways to reference the params.  One is to let the
userspace program ask for the reference via symlink.  The other is in
your driver.  This has the property you describe above, that you have
to keep track of it after the user has asked for it to go away.
	This property is on purpose.  When a user asks for something to
go away, it needs to disappear from configfs before the rmdir(2)
returns.  It's gone, and it is no longer accessible from userspace.  The
lifetiming of userspace access is clear, cut, and dried.
	However, some objects need some time to die.  So the driver has
its own reference, and it schedules any cleanup and death later.  But
that death and cleanup should happen as soon as possible, because
userspace has asked for it.

> 3)
> Change configfs so a user cannot remove directories with additional 
> references on it.

	You mean kernel-side references.  Again, you are asking for the
kernel to control the lifetime.

> 4)
> Trace back every object that relies on the params object about to be 
> removed, and delete the according params entry from the object's 
> params_list automatically... a solution with potentially unexpected side 
> effects.

	This, as you are certainly aware, is a non-starter.  It just
won't work well.

	Let's break for a minute.  Let me try to restate all of this.
You have an interrelationship between a few configfs objects.  When a
[referrer -> reference] relationship is put together, you need to prevent
the reference from going away.  You've proposed a couple of methods that
boil down to "when the reference object knows it is being referred to,
it prevents itself from going away".
	Therein lies the rub.  configfs works by putting all of that in
userspace.  Lifetime decisions can only occur in userspace.  This allows
us a very clean lifetime from the userspace perspective.  It also means
that if we want to lock an object down, it has to happen in userspace.
	So, the object cannot "prevent itself from going away".  The
kernel side just can't do this.  Userspace, however, can.  You've
discussed the most common way configfs allows userspace to do this, the
symlink.  I believe your only problem was that of naming.  There's no
reason you can't populate a param_list file at the same time you respond
to ->allow_link().  Or, you could have userspace name the links in a
manner providing ordering.  These solve this problem without breaking
the configfs contract.
	Think about your requirements again.  Do you absolutely require
that some part of your kernel driver make a lifetime decision?  If so,
configfs isn't going to fit.  Can the lifetime decision be moved to
userspace?  If so, configfs should work just fine as-is, and you'll gain
the clean lifetime and simple manipulation that configfs provides.  I
hope it's the latter :-)

Joel

-- 

"I don't want to achieve immortality through my work; I want to
 achieve immortality through not dying."
        - Woody Allen

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: configfs: return value for drop_item()/make_item()?
  2007-01-16  8:30 ` Joel Becker
@ 2007-01-16 16:42   ` Michael Noisternig
  0 siblings, 0 replies; 8+ messages in thread
From: Michael Noisternig @ 2007-01-16 16:42 UTC (permalink / raw)
  To: Joel Becker; +Cc: linux-kernel

> 	I'm sorry if I missed your previous post.  I've never ignored a
> configfs post on purpose :-)

Well, thanks a lot for the reply! :)

>> Here's the issue... the configfs system can prevent a user from 
>> _creating_ sub-directories in a certain directory (by not supplying 
>> struct configfs_group_operations->make_item()/make_group()) but it 
>> cannot prevent him from _removing_ sub-directories because struct 
>> configfs_group_operations->drop_item() is void.
> 
> 	This is intentional.  The entire point of configfs is to have
> the lifetime controlled from userspace.  If the kernel can pin things
> however it likes, we lose that property.

I fully agree with the idea of configfs not being allowed to destroy 
user-created objects. OTOH, while configfs is described as a filesystem 
for user-created objects under user control, compared to sysfs as a 
filesystem for kernel-created objects under kernel control, configfs 
_does_ permit kernel-created objects in a limited way (by filling in 
struct config_group->default_groups), and these objects can only be 
destroyed again by the kernel, and not by the user.

As such I don't understand fully why one doesn't want to merge sysfs and 
configfs (let's call it sysconfs for now) in such a way that it allows 
_both_ user- and kernel-created objects, with user-objects only allowed 
to be destroyed by the user and kernel-objects only by the kernel.

I'll explain more shortly, and also the connection with...

>> Similarly, struct configfs_group_operations->make_item() does not permit 
>> to return an error code, and as such it is impossible to 'check' the 
>> type of sub-directory a user wants to create (with returning a 
>> meaningful error code). This had already, unsuccessfully, been brought 
>> up before on 2006-08-29 by J. Berg (see 
>> http://marc.theaimsgroup.com/?l=linux-kernel&m=115692319227688&w=2).

One problem with configfs is that there is too little control over how 
many other objects a particular object may contain. IOW, an object may 
either contain
   - n sub-objects (n entries in ->default_groups), or
   - n..inf sub-objects (n entries in ->default_groups, 
->make_item()/make_group() supplied)

Often however, what you want is that an object may contain 0 or 1 other 
objects. If ->make_item()/make_group() would allow returning a 
meaningful error code the kernel could deny creation of a 2nd object 
other than by pretending to be out of memory.

For another example, and directly related to above link, suppose having 
an object with a number of attributes, one of them being called 'type'. 
Dependent on the value of 'type' this object may contain a particular 
type of sub-object (with type-dependent attributes). E.g. 'type' may be 
empty | 'a' | 'b' | 'ab', then dependent on this there should be 0 or 1 
directories called 'a', and 0 or 1 directories called 'b'. Doing it this 
way means that while the user decides what sub-directory/-ies the object 
has, he does not create them (directly) - it is the kernel which creates 
the object, and as such it is also the kernel only which is permitted to 
destroy the object again - by the user writing a different value to the 
'type' attribute (or clearing it). sysconfs could solve this.

Above problem could be solved in a different manner within the realms of 
configfs by not having a 'type' attribute, but the other way around 
defining the (implicit) type of the object by the user creating certain 
sub-directories ('a' and 'b'). I.e. if the user creates directory 'a' 
the object is of type a, if the user also creates 'b' then the object is 
of type ab. However, the user should not be permitted to create other 
objects than 'a' and 'b' - thus ->make_item()/make_group() should permit 
returning a meaningful error code.

> 	I don't quite see how this link relates, but perhaps I'm not
> understanding your entire question.  Currently, ->make_item() doesn't
> allow an error code because that would involve the ERR_PTR() construct.
> While that construct is quite useful, I'm always wary of adding
> complexity.  Thus, the tradeoff would have to be worth it.

I was thinking about
   ssize_t make_item(struct config_group *group, const char *name, 
struct config_item **new_item)
with return value 0 meaning no-error.

>> Please give some arguments why you think 
>> configfs_group_operations->drop_item() should remain void.
> 
> 	Very simply, the lifetime must be in the control of userspace.
> That's precisely the point of configfs.  If you'd like the kernel to
> control the lifetime, I suspect sysfs will be more to your liking.
> configfs is not meant to replace sysfs, merely to coexist for things
> that benefit from the configfs model and not the sysfs model.
> 	(More on the model and your problem near the bottom)

Well, I understand and agree to the philosophy... but I still don't know 
how to solve my problem elegantly... more below...

> 
>> (1)
>> Say the user creates one object, let's say as objects/myobj1/. This 
>> object is dependent on some (shared) parameters which the user created 
>> under params/myparams1/. Now while myobj1/ is 'active', I don't want to 
>> let the user remove myparams1/. I can prevent this by making the user 
>> create a symlink(2) in the objects/myobj1/ directory to myparams1/, i.e. 
>> objects/myobj1/params/ -> ../../params/myparams1/, to denote its use. 
>> Now - if I have read the documentation correctly - the user cannot 
>> remove myparams1/ without removing the params/ link first. So fine, so good.
> 
> 	This will certainly work.
> 
>> (2)
>> Next the user may create several objects which may be dependent on 
>> several params objects. Now I can solve this by creating a default group 
>> for each object, i.e. on myobj1 creation there is objects/myobj1/params/ 
>> automatically. In that directory the user may create symlink(2)s to 
>> several params/*/ dirs. Fine again.
>>
>> (3)
>> Now what I want is the list of params an object uses to be an ordered 
>> list. I cannot do this because there is no intrinsic order in a 
>> filesystem. I can get the order by instead having an attribute called 
>> param_list which contains the ordered list of all params to use, e.g.
> 
> 	Why does it need to be ordered?  Also, why can't the symlinks
> have a naming convention of order?  Sure, someone hacking at it can
> break that, but your tool that really does the work can create
> objects/myobj1/params/param1 -> myparams2.  So the listing does have
> meaning.  Just one possibility.

It must be ordered because it's a priority list.

I was thinking about having symlinks in a directory and deriving the 
order by the symlinks' filenames, too. I dismissed it originally for two 
reasons. First, I didn't see how to keep the order when some link gets 
deleted, e.g. there's 1,2,3 and then link 2 gets deleted. Now, thinking 
about it again, I can simply keep a ordered linked list internally, and 
therefrom remove the node for link 2. But it's still not perfect, 
because how do I insert a link between filenames 1 and 2? Ok, I have to 
delete all symlinks first and then rebuild them, and in the end it's 
like rewriting a params_list attribute file... except that it's not 
atomic. Second, a simple params_list file seems a lot more easy to 
handle from the user perspective... simply open the file with an editor, 
add or delete an entry, save it, close it. I don't know which method is 
better, the one described here with a params/ subdirectory containing 
symlinks with their order derived from their filenames, or method 2) 
from below with simply having a params_list file where params entries 
get renamed to <params removed> but are still valid until the user 
rewrites the params_list file.

>> 2)
>> Keep a reference on each configfs object (e.g. params) in use, so when 
>> the user removes it from configfs it is still present in memory. This, 
>> however, requires to rename each according entry in every affected 
>> param_list to <params removed> or similar... not a very user-friendly 
>> way to deal with the situation.
> 
> 	There are two ways to reference the params.  One is to let the
> userspace program ask for the reference via symlink.  The other is in
> your driver.  This has the property you describe above, that you have
> to keep track of it after the user has asked for it to go away.

Yes, these are exactly the two possible methods I see and describe above.

> 	This property is on purpose.  When a user asks for something to
> go away, it needs to disappear from configfs before the rmdir(2)
> returns.  It's gone, and it is no longer accessible from userspace.  The
> lifetiming of userspace access is clear, cut, and dried.
> 	However, some objects need some time to die.  So the driver has
> its own reference, and it schedules any cleanup and death later.  But
> that death and cleanup should happen as soon as possible, because
> userspace has asked for it.

Yes, I agree with the methodology, it's exactly how a filesystem is 
expected to handle such situtation. OTOH, the behavior for configfs 
files referenced by symlinks is already different.

> 	Let's break for a minute.  Let me try to restate all of this.
> You have an interrelationship between a few configfs objects.  When a
> [referrer -> reference] relationship is put together, you need to prevent
> the reference from going away.  You've proposed a couple of methods that
> boil down to "when the reference object knows it is being referred to,
> it prevents itself from going away".
> 	Therein lies the rub.  configfs works by putting all of that in
> userspace.  Lifetime decisions can only occur in userspace.  This allows
> us a very clean lifetime from the userspace perspective.  It also means
> that if we want to lock an object down, it has to happen in userspace.
> 	So, the object cannot "prevent itself from going away".  The
> kernel side just can't do this.  Userspace, however, can.  You've
> discussed the most common way configfs allows userspace to do this, the
> symlink.  I believe your only problem was that of naming.  There's no
> reason you can't populate a param_list file at the same time you respond
> to ->allow_link().  Or, you could have userspace name the links in a
> manner providing ordering.  These solve this problem without breaking
> the configfs contract.

See above. I still don't know what would be an optimal solution.

> 	Think about your requirements again.  Do you absolutely require
> that some part of your kernel driver make a lifetime decision?  If so,
> configfs isn't going to fit.  Can the lifetime decision be moved to
> userspace?  If so, configfs should work just fine as-is, and you'll gain
> the clean lifetime and simple manipulation that configfs provides.  I
> hope it's the latter :-)

configfs should be a perfect fit for my purposes, as it's the user (and 
the user only) who creates and destroys the objects. The problem lies 
elsewhere. :/

OTOH, I don't see why configfs and sysfs cannot (should not) be merged 
as described above for sysconfs.

I'll give an example once more:
I have a directory called 'statistics' in my module's main dir in the 
configfs tree. This directory will contain several attributes for 
statistical purposes. Normally you would place this object into sysfs 
because it's an object created by the kernel (->default_groups). 
However, I certainly don't want to rip apart the statistics dir from the 
rest of the module parameters. What this shows in my eyes is that (1) 
configfs _does_ create kernel objects, and (2) sysconfs might make sense.

> Joel

Thanks again!
   Michael

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: configfs: return value for drop_item()/make_item()?
  2007-01-23 13:49       ` Michael Noisternig
@ 2007-01-23 19:06         ` Joel Becker
  0 siblings, 0 replies; 8+ messages in thread
From: Joel Becker @ 2007-01-23 19:06 UTC (permalink / raw)
  To: Michael Noisternig; +Cc: linux-kernel

On Tue, Jan 23, 2007 at 02:49:59PM +0100, Michael Noisternig wrote:
> Sorry, I wasn't clear. I meant that it's not possible to let the user 
> create the parent directory via mkdir(2) within sysfs. I.e.
> # mkdir object  <-- create object/, configfs only
> # ls object
> type
> # echo b > object/type  <-- create b/, sysfs only
> # ls object
> type
> b/
> I cannot have both, sysfs and configfs functionality, within the same 
> module configuration file system tree.

	No, you can't have sysfs and configfs functionality in the same
exact portion of the tree.  But you can use both sysfs and configfs at
the same time in your driver.  The different parts will appear in the
different trees.  Up to you.

> 1. Is there any guarantee that strings passed to struct 
> configfs_item_operations->store_attribute are 0-terminated? If not, 
> there's not only potential for buffer overflows, but there already is - 
> in configfs_example.c, using simple_strtoul().

	There is no guarantee, but that's a bug.  The attribute handling
code came from sysfs.  I assume the initial assumption was that since
you can only copy a page, it's OK.  Clearly not the case.  sysfs fixed
this in October (commit 035ed7a49447bc8e15d4d9316fc6a359b2d94333).  I'll
be porting that over.  Thanks for noticing!

> 2. A minor one: I realize that the CONFIGFS_ITEM_NAME_LEN value of 20 is 
> taken over from sysfs, but in configfs a config_group now has 68 bytes 
> of size, and assuming that many times one would simply kmalloc a simple 
> struct config_group (without extending it) this would result in a waste 
> of 60 bytes per malloc. Do you want to keep that #define or reduce it to 
> 16? Just a thought...

	Hmm, interesting.  I'll look at that as well.

Joel

-- 

Life's Little Instruction Book #511

	"Call your mother."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: configfs: return value for drop_item()/make_item()?
  2007-01-23  1:13     ` Joel Becker
@ 2007-01-23 13:49       ` Michael Noisternig
  2007-01-23 19:06         ` Joel Becker
  0 siblings, 1 reply; 8+ messages in thread
From: Michael Noisternig @ 2007-01-23 13:49 UTC (permalink / raw)
  To: Joel Becker, linux-kernel

>> If you argue that they are in fact created by the user because they are 
>> a direct result of a user action, then I can apply the same argument to 
>> this one example:
>> ...
>>> This is precisely what configfs is designed to forbid. The kernel
>>> does not, ever, create configfs objects on its own. It does it as a
>>> result of userspace action.
>> No. The sub-directory only appears as a direct result of the user 
>> writing a value into the 'type' attribute. ;-)
> 
> 	Ok, you're stretching the metaphor.  Writing a value to a "type"
> attribute is, indeed, a userspace action.  However, configfs' contract
> is that only mkdir(2) creates objects.
> 	We're not trying to create the do-everything-kitchen-sink system
> here.  That way lies the problems we're trying to avoid.  That's why
> configfs has a specific contract it provides to (a) userspace and (b)
> client drivers.

Ok, so it's a design decision about configfs. I'll accept that. :)

>>> you're never going to get it from configfs. You should be using
>>> sysfs.
>> Hardly. sysfs doesn't allow the user creating directories. :>
> 
> 	sysfs certainly supports your "echo b > type" style of object
> creation.  You're type_file->store() method gets a "b" in the buffer and
> then does sysfs_mkdir() of your new object directory.  Here, the kernel
> is creating the new object (the directory).

Sorry, I wasn't clear. I meant that it's not possible to let the user 
create the parent directory via mkdir(2) within sysfs. I.e.
# mkdir object  <-- create object/, configfs only
# ls object
type
# echo b > object/type  <-- create b/, sysfs only
# ls object
type
b/
I cannot have both, sysfs and configfs functionality, within the same 
module configuration file system tree.

>> Hm, I had envisioned the user to fully configure the module via file 
>> system operations only. Now if the user is supposed to use a wrapper 
>> program this sheds a different light on all those 
>> what's-the-best-solution issues...
> 
> 	Certainly the user can do the configuration by hand.  It will
> always work.  But why make them understand your userspace<->kernel API
> when you can just provide a tool?  They're all going to script it up
> anyway.

Because the layout in say a user tool configuration file will look 
pretty much the same as the configfs tree.

Anyway, for something different now:

1. Is there any guarantee that strings passed to struct 
configfs_item_operations->store_attribute are 0-terminated? If not, 
there's not only potential for buffer overflows, but there already is - 
in configfs_example.c, using simple_strtoul().

2. A minor one: I realize that the CONFIGFS_ITEM_NAME_LEN value of 20 is 
taken over from sysfs, but in configfs a config_group now has 68 bytes 
of size, and assuming that many times one would simply kmalloc a simple 
struct config_group (without extending it) this would result in a waste 
of 60 bytes per malloc. Do you want to keep that #define or reduce it to 
16? Just a thought...


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: configfs: return value for drop_item()/make_item()?
  2007-01-22 12:35   ` Michael Noisternig
@ 2007-01-23  1:13     ` Joel Becker
  2007-01-23 13:49       ` Michael Noisternig
  0 siblings, 1 reply; 8+ messages in thread
From: Joel Becker @ 2007-01-23  1:13 UTC (permalink / raw)
  To: Michael Noisternig; +Cc: linux-kernel

On Mon, Jan 22, 2007 at 01:35:36PM +0100, Michael Noisternig wrote:
> Sure, but what I meant to say was that the user, when creating a 
> directory, did not request creation of such sub-directories, so I see 
> them as created by the kernel.

	Ahh, but userspace did!  It's part of the configfs contract.
They've asked for an new config item and all that it entails.

> If you argue that they are in fact created by the user because they are 
> a direct result of a user action, then I can apply the same argument to 
> this one example:
> ...
> >This is precisely what configfs is designed to forbid. The kernel
> >does not, ever, create configfs objects on its own. It does it as a
> >result of userspace action.
> 
> No. The sub-directory only appears as a direct result of the user 
> writing a value into the 'type' attribute. ;-)

	Ok, you're stretching the metaphor.  Writing a value to a "type"
attribute is, indeed, a userspace action.  However, configfs' contract
is that only mkdir(2) creates objects.
	We're not trying to create the do-everything-kitchen-sink system
here.  That way lies the problems we're trying to avoid.  That's why
configfs has a specific contract it provides to (a) userspace and (b)
client drivers.

> >you're never going to get it from configfs. You should be using
> >sysfs.
> 
> Hardly. sysfs doesn't allow the user creating directories. :>

	sysfs certainly supports your "echo b > type" style of object
creation.  You're type_file->store() method gets a "b" in the buffer and
then does sysfs_mkdir() of your new object directory.  Here, the kernel
is creating the new object (the directory).

> Well, you don't need PTR_ERR().

	Sure, you could use **new_item.  It's the same complexity
change.
 
> That's an interesting other solution, however it seems a bit redundant 
> (params are referenced by links as well as in the 'order' attribute 
> file) and not as simple as my method 2). I guess, for now, in lack of a 
> convincing solution, I will implement method 2) as the one easiest to 
> adapt to given my current code base.

	But they are not referenced by the order file.  It's just an
attribute :-)  Really, you can look at it either way.  But configfs has
a specific perspective based on its contracts, and so it works within
them.

> Hm, I had envisioned the user to fully configure the module via file 
> system operations only. Now if the user is supposed to use a wrapper 
> program this sheds a different light on all those 
> what's-the-best-solution issues...

	Certainly the user can do the configuration by hand.  It will
always work.  But why make them understand your userspace<->kernel API
when you can just provide a tool?  They're all going to script it up
anyway.

Joel

-- 

"The doctrine of human equality reposes on this: that there is no
 man really clever who has not found that he is stupid."
	- Gilbert K. Chesterson

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: configfs: return value for drop_item()/make_item()?
  2007-01-18 22:12 ` Joel Becker
@ 2007-01-22 12:35   ` Michael Noisternig
  2007-01-23  1:13     ` Joel Becker
  0 siblings, 1 reply; 8+ messages in thread
From: Michael Noisternig @ 2007-01-22 12:35 UTC (permalink / raw)
  To: Joel Becker, linux-kernel

Thanks for your reply again! See comments inline...

Joel Becker wrote:
>> I fully agree with the idea of configfs not being allowed to destroy
>> user-created objects. OTOH, while configfs is described as a filesystem
>> for user-created objects under user control, compared to sysfs as a
>> filesystem for kernel-created objects under kernel control, configfs
>> _does_ permit kernel-created objects in a limited way (by filling in
>> struct config_group->default_groups), and these objects can only be
>> destroyed again by the kernel, and not by the user.
> 
> 	They are not created by a kernel action.  They are created as a
> direct result of a user action.  The user must mkdir(2) the parent in
> the chain.  Only then do these default_groups appear.  Contrast sysfs,
> where filesystem structures can be added at any time, from any codepath,
> via the sysfs in-kernel API.

Sure, but what I meant to say was that the user, when creating a 
directory, did not request creation of such sub-directories, so I see 
them as created by the kernel.

If you argue that they are in fact created by the user because they are 
a direct result of a user action, then I can apply the same argument to 
this one example:

>> For another example, and directly related to above link, suppose
>> having an object with a number of attributes, one of them being
>> called 'type'. Dependent on the value of 'type' this object may
>> contain a particular type of sub-object (with type-dependent
>> attributes). E.g. 'type' may be empty | 'a' | 'b' | 'ab', then
>> dependent on this there should be 0 or 1 directories called 'a',
>> and 0 or 1 directories called 'b'. Doing it this way means that
>> while the user decides what sub-directory/-ies the object has, he
>> does not create them (directly) - it is the kernel which creates 
>> the object, and as such it is also the kernel only which is
>> permitted to destroy the object again - by the user writing a
>> different value to the 'type' attribute (or clearing it). sysconfs
>> could solve this.
> 
> This is precisely what configfs is designed to forbid. The kernel
> does not, ever, create configfs objects on its own. It does it as a
> result of userspace action.

No. The sub-directory only appears as a direct result of the user 
writing a value into the 'type' attribute. ;-)

> If you want the following:
> 
> # cd mydir
> # ls -l
> -rw-r--r-- 1 root root 0 2006-12-28 07:11 type
> # echo 'ab' > type
> # ls -l mydir
> drwxr-xr-x 2 root root 4096 2007-01-08 14:21 ab
> -rw-r--r-- 1 root root 2 2007-01-08 14:21 type
> # echo '' > type
> # ls -l mydir
> -rw-r--r-- 1 root root 0 2007-01-08 14:22 type
> 
> you're never going to get it from configfs. You should be using
> sysfs.

Hardly. sysfs doesn't allow the user creating directories. :>

>> As such I don't understand fully why one doesn't want to merge sysfs and
>> configfs (let's call it sysconfs for now) in such a way that it allows
>> _both_ user- and kernel-created objects, with user-objects only allowed
>> to be destroyed by the user and kernel-objects only by the kernel.
> 
> 	The programming interface is very, very different.  Check out
> historical messages on this topic:
> 
> http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg95708.html
> http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg95711.html
> http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg95714.html
> http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg95717.html

Well, you could still use type (user object/kernel object) dependent 
structure pointers?

>> Often however, what you want is that an object may contain 0 or 1 other
>> objects. If ->make_item()/make_group() would allow returning a
>> meaningful error code the kernel could deny creation of a 2nd object
>> other than by pretending to be out of memory.
> 
> 	You make a reasonable case that ENOMEM isn't always the error
> you want, but perhaps we can add a better umbrella error code?  I'm wary
> of introducing PTR_ERR() or any other complexity if we don't _need_ it.
> I'm all for thoughts on possibly compromises.

>> I was thinking about
>> ssize_t make_item(struct config_group *group, const char *name, struct
>> config_item **new_item)
>> with return value 0 meaning no-error.
> 
> 	Sure, it's another way to go, but it's effectively the same
> thing.

Well, you don't need PTR_ERR().

>> I was thinking about having symlinks in a directory and deriving the
>> order by the symlinks' filenames, too. I dismissed it originally for two
>> reasons. First, I didn't see how to keep the order when some link gets
>> deleted, e.g. there's 1,2,3 and then link 2 gets deleted. Now, thinking
>> about it again, I can simply keep a ordered linked list internally, and
>> therefrom remove the node for link 2. But it's still not perfect,
>> because how do I insert a link between filenames 1 and 2? Ok, I have to
>> delete all symlinks first and then rebuild them, and in the end it's
>> like rewriting a params_list attribute file... except that it's not
>> atomic. Second, a simple params_list file seems a lot more easy to
>> handle from the user perspective... simply open the file with an editor,
>> add or delete an entry, save it, close it. I don't know which method is
>> better, the one described here with a params/ subdirectory containing
>> symlinks with their order derived from their filenames, or method 2)
>> from below with simply having a params_list file where params entries
>> get renamed to <params removed> but are still valid until the user
>> rewrites the params_list file.
> 
> 	Why not both?  You need the reference, and the reference must
> come from a userspace action.  While a set of ordered names would be
> easy for some folks, maybe it isn't for you.  So, the userspace process
> first symlinks all dependant parameters, thus taking a reference.  Then,
> they write to params/order to order them.  If they remove a symlink, you
> just delete the dentry from params/order.
> 
>     # cd object/params
>     # ls -l
>     -rw-r--r-- 1 root root    0 2007-01-08 14:22 order
>     # ln -s ../../params/param1 .
>     # ln -s ../../params/param2 .
>     # ls -l
>     -rw-r--r-- 1 root root    0 2007-01-08 14:22 order
>     lrwxrwxrwx 1 root root   19 2007-01-08 15:30 param1 -> ../../params/param1
>     lrwxrwxrwx 1 root root   19 2007-01-08 15:30 param2 -> ../../params/param2
>     # cat order
>     # cat >order
>     param2
>     param1
>     ^D
>     # cat order
>     param2
>     param1
>     # rm param2
>     # ls -l
>     -rw-r--r-- 1 root root    0 2007-01-08 14:31 order
>     lrwxrwxrwx 1 root root   19 2007-01-08 15:30 param1 -> ../../params/param1
>     # cat order
>     param1
> 
> 	Note that you don't need a "<param2 deleted>" placeholder in
> 'order'.  Userspace explicitly removed the symlink, they should know
> what they did.

That's an interesting other solution, however it seems a bit redundant 
(params are referenced by links as well as in the 'order' attribute 
file) and not as simple as my method 2). I guess, for now, in lack of a 
convincing solution, I will implement method 2) as the one easiest to 
adapt to given my current code base.

> 	Well, I hope I've come up with a few ways of doing this that
> perhaps you'll either like or find inspiring.  One thing to remember is
> that, while I'm using shell commands to show the configfs operations, in
> reality there'd be a wrapper program for your user.  You want the actual
> user to be given a tool that speaks what they *want* to do, not how it
> actually gets done.

Hm, I had envisioned the user to fully configure the module via file 
system operations only. Now if the user is supposed to use a wrapper 
program this sheds a different light on all those 
what's-the-best-solution issues...

> Joel

Thanks, Michael


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: configfs: return value for drop_item()/make_item()?
       [not found] <45AF6D0C.8020600@cosy.sbg.ac.at>
@ 2007-01-18 22:12 ` Joel Becker
  2007-01-22 12:35   ` Michael Noisternig
  0 siblings, 1 reply; 8+ messages in thread
From: Joel Becker @ 2007-01-18 22:12 UTC (permalink / raw)
  To: Michael Noisternig; +Cc: linux-kernel

On Thu, Jan 18, 2007 at 01:50:20PM +0100, Michael Noisternig wrote:
> did you get my last reply? I hope you still consider it to be worthwhile
> to comment on. :)

	I didn't get it, I'm sorry.  I wonder what happened.  Did you
send it to me, linux-kernel, or both (both is preferred).

> And sorry for your double-reception of my last mail, I was unwittingly
> ignoring your follow-up-to setting.

	I filter dupes by Message-ID :-)

> -------- Original Message --------
> Subject: Re: configfs: return value for drop_item()/make_item()?
> From: Michael Noisternig
> Date: Tue Jan 16 2007 - 11:40:50 EST
> 
> I fully agree with the idea of configfs not being allowed to destroy
> user-created objects. OTOH, while configfs is described as a filesystem
> for user-created objects under user control, compared to sysfs as a
> filesystem for kernel-created objects under kernel control, configfs
> _does_ permit kernel-created objects in a limited way (by filling in
> struct config_group->default_groups), and these objects can only be
> destroyed again by the kernel, and not by the user.

	They are not created by a kernel action.  They are created as a
direct result of a user action.  The user must mkdir(2) the parent in
the chain.  Only then do these default_groups appear.  Contrast sysfs,
where filesystem structures can be added at any time, from any codepath,
via the sysfs in-kernel API.

> As such I don't understand fully why one doesn't want to merge sysfs and
> configfs (let's call it sysconfs for now) in such a way that it allows
> _both_ user- and kernel-created objects, with user-objects only allowed
> to be destroyed by the user and kernel-objects only by the kernel.

	The programming interface is very, very different.  Check out
historical messages on this topic:

http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg95708.html
http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg95711.html
http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg95714.html
http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg95717.html

	In addition, userspace would have no idea at any given time
whether they were dealing with kernel managed objects or user managed
ones.  So you'd confuse both the driver programmer *and* the userspace
programmer.

> One problem with configfs is that there is too little control over how
> many other objects a particular object may contain. IOW, an object may
> either contain
> - n sub-objects (n entries in ->default_groups), or
> - n..inf sub-objects (n entries in ->default_groups,
> ->make_item()/make_group() supplied)

	Clearly you can have 0..N sub-objects by returning NULL from
successive ->make_item() calls, but as you point out, that results in
ENOMEM, not the cleanest error for this case.

> Often however, what you want is that an object may contain 0 or 1 other
> objects. If ->make_item()/make_group() would allow returning a
> meaningful error code the kernel could deny creation of a 2nd object
> other than by pretending to be out of memory.

	You make a reasonable case that ENOMEM isn't always the error
you want, but perhaps we can add a better umbrella error code?  I'm wary
of introducing PTR_ERR() or any other complexity if we don't _need_ it.
I'm all for thoughts on possibly compromises.

> For another example, and directly related to above link, suppose having
> an object with a number of attributes, one of them being called 'type'.
> Dependent on the value of 'type' this object may contain a particular
> type of sub-object (with type-dependent attributes). E.g. 'type' may be
> empty | 'a' | 'b' | 'ab', then dependent on this there should be 0 or 1
> directories called 'a', and 0 or 1 directories called 'b'. Doing it this
> way means that while the user decides what sub-directory/-ies the object
> has, he does not create them (directly) - it is the kernel which creates
> the object, and as such it is also the kernel only which is permitted to
> destroy the object again - by the user writing a different value to the
> 'type' attribute (or clearing it). sysconfs could solve this.

	This is precisely what configfs is designed to forbid.  The
kernel does not, ever, create configfs objects on its own.  It does it
as a result of userspace action.  If you want the following:

    # cd mydir
    # ls -l
    -rw-r--r-- 1 root root    0 2006-12-28 07:11 type
    # echo 'ab' > type
    # ls -l mydir
    drwxr-xr-x 2 root root 4096 2007-01-08 14:21 ab
    -rw-r--r-- 1 root root    2 2007-01-08 14:21 type
    # echo '' > type
    # ls -l mydir
    -rw-r--r-- 1 root root    0 2007-01-08 14:22 type

you're never going to get it from configfs.  You should be using sysfs.

> Above problem could be solved in a different manner within the realms of
> configfs by not having a 'type' attribute, but the other way around
> defining the (implicit) type of the object by the user creating certain
> sub-directories ('a' and 'b'). I.e. if the user creates directory 'a'
> the object is of type a, if the user also creates 'b' then the object is
> of type ab. However, the user should not be permitted to create other
> objects than 'a' and 'b' - thus ->make_item()/make_group() should permit
> returning a meaningful error code.

	Or just returning an error, with the userspace program that
actually does the work interpretting it correctly.  Remember, while it
is convenient to be able to poke at configfs via the shell, real users
are going to be using a wrapper program (eg, OCFS2 users don't manually
add node information via mkdir(1) and echo(1), they use o2cb_ctl, which
does it for them via mkdir(2) and write(2)).
	Really, if you wanted to do this via configfs, you'd do
something like this:

    # cd /sys/kernel/config
    # ls -l
    # modprobe mymodule
    # ls -l
    drwxr-xr-x 2 root root 4096 2007-01-08 14:21 mymodule
    # cd mymodule
    # ls -l
    drwxr-xr-x 2 root root 4096 2007-01-08 14:21 a
    drwxr-xr-x 2 root root 4096 2007-01-08 14:21 b
    drwxr-xr-x 2 root root 4096 2007-01-08 14:21 ab
    # mkdir a/object
    # ls -l a
    drwxr-xr-x 2 root root 4096 2007-01-08 14:22 object
    # mkdir b/object
    mkdir: cannot create directory `b/object`: Out of memory

	So your types are explicit in the fact that you have
default_groups for type 'a', 'b', and 'ab'.  You create an object of
type 'a' via 'mkdir /sys/kernel/config/mymodule/a/object', and then you
can't create objects under 'b' or 'ab'.  When you 'rmdir a/object', now
you can make 'b/object'.
	You will notice, of course, the "Out of memory" error, because
it's only ENOMEM right now.  Here, we could come up with a better
umbrella error code, or the real wrapper program could say "I know this
means an object already exists".

> I was thinking about
> ssize_t make_item(struct config_group *group, const char *name, struct
> config_item **new_item)
> with return value 0 meaning no-error.

	Sure, it's another way to go, but it's effectively the same
thing.

> Well, I understand and agree to the philosophy... but I still don't know
> how to solve my problem elegantly... more below...

	Different problems (I'm sure you know that).  That's why I'm
taking the time to respond, though, I'd like to help you solve the
problem.

> It must be ordered because it's a priority list.

	Just curious, I can certainly see a use.

> I was thinking about having symlinks in a directory and deriving the
> order by the symlinks' filenames, too. I dismissed it originally for two
> reasons. First, I didn't see how to keep the order when some link gets
> deleted, e.g. there's 1,2,3 and then link 2 gets deleted. Now, thinking
> about it again, I can simply keep a ordered linked list internally, and
> therefrom remove the node for link 2. But it's still not perfect,
> because how do I insert a link between filenames 1 and 2? Ok, I have to
> delete all symlinks first and then rebuild them, and in the end it's
> like rewriting a params_list attribute file... except that it's not
> atomic. Second, a simple params_list file seems a lot more easy to
> handle from the user perspective... simply open the file with an editor,
> add or delete an entry, save it, close it. I don't know which method is
> better, the one described here with a params/ subdirectory containing
> symlinks with their order derived from their filenames, or method 2)
> from below with simply having a params_list file where params entries
> get renamed to <params removed> but are still valid until the user
> rewrites the params_list file.

	Why not both?  You need the reference, and the reference must
come from a userspace action.  While a set of ordered names would be
easy for some folks, maybe it isn't for you.  So, the userspace process
first symlinks all dependant parameters, thus taking a reference.  Then,
they write to params/order to order them.  If they remove a symlink, you
just delete the dentry from params/order.

    # cd object/params
    # ls -l
    -rw-r--r-- 1 root root    0 2007-01-08 14:22 order
    # ln -s ../../params/param1 .
    # ln -s ../../params/param2 .
    # ls -l
    -rw-r--r-- 1 root root    0 2007-01-08 14:22 order
    lrwxrwxrwx 1 root root   19 2007-01-08 15:30 param1 -> ../../params/param1
    lrwxrwxrwx 1 root root   19 2007-01-08 15:30 param2 -> ../../params/param2
    # cat order
    # cat >order
    param2
    param1
    ^D
    # cat order
    param2
    param1
    # rm param2
    # ls -l
    -rw-r--r-- 1 root root    0 2007-01-08 14:31 order
    lrwxrwxrwx 1 root root   19 2007-01-08 15:30 param1 -> ../../params/param1
    # cat order
    param1

	Note that you don't need a "<param2 deleted>" placeholder in
'order'.  Userspace explicitly removed the symlink, they should know
what they did.

> configfs should be a perfect fit for my purposes, as it's the user (and
> the user only) who creates and destroys the objects. The problem lies
> elsewhere. :/

	Well, I hope I've come up with a few ways of doing this that
perhaps you'll either like or find inspiring.  One thing to remember is
that, while I'm using shell commands to show the configfs operations, in
reality there'd be a wrapper program for your user.  You want the actual
user to be given a tool that speaks what they *want* to do, not how it
actually gets done.

> I'll give an example once more:
> I have a directory called 'statistics' in my module's main dir in the
> configfs tree. This directory will contain several attributes for
> statistical purposes. Normally you would place this object into sysfs
> because it's an object created by the kernel (->default_groups).
> However, I certainly don't want to rip apart the statistics dir from the
> rest of the module parameters. What this shows in my eyes is that (1)
> configfs _does_ create kernel objects, and (2) sysconfs might make sense.

	As I stated above, configfs does not allow objects to be created
in response to kernel actions.  Sure, configfs does the actual work of
filling out default_groups, and default_attrs, and even the object you
just called mkdir(2) on.  But it's all in response to a userspace
action.  That's what I mean by "user created".
	Something to remember is taht there is nothing wrong with
putting some pieces in configfs and some pieces in sysfs.  Really.  You
should be mostly hiding this from the user anyway.  configfs and sysfs
are userspace<->kernelspace interfaces.  They provide some nice
properties for the developer of kernel drivers needing a userspace
interface and the developer of a userspace program communicating with
kernelspace.  But if you want a real user, "Joe Hacker" as it work, to
see these things, you don't want them caring whether it's in configfs,
sysfs, procfs, ioctl(2), or mmap(2).  You want them using a tool.
	You and I are hackers.  We know that the kernel exposes our
laptop's batter info in /proc/acpi/battery/BAT0/state.  So we can do
'cat /proc/acpi/battery/BAT0/state' if we like.  But we're the
exception.  Almost everyone is going to do "acpitool -b".
	If you have your configuration in configfs and your statistics
in sysfs, it provides you a nice set of programming interfaces.  The
fact that your tool checks two places is irrelevant to your users.  They
just know to call 'michaelstool --stats' to get statistics, and
'michaelstool --config' to configure the driver.

Joel

-- 

 print STDOUT q
 Just another Perl hacker,
 unless $spring
	- Larry Wall

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2007-01-23 19:06 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-01-15 10:55 configfs: return value for drop_item()/make_item()? Michael Noisternig
2007-01-16  8:30 ` Joel Becker
2007-01-16 16:42   ` Michael Noisternig
     [not found] <45AF6D0C.8020600@cosy.sbg.ac.at>
2007-01-18 22:12 ` Joel Becker
2007-01-22 12:35   ` Michael Noisternig
2007-01-23  1:13     ` Joel Becker
2007-01-23 13:49       ` Michael Noisternig
2007-01-23 19:06         ` Joel Becker

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).