| Message ID | d6715b5b-0aa3-3032-43c6-eccd907a60b8@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <linux/kobject.h> #include <linux/bug.h> #include <linux/genhd.h> -#include <linux/debugfs.h> #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, &btrfs_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, - &btrfs_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(&btrfs_kset->kobj, &btrfs_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(&btrfs_kset->kobj, &btrfs_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,
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 <sandeen@redhat.com> --- -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html