[PATCH] configfs: change depends -> select SYSFS

[PATCH] configfs: change depends -> select SYSFS

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Hi Linus,

The following patch should be included for 38-rc1 to fix a rdd
reported linux-next kbuild warning introduced by TARGET_CORE
using 'select CONFIGFS_FS' from over the holidays, that was not
included with the initial target merge yesterday.

Thanks!

----------------------------------------------------------------

This patch changes configfs to select SYSFS to fix the following:

warning: (TARGET_CORE && GFS2_FS) selects CONFIGFS_FS which has unmet direct dependencies (SYSFS)

This patch also fixes whitespace breakage in the 'help' section.

Reported-by: Randy Dunlap <randy.dunlap@oracle.com>
Acked-by: Joel Becker <jlbec@evilplan.org>
Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
---
 fs/configfs/Kconfig |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/configfs/Kconfig b/fs/configfs/Kconfig
index 13587cc..0e4bd23 100644
--- a/fs/configfs/Kconfig
+++ b/fs/configfs/Kconfig
@@ -1,11 +1,11 @@
 config CONFIGFS_FS
 	tristate "Userspace-driven configuration filesystem"
-	depends on SYSFS
+	select SYSFS
 	help
-	  configfs is a ram-based filesystem that provides the converse
-	  of sysfs's functionality. Where sysfs is a filesystem-based
-	  view of kernel objects, configfs is a filesystem-based manager
-	  of kernel objects, or config_items.
+	configfs is a RAM-based filesystem that provides the converse
+	of sysfs's functionality. Where sysfs is a filesystem-based
+	view of kernel objects, configfs is a filesystem-based manager
+	of kernel objects, or config_items.
 
-	  Both sysfs and configfs can and should exist together on the
-	  same system. One is not a replacement for the other.
+	Both sysfs and configfs can and should exist together on the
+	same system. One is not a replacement for the other.
-- 
1.7.3.5

Re: [PATCH] configfs: change depends -> select SYSFS

[Stefan Richter]

On Jan 15 Nicholas A. Bellinger wrote:
> This patch changes configfs to select SYSFS to fix the following:
> 
> warning: (TARGET_CORE && GFS2_FS) selects CONFIGFS_FS which has unmet direct dependencies (SYSFS)

Why don't you fix target-core's Kconfig instead?

> This patch also fixes whitespace breakage in the 'help' section.

There is no whitespace breakage in fs/configfs/Kconfig.
-- 
Stefan Richter
-=====-==-== ---= =----
http://arcgraph.de/sr/

Re: [PATCH] configfs: change depends -> select SYSFS

[Nicholas A. Bellinger]

On Sun, 2011-01-16 at 14:11 +0100, Stefan Richter wrote:
> On Jan 15 Nicholas A. Bellinger wrote:
> > This patch changes configfs to select SYSFS to fix the following:
> > 
> > warning: (TARGET_CORE && GFS2_FS) selects CONFIGFS_FS which has unmet direct dependencies (SYSFS)
> 
> Why don't you fix target-core's Kconfig instead?

The thought here was that since modern configfs is mounted
at /sys/kernel/config/, selecting SYSFS by default when building
CONFIGFS_FS made the most sense for existing configfs consumers.

> > This patch also fixes whitespace breakage in the 'help' section.
> 
> There is no whitespace breakage in fs/configfs/Kconfig.

Fixed in scsi-post-merge-2.6.git/for-linus.  Thanks for your comments!

--nab

Re: [PATCH] configfs: change depends -> select SYSFS

[Stefan Richter]

On Jan 16 Nicholas A. Bellinger wrote:
> On Sun, 2011-01-16 at 14:11 +0100, Stefan Richter wrote:
> > On Jan 15 Nicholas A. Bellinger wrote:
> > > This patch changes configfs to select SYSFS to fix the following:
> > > 
> > > warning: (TARGET_CORE && GFS2_FS) selects CONFIGFS_FS which has unmet direct dependencies (SYSFS)
> > 
> > Why don't you fix target-core's Kconfig instead?
> 
> The thought here was that since modern configfs is mounted
> at /sys/kernel/config/, selecting SYSFS by default when building
> CONFIGFS_FS made the most sense for existing configfs consumers.

I for one think that layered "select" directives will open too many cans
of worms.

Best don't use select at all.

If you use it, select only options that don't depend on anything else.

If you feel that people really want you to provide a select for them which
selects something that in turn depends on other things, then I suggest you
rather let your own option depend on these lower dependencies:

config HIGHLEVEL_FEATURE
	tristate "some driver"
	depends on SYSFS  # because CONFIGFS depends on it
	select CONFIGFS
-- 
Stefan Richter
-=====-==-== ---= =---=
http://arcgraph.de/sr/

Re: [PATCH] configfs: change depends -> select SYSFS

[Linus Torvalds]

On Sun, Jan 16, 2011 at 3:06 PM, Stefan Richter
<stefanr@s5r6.in-berlin.de> wrote:
> I for one think that layered "select" directives will open too many cans
> of worms.
>
> Best don't use select at all.

