Re: [PATCH] btrfs: remove pointless debugfs interface

2016-09-01 Thread David Sterba 
On Wed, Aug 31, 2016 at 06:36:34PM -0400, Jeff Mahoney wrote:
> On 8/31/16 3:08 PM, David Sterba wrote:
> > On Wed, Aug 31, 2016 at 10:13:49AM -0500, Eric Sandeen wrote:
> >> A /sys/kernel/debug/btrfs/test file was added nearly
> >> two and a half years ago, but it serves no purpose;
> > 
> > It does. Introduced in 1bae30982bc86ab66d61ccb6e22792593b45d44d says
> > something about helping developers to easily export information from the
> > filesystem, to aid debugging. Writing the debugfs support code is not
> > obviously trivial, so it's idling in the source. Exporing a new value is
> > as easy as copy and update 3 lines of code. If you have no use for it,
> > fine.
> > 
> >> it stores and returns a value, but nothing in the btrfs
> >> code uses this value in any way.  There are no other btrfs
> >> files in this debugfs dir.
> >>
> >> This was brought to my attention because it is world-writable;
> >> it is the only such file under /sys/kernel/debug, and without
> >> knowledge of its purpose, some users were alarmed by this.
> > 
> > So let's fix the permissions.
> 
> Perhaps we can also just stick it behind a CONFIG option as well if the
> intention is to keep it around for developer debugging purposes.

So let's hide creating the 'test' file under BTRFS_DEBUG at least.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: remove pointless debugfs interface

2016-09-01 Thread David Sterba 
On Wed, Aug 31, 2016 at 04:38:16PM -0500, Eric Sandeen wrote:
> On 8/31/16 2:08 PM, David Sterba wrote:
> > On Wed, Aug 31, 2016 at 10:13:49AM -0500, Eric Sandeen wrote:
> >> A /sys/kernel/debug/btrfs/test file was added nearly
> >> two and a half years ago, but it serves no purpose;
> > 
> > It does. Introduced in 1bae30982bc86ab66d61ccb6e22792593b45d44d says
> > something about helping developers to easily export information from the
> > filesystem, to aid debugging. Writing the debugfs support code is not
> > obviously trivial, so it's idling in the source. Exporing a new value is
> > as easy as copy and update 3 lines of code. If you have no use for it,
> > fine.
> 
> I had thought that Documentation/filesystems/debugfs.txt would suffice,
> but if you keep stuff lying around in btrfs just in case somebody needs
> to export a global variable in the future, I suppose that's cool too.  ;)

How much time would you spend coding it?  I guess more than a couple of
minutes, possibly more than once.  And not in the middle of debugging
something else.  There are more examples of code that has no apparent
user but is used for debugging.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: remove pointless debugfs interface

2016-08-31 Thread Jeff Mahoney 
On 8/31/16 3:08 PM, David Sterba wrote:
> On Wed, Aug 31, 2016 at 10:13:49AM -0500, Eric Sandeen wrote:
>> A /sys/kernel/debug/btrfs/test file was added nearly
>> two and a half years ago, but it serves no purpose;
> 
> It does. Introduced in 1bae30982bc86ab66d61ccb6e22792593b45d44d says
> something about helping developers to easily export information from the
> filesystem, to aid debugging. Writing the debugfs support code is not
> obviously trivial, so it's idling in the source. Exporing a new value is
> as easy as copy and update 3 lines of code. If you have no use for it,
> fine.
> 
>> it stores and returns a value, but nothing in the btrfs
>> code uses this value in any way.  There are no other btrfs
>> files in this debugfs dir.
>>
>> This was brought to my attention because it is world-writable;
>> it is the only such file under /sys/kernel/debug, and without
>> knowledge of its purpose, some users were alarmed by this.
> 
> So let's fix the permissions.

Perhaps we can also just stick it behind a CONFIG option as well if the
intention is to keep it around for developer debugging purposes.

-Jeff

-- 
Jeff Mahoney
SUSE Labs



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] btrfs: remove pointless debugfs interface

2016-08-31 Thread Eric Sandeen 
On 8/31/16 2:08 PM, David Sterba wrote:
> On Wed, Aug 31, 2016 at 10:13:49AM -0500, Eric Sandeen wrote:
>> A /sys/kernel/debug/btrfs/test file was added nearly
>> two and a half years ago, but it serves no purpose;
> 
> It does. Introduced in 1bae30982bc86ab66d61ccb6e22792593b45d44d says
> something about helping developers to easily export information from the
> filesystem, to aid debugging. Writing the debugfs support code is not
> obviously trivial, so it's idling in the source. Exporing a new value is
> as easy as copy and update 3 lines of code. If you have no use for it,
> fine.

I had thought that Documentation/filesystems/debugfs.txt would suffice,
but if you keep stuff lying around in btrfs just in case somebody needs
to export a global variable in the future, I suppose that's cool too.  ;)