That really doesn't work.

The reason everybody wants "select", and "depends on" is near useless,
is that from a usability perspective, it's totally unacceptable to say
"you need to know to enable that random other totally conceptually
unrelated config option".

Why the hell should a OCFS2 user have to know that he needs to enable
configfs to be able to enable ocfs2?

The thing is, we should do exactly the reverse of what you say, and
work towards the goal of "depends on" going away. Right now, the main
reason to use "depends on" is actually just that our Kconfig solvers
are stupid, and don't generally handle "select" very well. But that's
hopefully getting fixed.

                   Linus

Re: [PATCH] configfs: change depends -> select SYSFS

[Al Viro]

On Sun, Jan 16, 2011 at 03:11:57PM -0800, Linus Torvalds wrote:
> Why the hell should a OCFS2 user have to know that he needs to enable
> configfs to be able to enable ocfs2?
> 
> The thing is, we should do exactly the reverse of what you say, and
> work towards the goal of "depends on" going away. Right now, the main
> reason to use "depends on" is actually just that our Kconfig solvers
> are stupid, and don't generally handle "select" very well. But that's
> hopefully getting fixed.

How do you eliminate things like "it really depends on having DMA"?
When we configure e.g. s390 or uml kernel, it's not like we could
enable a PCI driver and have architecture magically switch to a
PCI-supporting one, after all...

Re: [PATCH] configfs: change depends -> select SYSFS

[Linus Torvalds]

On Sun, Jan 16, 2011 at 3:19 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> How do you eliminate things like "it really depends on having DMA"?
> When we configure e.g. s390 or uml kernel, it's not like we could
> enable a PCI driver and have architecture magically switch to a
> PCI-supporting one, after all...

If it's something that cannot be changed, then it should be "depends on".

Think about it - writing a "hard requirement" as "depends on" is
perfectly sane. It makes semantic sense, and it is useful to filter
out things that you simply CANNOT have, because you've selected a
configuration or an architecture that simply do not _support_ those
features.

But if it's something where it's not about a feature that is needed,
but about a configuration dependency, it's IMNSHO almost always the
wrong thing to have "depends on".

Another way of thinking about this is to think about what features are
the one that a user *cares* about, and wants to enable or disable by
hand.

Just think about that for a moment - isn't _that_ what a kernel config
should be asking about?

And now ask yourself, do you really expect some random user to say "I
want to enable SYSFS and CONFIGFS, because I am going to use ocfs2 on
my system"?

Really?

Or do you expect a high-quality implementation of a configuration
script to allow the user to just say "I want ocfs2", and then figure
out the dependencies and solve them for you?

I'd say that the latter case is OBVIOUSLY the quality implementation,
while the former one is just stupid.

                   Linus

Re: [PATCH] configfs: change depends -> select SYSFS

[Al Viro]

On Sun, Jan 16, 2011 at 03:45:35PM -0800, Linus Torvalds wrote:

> And now ask yourself, do you really expect some random user to say "I
> want to enable SYSFS and CONFIGFS, because I am going to use ocfs2 on
> my system"?
> 
> Really?
> 
> Or do you expect a high-quality implementation of a configuration
> script to allow the user to just say "I want ocfs2", and then figure
> out the dependencies and solve them for you?
> 
> I'd say that the latter case is OBVIOUSLY the quality implementation,
> while the former one is just stupid.

It's a bit more complicated than that...
	* nitpick: config option for a driver foo should be seen only after
foo got enabled, so that's another case where we need depends on
	* nitpick 2: some switchable things really shouldn't be selectable,
or you'll get users wondering which of the drivers they'd enabled had
lead to 32bit kernel on their amd64 boxen ;-)
	* all nitpicks aside, there ought to be a way to say "no, I _REALLY_
don't want that shit doing #define if(x) <horrible pile of crap> and if it
means that I can't have oprofile, so be it".  Without chasing tons of select
chains to see which FPOS do I need to disable to get rid of the damn thing.
Right now the propagation of select chains makes it more and more unpleasant.

IOW, it would be very nice to be able to pin a select target down, turning
selects leading to it into hard dependencies.

Another obviously missing thing is that dependencies should propagate back
through select chains.  I.e. if A selects B, then hard dependencies of B
should append to those of A.

It gets really lousy when you have shitloads of drivers spread over several
Kconfig menus, all selecting an option that has a hard dependency.  As it
is, if you want e.g. allmodconfig to produce something that builds you have
to chase them all down and add dependencies to each of those drivers.  We
have such piles in e.g. drivers/media.

Re: [PATCH] configfs: change depends -> select SYSFS

[Rolf Eike Beer]

> On Sun, Jan 16, 2011 at 03:45:35PM -0800, Linus Torvalds wrote:
>
>> And now ask yourself, do you really expect some random user to say "I
>> want to enable SYSFS and CONFIGFS, because I am going to use ocfs2 on
>> my system"?
>>
>> Really?
>>
>> Or do you expect a high-quality implementation of a configuration
>> script to allow the user to just say "I want ocfs2", and then figure
>> out the dependencies and solve them for you?
>>
>> I'd say that the latter case is OBVIOUSLY the quality implementation,
>> while the former one is just stupid.
>
> It's a bit more complicated than that...
> 	* nitpick: config option for a driver foo should be seen only after
> foo got enabled, so that's another case where we need depends on
> 	* nitpick 2: some switchable things really shouldn't be selectable,
> or you'll get users wondering which of the drivers they'd enabled had
> lead to 32bit kernel on their amd64 boxen ;-)
> 	* all nitpicks aside, there ought to be a way to say "no, I _REALLY_
> don't want that shit doing #define if(x) <horrible pile of crap> and if it
> means that I can't have oprofile, so be it".  Without chasing tons of
> select
> chains to see which FPOS do I need to disable to get rid of the damn
> thing.
> Right now the propagation of select chains makes it more and more
> unpleasant.
>
> IOW, it would be very nice to be able to pin a select target down, turning
> selects leading to it into hard dependencies.
>
> Another obviously missing thing is that dependencies should propagate back
> through select chains.  I.e. if A selects B, then hard dependencies of B
> should append to those of A.
>
> It gets really lousy when you have shitloads of drivers spread over
> several
> Kconfig menus, all selecting an option that has a hard dependency.  As it
> is, if you want e.g. allmodconfig to produce something that builds you
> have
> to chase them all down and add dependencies to each of those drivers.  We
> have such piles in e.g. drivers/media.

This sounds _exactly_ like doing a software package selection on distro
install. "No, I don't ever want mono because even if I am then missing
$foo". And it will likely need the same handling, e.g. as zypper does it
already: you don't want to install mono, but $foo depends on it. What to
do? 1) keep mono, 2) don't install $foo. The third option zypper usually
gives (ignore those dependency) usually doesn't make any sense for kernel
stuff so we can omit that.

For menuconfig this will end up that we have something beyond y-n-m, we
need also a forced-no as we currently have auto-m and auto-y.

Eike

Re: [PATCH] configfs: change depends -> select SYSFS

[Stefan Richter]

On Jan 16 Linus Torvalds wrote:
> Or do you expect a high-quality implementation of a configuration
> script to allow the user to just say "I want ocfs2", and then figure
> out the dependencies and solve them for you?
> 
> I'd say that the latter case is OBVIOUSLY the quality implementation,
> while the former one is just stupid.

As Al noted, there also needs to be a convenient way to *disable* options.

Hence, the Kconfig files should merely declare the dependencies.  Then,
some user interfaces should deal with the this database of dependencies
and let different people achieve different goals as easily as possible.

We already have different UIs.  But perhaps we have too much UI down in
the Kconfig files.  I.e., perhaps Kconfig shouldn't know the difference
between depends-on and select.  Let the different UIs present the
dependency graph to the operator in different ways, suitable to different
tasks.

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

Re: [PATCH] configfs: change depends -> select SYSFS

[Randy Dunlap]

On 01/16/11 16:52, Stefan Richter wrote:
> On Jan 16 Linus Torvalds wrote:
>> Or do you expect a high-quality implementation of a configuration
>> script to allow the user to just say "I want ocfs2", and then figure
>> out the dependencies and solve them for you?
>>
>> I'd say that the latter case is OBVIOUSLY the quality implementation,
>> while the former one is just stupid.
> 
> As Al noted, there also needs to be a convenient way to *disable* options.
> 
> Hence, the Kconfig files should merely declare the dependencies.  Then,
> some user interfaces should deal with the this database of dependencies
> and let different people achieve different goals as easily as possible.
> 
> We already have different UIs.  But perhaps we have too much UI down in
> the Kconfig files.  I.e., perhaps Kconfig shouldn't know the difference
> between depends-on and select.  Let the different UIs present the
> dependency graph to the operator in different ways, suitable to different
> tasks.

I agree, I think that the UIs should list a kconfig symbol and its dependencies,
then let the user enable them as needed.

> /handwave off.


-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] configfs: change depends -> select SYSFS

[Bernd Petrovitsch]

On Mon, 2011-01-17 at 01:52 +0100, Stefan Richter wrote:
[...] 
> We already have different UIs.  But perhaps we have too much UI down in
> the Kconfig files.  I.e., perhaps Kconfig shouldn't know the difference

IOMHO not really.

> between depends-on and select.  Let the different UIs present the