>> it stores and returns a value, but nothing in the btrfs
>> code uses this value in any way.  There are no other btrfs
>> files in this debugfs dir.
>>
>> This was brought to my attention because it is world-writable;
>> it is the only such file under /sys/kernel/debug, and without
>> knowledge of its purpose, some users were alarmed by this.
> 
> So let's fix the permissions.

*shrug* ok.

-Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: remove pointless debugfs interface

2016-08-31 Thread David Sterba 
On Wed, Aug 31, 2016 at 10:13:49AM -0500, Eric Sandeen wrote:
> A /sys/kernel/debug/btrfs/test file was added nearly
> two and a half years ago, but it serves no purpose;

It does. Introduced in 1bae30982bc86ab66d61ccb6e22792593b45d44d says
something about helping developers to easily export information from the
filesystem, to aid debugging. Writing the debugfs support code is not
obviously trivial, so it's idling in the source. Exporing a new value is
as easy as copy and update 3 lines of code. If you have no use for it,
fine.

> it stores and returns a value, but nothing in the btrfs
> code uses this value in any way.  There are no other btrfs
> files in this debugfs dir.
> 
> This was brought to my attention because it is world-writable;
> it is the only such file under /sys/kernel/debug, and without
> knowledge of its purpose, some users were alarmed by this.

So let's fix the permissions.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] btrfs: remove pointless debugfs interface

2016-08-31 Thread Eric Sandeen 
A /sys/kernel/debug/btrfs/test file was added nearly
two and a half years ago, but it serves no purpose;
it stores and returns a value, but nothing in the btrfs
code uses this value in any way.  There are no other btrfs
files in this debugfs dir.

This was brought to my attention because it is world-writable;
it is the only such file under /sys/kernel/debug, and without
knowledge of its purpose, some users were alarmed by this.

Another option would be to change the perms, but given that
there is no point to it as far as I can tell, it seems best
to simply remove it.

Signed-off-by: Eric Sandeen 
---

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 4879656..8c3ffb9 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -24,7 +24,6 @@
 #include 
 #include 
 #include 
-#include 
 
 #include "ctree.h"
 #include "disk-io.h"
@@ -728,12 +727,6 @@ int btrfs_sysfs_add_device_link(struct btrfs_fs_devices 
*fs_devices,
 /* /sys/fs/btrfs/ entry */
 static struct kset *btrfs_kset;
 
-/* /sys/kernel/debug/btrfs */
-static struct dentry *btrfs_debugfs_root_dentry;
-
-/* Debugging tunables and exported data */
-u64 btrfs_debugfs_test;
-
 /*
  * Can be called by the device discovery thread.
  * And parent can be specified for seed device
@@ -827,19 +820,6 @@ void btrfs_sysfs_feature_update(struct btrfs_fs_info 
*fs_info,
ret = sysfs_create_group(fsid_kobj, _feature_attr_group);
 }
 
-static int btrfs_init_debugfs(void)
-{
-#ifdef CONFIG_DEBUG_FS
-   btrfs_debugfs_root_dentry = debugfs_create_dir("btrfs", NULL);
-   if (!btrfs_debugfs_root_dentry)
-   return -ENOMEM;
-
-   debugfs_create_u64("test", S_IRUGO | S_IWUGO, btrfs_debugfs_root_dentry,
-   _debugfs_test);
-#endif
-   return 0;
-}
-
 int btrfs_init_sysfs(void)
 {
int ret;
@@ -848,28 +828,19 @@ int btrfs_init_sysfs(void)
if (!btrfs_kset)
return -ENOMEM;
 
-   ret = btrfs_init_debugfs();
-   if (ret)
-   goto out1;
-
init_feature_attrs();
ret = sysfs_create_group(_kset->kobj, _feature_attr_group);
-   if (ret)
-   goto out2;
+   if (ret) {
+   kset_unregister(btrfs_kset);
+   return ret;
+   }
 
return 0;
-out2:
-   debugfs_remove_recursive(btrfs_debugfs_root_dentry);
-out1:
-   kset_unregister(btrfs_kset);
-
-   return ret;
 }
 
 void btrfs_exit_sysfs(void)
 {
sysfs_remove_group(_kset->kobj, _feature_attr_group);
kset_unregister(btrfs_kset);
-   debugfs_remove_recursive(btrfs_debugfs_root_dentry);
 }
 
diff --git a/fs/btrfs/sysfs.h b/fs/btrfs/sysfs.h
index d7da1a4..45bad52 100644
--- a/fs/btrfs/sysfs.h
+++ b/fs/btrfs/sysfs.h
@@ -1,11 +1,6 @@
 #ifndef _BTRFS_SYSFS_H_
 #define _BTRFS_SYSFS_H_
 
-/*
- * Data exported through sysfs
- */
-extern u64 btrfs_debugfs_test;
-
 enum btrfs_feature_set {
FEAT_COMPAT,
FEAT_COMPAT_RO,

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html