The UI should know the difference as it should guide/force the user -
especially the less experienced ones which aren't expected to know every
bit of hardware and kernel-internal stuff (as in "do I need gzip
compression?").

And there are two (IMHO very) different semantics:
- select: enables/activates/includes necessary other parts (like the
  "gzip compression" example from above and other library-like).
- depends on: to purposely disable knobs because it makes no sense to
  configure e.g. ext-specific stuff if I don't have/want ext or because
  the physical hardware is not present/available (find more and better
  in Al Viros mail of course).

> dependency graph to the operator in different ways, suitable to different
> tasks.

That would be quite cute.

> /handwave off.
AOL.
Bernd
-- 
Bernd Petrovitsch                  Email : bernd@petrovitsch.priv.at
                     LUGA : http://www.luga.at

Re: [PATCH] configfs: change depends -> select SYSFS

[Nicholas A. Bellinger]

On Mon, 2011-01-17 at 00:06 +0100, Stefan Richter wrote:
> On Jan 16 Nicholas A. Bellinger wrote:
> > On Sun, 2011-01-16 at 14:11 +0100, Stefan Richter wrote:
> > > On Jan 15 Nicholas A. Bellinger wrote:
> > > > This patch changes configfs to select SYSFS to fix the following:
> > > > 
> > > > warning: (TARGET_CORE && GFS2_FS) selects CONFIGFS_FS which has unmet direct dependencies (SYSFS)
> > > 
> > > Why don't you fix target-core's Kconfig instead?
> > 
> > The thought here was that since modern configfs is mounted
> > at /sys/kernel/config/, selecting SYSFS by default when building
> > CONFIGFS_FS made the most sense for existing configfs consumers.
> 
> I for one think that layered "select" directives will open too many cans
> of worms.
> 
> Best don't use select at all.
> 
> If you use it, select only options that don't depend on anything else.
> 
> If you feel that people really want you to provide a select for them which
> selects something that in turn depends on other things, then I suggest you
> rather let your own option depend on these lower dependencies:
> 
> config HIGHLEVEL_FEATURE
> 	tristate "some driver"
> 	depends on SYSFS  # because CONFIGFS depends on it
> 	select CONFIGFS

I think this is a fair point..  As I don't really have a strong
preference either way, I will have to defer to Randy and Joel's better
judgement here.  Guys, what would you prefer..?

Thanks,

--nab

Re: [PATCH] configfs: change depends -> select SYSFS

[James Bottomley]

On Mon, 2011-01-17 at 00:06 +0100, Stefan Richter wrote:
> On Jan 16 Nicholas A. Bellinger wrote:
> > On Sun, 2011-01-16 at 14:11 +0100, Stefan Richter wrote:
> > > On Jan 15 Nicholas A. Bellinger wrote:
> > > > This patch changes configfs to select SYSFS to fix the following:
> > > > 
> > > > warning: (TARGET_CORE && GFS2_FS) selects CONFIGFS_FS which has unmet direct dependencies (SYSFS)
> > > 
> > > Why don't you fix target-core's Kconfig instead?
> > 
> > The thought here was that since modern configfs is mounted
> > at /sys/kernel/config/, selecting SYSFS by default when building
> > CONFIGFS_FS made the most sense for existing configfs consumers.
> 
> I for one think that layered "select" directives will open too many cans
> of worms.

select, since we have it, should be clean ... as in if you select
something, you don't have to expose yourself to a huge pile of missing
depends that only show up in obscure configurations.


> Best don't use select at all.

The object of select is not to trip up the user.  If we used a purely
depend based configuration, the user would have to know to select, say,
the right SCSI transport classes before they get presented with drivers.
It's completely correct, since transport classes are internal
implementations, to have the user select drivers and Kconfig work out
via the select directive what transport classes are needed.

> If you use it, select only options that don't depend on anything else.
> 
> If you feel that people really want you to provide a select for them which
> selects something that in turn depends on other things, then I suggest you
> rather let your own option depend on these lower dependencies:
> 
> config HIGHLEVEL_FEATURE
> 	tristate "some driver"
> 	depends on SYSFS  # because CONFIGFS depends on it
> 	select CONFIGFS

This is what I don't understand.

Actually I think the whole premise of the patch (to get back to the
original topic) is wrong.

TARGET_CORE depends on SCSI; SCSI has to have sysfs to survive ... we
just don't work without it yet we neither select nor depend on it.
SYSFS is only deselectable for embedded anyway, so I think the
configuration which generated this whole argument was likely a bogus one
and consequently, none of the patches are needed (or if they are,
they're the tip of the iceberg).

James

Re: [PATCH] configfs: change depends -> select SYSFS

[Nicholas A. Bellinger]

On Sun, 2011-01-16 at 18:22 -0600, James Bottomley wrote:
> On Mon, 2011-01-17 at 00:06 +0100, Stefan Richter wrote:
> > On Jan 16 Nicholas A. Bellinger wrote:
> > > On Sun, 2011-01-16 at 14:11 +0100, Stefan Richter wrote:
> > > > On Jan 15 Nicholas A. Bellinger wrote:
> > > > > This patch changes configfs to select SYSFS to fix the following:
> > > > > 
> > > > > warning: (TARGET_CORE && GFS2_FS) selects CONFIGFS_FS which has unmet direct dependencies (SYSFS)
> > > > 
> > > > Why don't you fix target-core's Kconfig instead?
> > > 
> > > The thought here was that since modern configfs is mounted
> > > at /sys/kernel/config/, selecting SYSFS by default when building
> > > CONFIGFS_FS made the most sense for existing configfs consumers.
> > 
> > I for one think that layered "select" directives will open too many cans
> > of worms.
> 
> select, since we have it, should be clean ... as in if you select
> something, you don't have to expose yourself to a huge pile of missing
> depends that only show up in obscure configurations.
> 
> 
> > Best don't use select at all.
> 
> The object of select is not to trip up the user.  If we used a purely
> depend based configuration, the user would have to know to select, say,
> the right SCSI transport classes before they get presented with drivers.
> It's completely correct, since transport classes are internal
> implementations, to have the user select drivers and Kconfig work out
> via the select directive what transport classes are needed.
> 
> > If you use it, select only options that don't depend on anything else.
> > 
> > If you feel that people really want you to provide a select for them which
> > selects something that in turn depends on other things, then I suggest you
> > rather let your own option depend on these lower dependencies:
> > 
> > config HIGHLEVEL_FEATURE
> > 	tristate "some driver"
> > 	depends on SYSFS  # because CONFIGFS depends on it
> > 	select CONFIGFS
> 
> This is what I don't understand.
> 
> Actually I think the whole premise of the patch (to get back to the
> original topic) is wrong.
> 
> TARGET_CORE depends on SCSI; SCSI has to have sysfs to survive ... we
> just don't work without it yet we neither select nor depend on it.
> SYSFS is only deselectable for embedded anyway, so I think the
> configuration which generated this whole argument was likely a bogus one
> and consequently, none of the patches are needed (or if they are,
> they're the tip of the iceberg).
> 

This sounds fine for TARGET_CORE, but would still leave GFS2_FS with an
unmet direct dependency according to the original warning above.
Unfortuately I do not recall which exactly linux-next build
configuration was causing this warning to occur from the original post:

http://marc.info/?l=linux-next&m=129355383112997&w=2

Any more thoughts here Randy..?

Thanks,

--nab

Re: [PATCH] configfs: change depends -> select SYSFS

[Randy Dunlap]

On Sun, 16 Jan 2011 22:05:54 -0800 Nicholas A. Bellinger wrote:

> On Sun, 2011-01-16 at 18:22 -0600, James Bottomley wrote:
> > On Mon, 2011-01-17 at 00:06 +0100, Stefan Richter wrote:
> > > On Jan 16 Nicholas A. Bellinger wrote:
> > > > On Sun, 2011-01-16 at 14:11 +0100, Stefan Richter wrote:
> > > > > On Jan 15 Nicholas A. Bellinger wrote:
> > > > > > This patch changes configfs to select SYSFS to fix the following:
> > > > > > 
> > > > > > warning: (TARGET_CORE && GFS2_FS) selects CONFIGFS_FS which has unmet direct dependencies (SYSFS)
> > > > > 
> > > > > Why don't you fix target-core's Kconfig instead?
> > > > 
> > > > The thought here was that since modern configfs is mounted
> > > > at /sys/kernel/config/, selecting SYSFS by default when building
> > > > CONFIGFS_FS made the most sense for existing configfs consumers.
> > > 
> > > I for one think that layered "select" directives will open too many cans
> > > of worms.
> > 
> > select, since we have it, should be clean ... as in if you select
> > something, you don't have to expose yourself to a huge pile of missing
> > depends that only show up in obscure configurations.
> > 
> > 
> > > Best don't use select at all.
> > 
> > The object of select is not to trip up the user.  If we used a purely
> > depend based configuration, the user would have to know to select, say,
> > the right SCSI transport classes before they get presented with drivers.
> > It's completely correct, since transport classes are internal
> > implementations, to have the user select drivers and Kconfig work out
> > via the select directive what transport classes are needed.
> > 
> > > If you use it, select only options that don't depend on anything else.
> > > 
> > > If you feel that people really want you to provide a select for them which
> > > selects something that in turn depends on other things, then I suggest you
> > > rather let your own option depend on these lower dependencies:
> > > 
> > > config HIGHLEVEL_FEATURE
> > > 	tristate "some driver"
> > > 	depends on SYSFS  # because CONFIGFS depends on it
> > > 	select CONFIGFS
> > 
> > This is what I don't understand.
> > 
> > Actually I think the whole premise of the patch (to get back to the
> > original topic) is wrong.
> > 
> > TARGET_CORE depends on SCSI; SCSI has to have sysfs to survive ... we
> > just don't work without it yet we neither select nor depend on it.
> > SYSFS is only deselectable for embedded anyway, so I think the
> > configuration which generated this whole argument was likely a bogus one
> > and consequently, none of the patches are needed (or if they are,
> > they're the tip of the iceberg).
> > 
> 
> This sounds fine for TARGET_CORE, but would still leave GFS2_FS with an
> unmet direct dependency according to the original warning above.
> Unfortuately I do not recall which exactly linux-next build
> configuration was causing this warning to occur from the original post:
> 
> http://marc.info/?l=linux-next&m=129355383112997&w=2
> 
> Any more thoughts here Randy..?


I've looked at GFS2 a bit now and I think that the warning is bogus:

kconfig complains with:
warning: (TARGET_CORE && GFS2_FS) selects CONFIGFS_FS which has unmet direct dependencies (SYSFS)

but the "select" is conditional:
config GFS2_FS
	tristate "GFS2 file system support"
	depends on (64BIT || LBDAF)
	select DLM if GFS2_FS_LOCKING_DLM
	select CONFIGFS_FS if GFS2_FS_LOCKING_DLM
	select SYSFS if GFS2_FS_LOCKING_DLM

and the same condition selects both SYSFS and CONFIGFS_FS.  Furthermore, the
conditional is not true, so neither of them is being selected/enabled.
Looks like a minor kconfig buglet to me.


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] configfs: change depends -> select SYSFS

[Nicholas A. Bellinger]

On Mon, 2011-01-17 at 10:24 -0800, Randy Dunlap wrote:
> On Sun, 16 Jan 2011 22:05:54 -0800 Nicholas A. Bellinger wrote:
> 
> > On Sun, 2011-01-16 at 18:22 -0600, James Bottomley wrote:

<SNIP>

> > > This is what I don't understand.
> > > 
> > > Actually I think the whole premise of the patch (to get back to the
> > > original topic) is wrong.
> > > 
> > > TARGET_CORE depends on SCSI; SCSI has to have sysfs to survive ... we
> > > just don't work without it yet we neither select nor depend on it.
> > > SYSFS is only deselectable for embedded anyway, so I think the
> > > configuration which generated this whole argument was likely a bogus one
> > > and consequently, none of the patches are needed (or if they are,
> > > they're the tip of the iceberg).
> > > 
> > 
> > This sounds fine for TARGET_CORE, but would still leave GFS2_FS with an
> > unmet direct dependency according to the original warning above.
> > Unfortuately I do not recall which exactly linux-next build
> > configuration was causing this warning to occur from the original post:
> > 
> > http://marc.info/?l=linux-next&m=129355383112997&w=2
> > 
> > Any more thoughts here Randy..?
> 
> 
> I've looked at GFS2 a bit now and I think that the warning is bogus:
> 
> kconfig complains with:
> warning: (TARGET_CORE && GFS2_FS) selects CONFIGFS_FS which has unmet direct dependencies (SYSFS)
> 
> but the "select" is conditional:
> config GFS2_FS
> 	tristate "GFS2 file system support"
> 	depends on (64BIT || LBDAF)
> 	select DLM if GFS2_FS_LOCKING_DLM
> 	select CONFIGFS_FS if GFS2_FS_LOCKING_DLM
> 	select SYSFS if GFS2_FS_LOCKING_DLM
> 
> and the same condition selects both SYSFS and CONFIGFS_FS.  Furthermore, the
> conditional is not true, so neither of them is being selected/enabled.
> Looks like a minor kconfig buglet to me.
> 

Ok, so Linus has pulled the CONFIGFS_FS -> select SYSFS series and it
looks like this 'select SYSFS ...' bit for GFS2_FS can safely be dropped
now..

Care to carry this one via your kbuild tree..?

Thanks!

--nab

Re: [PATCH] configfs: change depends -> select SYSFS

[Randy Dunlap]

On 01/17/11 14:10, Nicholas A. Bellinger wrote:
> On Mon, 2011-01-17 at 10:24 -0800, Randy Dunlap wrote:
>> On Sun, 16 Jan 2011 22:05:54 -0800 Nicholas A. Bellinger wrote:
>>
>>> On Sun, 2011-01-16 at 18:22 -0600, James Bottomley wrote:
> 
> <SNIP>
> 
>>>> This is what I don't understand.
>>>>
>>>> Actually I think the whole premise of the patch (to get back to the
>>>> original topic) is wrong.
>>>>
>>>> TARGET_CORE depends on SCSI; SCSI has to have sysfs to survive ... we
>>>> just don't work without it yet we neither select nor depend on it.
>>>> SYSFS is only deselectable for embedded anyway, so I think the
>>>> configuration which generated this whole argument was likely a bogus one
>>>> and consequently, none of the patches are needed (or if they are,
>>>> they're the tip of the iceberg).
>>>>
>>>
>>> This sounds fine for TARGET_CORE, but would still leave GFS2_FS with an
>>> unmet direct dependency according to the original warning above.
>>> Unfortuately I do not recall which exactly linux-next build
>>> configuration was causing this warning to occur from the original post:
>>>
>>> http://marc.info/?l=linux-next&m=129355383112997&w=2
>>>
>>> Any more thoughts here Randy..?
>>
>>
>> I've looked at GFS2 a bit now and I think that the warning is bogus:
>>
>> kconfig complains with:
>> warning: (TARGET_CORE && GFS2_FS) selects CONFIGFS_FS which has unmet direct dependencies (SYSFS)
>>
>> but the "select" is conditional:
>> config GFS2_FS
>> 	tristate "GFS2 file system support"
>> 	depends on (64BIT || LBDAF)
>> 	select DLM if GFS2_FS_LOCKING_DLM
>> 	select CONFIGFS_FS if GFS2_FS_LOCKING_DLM
>> 	select SYSFS if GFS2_FS_LOCKING_DLM
>>
>> and the same condition selects both SYSFS and CONFIGFS_FS.  Furthermore, the
>> conditional is not true, so neither of them is being selected/enabled.
>> Looks like a minor kconfig buglet to me.
>>
> 
> Ok, so Linus has pulled the CONFIGFS_FS -> select SYSFS series and it
> looks like this 'select SYSFS ...' bit for GFS2_FS can safely be dropped
> now..
> 
> Care to carry this one via your kbuild tree..?


Who are you asking?  (I don't have a kbuild tree.)

-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] configfs: change depends -> select SYSFS

[Nicholas A. Bellinger]

On Mon, 2011-01-17 at 16:13 -0800, Randy Dunlap wrote:
> On 01/17/11 14:10, Nicholas A. Bellinger wrote:
> > On Mon, 2011-01-17 at 10:24 -0800, Randy Dunlap wrote:
> >> On Sun, 16 Jan 2011 22:05:54 -0800 Nicholas A. Bellinger wrote:
> >>
> >>> On Sun, 2011-01-16 at 18:22 -0600, James Bottomley wrote:
> > 
> > <SNIP>
> > 
> >>>> This is what I don't understand.
> >>>>
> >>>> Actually I think the whole premise of the patch (to get back to the
> >>>> original topic) is wrong.
> >>>>
> >>>> TARGET_CORE depends on SCSI; SCSI has to have sysfs to survive ... we
> >>>> just don't work without it yet we neither select nor depend on it.
> >>>> SYSFS is only deselectable for embedded anyway, so I think the
> >>>> configuration which generated this whole argument was likely a bogus one
> >>>> and consequently, none of the patches are needed (or if they are,
> >>>> they're the tip of the iceberg).
> >>>>
> >>>
> >>> This sounds fine for TARGET_CORE, but would still leave GFS2_FS with an
> >>> unmet direct dependency according to the original warning above.
> >>> Unfortuately I do not recall which exactly linux-next build
> >>> configuration was causing this warning to occur from the original post:
> >>>
> >>> http://marc.info/?l=linux-next&m=129355383112997&w=2
> >>>
> >>> Any more thoughts here Randy..?
> >>
> >>
> >> I've looked at GFS2 a bit now and I think that the warning is bogus:
> >>
> >> kconfig complains with:
> >> warning: (TARGET_CORE && GFS2_FS) selects CONFIGFS_FS which has unmet direct dependencies (SYSFS)
> >>
> >> but the "select" is conditional:
> >> config GFS2_FS
> >> 	tristate "GFS2 file system support"
> >> 	depends on (64BIT || LBDAF)
> >> 	select DLM if GFS2_FS_LOCKING_DLM
> >> 	select CONFIGFS_FS if GFS2_FS_LOCKING_DLM
> >> 	select SYSFS if GFS2_FS_LOCKING_DLM
> >>
> >> and the same condition selects both SYSFS and CONFIGFS_FS.  Furthermore, the
> >> conditional is not true, so neither of them is being selected/enabled.
> >> Looks like a minor kconfig buglet to me.
> >>
> > 
> > Ok, so Linus has pulled the CONFIGFS_FS -> select SYSFS series and it
> > looks like this 'select SYSFS ...' bit for GFS2_FS can safely be dropped
> > now..
> > 
> > Care to carry this one via your kbuild tree..?
> 
> 
> Who are you asking?  (I don't have a kbuild tree.)
> 

Err sorry, I thought you where feeding these types of kbuild/kconfig
changes into mainline via your own tree.  In that case, here is the
patch committed into the LIO upstream tree and
scsi-post-merge-2.6.git/for-linus branch.

Thanks!

>From 5ab4b07c2b516e23cafdc24b06d5cbfd44141ca9 Mon Sep 17 00:00:00 2001
From: Nicholas Bellinger <nab@linux-iscsi.org>
Date: Tue, 18 Jan 2011 00:40:31 +0000
Subject: [PATCH] gfs2: Remove 'select SYSFS ...' from Kconfig

With CONFIGFS_FS now doing 'select SYSFS' by default, the extra
'select SYSFS if GFS2_FS_LOCKING_DLM' for GFS2_FS is now unnecessary.

Reported-by: Randy Dunlap <randy.dunlap@oracle.com>
Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
---
 fs/gfs2/Kconfig |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/fs/gfs2/Kconfig b/fs/gfs2/Kconfig
index c465ae0..ff0a8eb 100644
--- a/fs/gfs2/Kconfig
+++ b/fs/gfs2/Kconfig
@@ -3,7 +3,6 @@ config GFS2_FS
        depends on (64BIT || LBDAF)
        select DLM if GFS2_FS_LOCKING_DLM
        select CONFIGFS_FS if GFS2_FS_LOCKING_DLM
-       select SYSFS if GFS2_FS_LOCKING_DLM
        select IP_SCTP if DLM_SCTP
        select FS_POSIX_ACL
        select CRC32
-- 
1.7.3.5

Re: [PATCH] configfs: change depends -> select SYSFS

[Randy Dunlap]

On Sun, 16 Jan 2011 18:22:12 -0600 James Bottomley wrote:

> On Mon, 2011-01-17 at 00:06 +0100, Stefan Richter wrote:
> > On Jan 16 Nicholas A. Bellinger wrote:
> > > On Sun, 2011-01-16 at 14:11 +0100, Stefan Richter wrote:
> > > > On Jan 15 Nicholas A. Bellinger wrote:
> > > > > This patch changes configfs to select SYSFS to fix the following:
> > > > > 
> > > > > warning: (TARGET_CORE && GFS2_FS) selects CONFIGFS_FS which has unmet direct dependencies (SYSFS)
> > > > 
> > > > Why don't you fix target-core's Kconfig instead?
> > > 
> > > The thought here was that since modern configfs is mounted
> > > at /sys/kernel/config/, selecting SYSFS by default when building
> > > CONFIGFS_FS made the most sense for existing configfs consumers.
> > 
> > I for one think that layered "select" directives will open too many cans
> > of worms.
> 
> select, since we have it, should be clean ... as in if you select
> something, you don't have to expose yourself to a huge pile of missing
> depends that only show up in obscure configurations.
> 
> 
> > Best don't use select at all.
> 
> The object of select is not to trip up the user.  If we used a purely
> depend based configuration, the user would have to know to select, say,
> the right SCSI transport classes before they get presented with drivers.
> It's completely correct, since transport classes are internal
> implementations, to have the user select drivers and Kconfig work out
> via the select directive what transport classes are needed.
> 
> > If you use it, select only options that don't depend on anything else.
> > 
> > If you feel that people really want you to provide a select for them which
> > selects something that in turn depends on other things, then I suggest you
> > rather let your own option depend on these lower dependencies:
> > 
> > config HIGHLEVEL_FEATURE
> > 	tristate "some driver"
> > 	depends on SYSFS  # because CONFIGFS depends on it
> > 	select CONFIGFS
> 
> This is what I don't understand.
> 
> Actually I think the whole premise of the patch (to get back to the
> original topic) is wrong.
> 
> TARGET_CORE depends on SCSI; SCSI has to have sysfs to survive ... we
> just don't work without it yet we neither select nor depend on it.
> SYSFS is only deselectable for embedded anyway, so I think the
> configuration which generated this whole argument was likely a bogus one
> and consequently, none of the patches are needed (or if they are,
> they're the tip of the iceberg).

This sounds like a problem.
SCSI subsystem will certainly build without having SYSFS enabled, but I think
that you are saying that it won't function without SYSFS.  Then it depends on
SYSFS and should say so somewhere (IMO of course).

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] configfs: change depends -> select SYSFS

[Linus Torvalds]

On Sat, Jan 15, 2011 at 1:10 PM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
>
> This patch changes configfs to select SYSFS to fix the following:
>
> warning: (TARGET_CORE && GFS2_FS) selects CONFIGFS_FS which has unmet direct dependencies (SYSFS)

I'm ok with this part. But:

> This patch also fixes whitespace breakage in the 'help' section.

This is just bogus. The help text is _supposed_ to have that extra
indentation so that it stands out from the rest of the Kconfig stuff.

                   Linus

Re: [PATCH] configfs: change depends -> select SYSFS

[Nicholas A. Bellinger]

On Sun, 2011-01-16 at 09:54 -0800, Linus Torvalds wrote:
> On Sat, Jan 15, 2011 at 1:10 PM, Nicholas A. Bellinger
> <nab@linux-iscsi.org> wrote:
> >
> > This patch changes configfs to select SYSFS to fix the following:
> >
> > warning: (TARGET_CORE && GFS2_FS) selects CONFIGFS_FS which has unmet direct dependencies (SYSFS)
> 
> I'm ok with this part. But:
> 
> > This patch also fixes whitespace breakage in the 'help' section.
> 
> This is just bogus. The help text is _supposed_ to have that extra
> indentation so that it stands out from the rest of the Kconfig stuff.
> 

Ahh, thanks for the clarification here.   At some point I saw a patch
that did make this change, and it stuck with me that it was the
preferred method.  This won't happen again.

The updated patch to keep the extra indentation for configfs, along with
the three extra 'depends CONFIGFS_FS' patches from yesterday are
available in a pullable branch against this afternoon's
linux-2.6.git/master here:

git://git.kernel.org/pub/scm/linux/kernel/git/nab/scsi-post-merge-2.6.git for-linus

Please let me know if you have any questions,

--nab

Nicholas Bellinger (4):
  configfs: change depends -> select SYSFS
  net: Make NETCONSOLE_DYNAMIC depend on CONFIGFS_FS
  dlm: Make DLM depend on CONFIGFS_FS
  ocfs2: Make OCFS2_FS depend on CONFIGFS_FS

 drivers/net/Kconfig |    3 +--
 fs/configfs/Kconfig |    4 ++--
 fs/dlm/Kconfig      |    3 +--
 fs/ocfs2/Kconfig    |    3 +--
 4 files changed, 5 insertions(+), 8 deletions(-)