* [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs
@ 2013-09-12 2:29 Tejun Heo
2013-09-12 2:29 ` [PATCH 1/7] sysfs: drop semicolon from to_sysfs_dirent() definition Tejun Heo
` (8 more replies)
0 siblings, 9 replies; 21+ messages in thread
From: Tejun Heo @ 2013-09-12 2:29 UTC (permalink / raw)
To: gregkh; +Cc: linux-kernel, kay, ebiederm, netdev, lizefan
Hello,
I'll send out multiple patchsets to separate out sysfs from driver
core and kobject. The eventual goal is making sysfs modular enough so
that cgroup can replace its nightmarish cgroupfs implementation which
repeated and worsened all the past mistakes of sysfs. This patchset
is first of the effort and separates out kobject namespace handling
from sysfs.
I never really understood why namespace support was added the way it
was added. Namespace information is communicated to sysfs via
callbacks and back-queries to upper layer, which is a very unusual and
weird thing to do when all the involved operations are synchronous.
For example, a tagged attribute creation looks like the following.
driver code driver callback
v ^
netdev_class_create_file() |
v class_attr->namespace()
class_create_file() class_attr_namespace()
v |
sysfs_create_file() |
v |
sysfs_attr_ns() -------------> sysfs_ops->namespace()
This is an absurd thing to do. It significantly obfuscates what's
going on and adds unnecessary uncertainties - for example, can
namespace() return value disagree with the recorded s_ns value without
being renamed? If so, how should that be handled? If not, what
guarantees that? Even the basic placements of callbacks don't make
much, if any, sense. Why is per-directory namespace() callback in
kobj_type while per-attr namespace() callback is in sysfs_ops? What
does this even mean?
Maybe there's some grand design scheme governing all this but it isn't
obvious at all and the whole thing looks like a hodgepodge of
short-sighted hacks.
There is absolutely *nothing* which requires this convolution. NS tag
can simply be passed down the stack just like any other type of
information and adding an extra argument or variant of interface to
pass down the extra information is way more straight-forward and
apparently even takes less amount of code, so let's please stop the
insanity.
This patchset contains the following seven patches.
0001-sysfs-drop-semicolon-from-to_sysfs_dirent-definition.patch
0002-sysfs-make-attr-namespace-interface-less-convoluted.patch
0003-sysfs-remove-ktype-namespace-invocations-in-director.patch
0004-sysfs-remove-ktype-namespace-invocations-in-symlink-.patch
0005-sysfs-drop-kobj_ns_type-handling.patch
0006-sysfs-clean-up-sysfs_get_dirent.patch
0007-sysfs-name-comes-before-ns.patch
0001 is a minor prep patch.
0002 removes the attr namespace callback.
0003-0004 push the dir namespace callback invocations from sysfs to
kobjct layer. Eventually, this callback should go too.
0005 simplifies sysfs ns support such that sysfs doesn't have any
specific knowledge of kobj namespaces. It now purely deals with
pointer tags. Combined with the previous changes, this makes sysfs ns
support mostly modular.
0006-0007 are cleanup patches to make param orders conventional and
consistent - optional param after mandatory one; otherwise, things get
extremely confusing with different variants of interfaces which take
or don't take optional params. No idea how/why this was done the
wrong way.
This patchset is based on top of the current master
c2d95729e3094ecdd8c54e856bbe971adbbd7f48 ("Merge branch 'akpm'
(patches from Andrew Morton)") and available in the following git
branch.
git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git review-sysfs-separate-out-ns
The series was lightly tested with normal and basic namespace
operations. diffstat follows.
drivers/base/class.c | 29 ++----
drivers/base/core.c | 8 +
drivers/gpio/gpiolib.c | 2
drivers/md/bitmap.c | 4
drivers/md/md.c | 2
drivers/md/md.h | 2
drivers/net/bonding/bond_sysfs.c | 14 ---
fs/sysfs/bin.c | 2
fs/sysfs/dir.c | 163 ++++++++++++++-------------------------
fs/sysfs/file.c | 105 ++++++-------------------
fs/sysfs/group.c | 29 +++---
fs/sysfs/inode.c | 6 -
fs/sysfs/mount.c | 24 +----
fs/sysfs/symlink.c | 49 +++--------
fs/sysfs/sysfs.h | 43 +++-------
include/linux/device.h | 24 ++++-
include/linux/kobject.h | 1
include/linux/netdevice.h | 16 +++
include/linux/sysfs.h | 88 ++++++++++++++-------
lib/kobject.c | 31 ++++++-
net/core/net-sysfs.c | 14 +--
21 files changed, 294 insertions(+), 362 deletions(-)
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/7] sysfs: drop semicolon from to_sysfs_dirent() definition
2013-09-12 2:29 [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs Tejun Heo
@ 2013-09-12 2:29 ` Tejun Heo
2013-09-12 2:29 ` [PATCH 2/7] sysfs: make attr namespace interface less convoluted Tejun Heo
` (7 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Tejun Heo @ 2013-09-12 2:29 UTC (permalink / raw)
To: gregkh; +Cc: linux-kernel, kay, ebiederm, netdev, lizefan, Tejun Heo
The expansion of to_sysfs_dirent() contains an unncessary trailing
semicolon making it impossible to use in the middle of statements.
Drop it.
Signed-off-by: Tejun Heo <tj@kernel.org>
---
fs/sysfs/dir.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 4d83ced..834c64c 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -28,7 +28,7 @@
DEFINE_MUTEX(sysfs_mutex);
DEFINE_SPINLOCK(sysfs_assoc_lock);
-#define to_sysfs_dirent(X) rb_entry((X), struct sysfs_dirent, s_rb);
+#define to_sysfs_dirent(X) rb_entry((X), struct sysfs_dirent, s_rb)
static DEFINE_SPINLOCK(sysfs_ino_lock);
static DEFINE_IDA(sysfs_ino_ida);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/7] sysfs: make attr namespace interface less convoluted
2013-09-12 2:29 [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs Tejun Heo
2013-09-12 2:29 ` [PATCH 1/7] sysfs: drop semicolon from to_sysfs_dirent() definition Tejun Heo
@ 2013-09-12 2:29 ` Tejun Heo
2013-09-12 19:46 ` David Miller
2013-09-12 2:29 ` [PATCH 3/7] sysfs: remove ktype->namespace() invocations in directory code Tejun Heo
` (6 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Tejun Heo @ 2013-09-12 2:29 UTC (permalink / raw)
To: gregkh; +Cc: linux-kernel, kay, ebiederm, netdev, lizefan, Tejun Heo
sysfs ns (namespace) implementation became more convoluted than
necessary while trying to hide ns information from visible interface.
The relatively recent attr ns support is a good example.
* attr ns tag is determined by sysfs_ops->namespace() callback while
dir tag is determined by kobj_type->namespace(). The placement is
arbitrary.
* Instead of performing operations with explicit ns tag, the namespace
callback is routed through sysfs_attr_ns(), sysfs_ops->namespace(),
class_attr_namespace(), class_attr->namespace(). It's not simpler
in any sense. The only thing this convolution does is traversing
the whole stack backwards.
The namespace callbacks are unncessary because the operations involved
are inherently synchronous. The information can be provided in in
straight-forward top-down direction and reversing that direction is
unnecessary and against basic design principles.
This backward interface is unnecessarily convoluted and hinders
properly separating out sysfs from driver model / kobject for proper
layering. This patch updates attr ns support such that
* sysfs_ops->namespace() and class_attr->namespace() are dropped.
* sysfs_{create|remove}_file_ns(), which take explicit @ns param, are
added and sysfs_{create|remove}_file() are now simple wrappers
around the ns aware functions.
* ns handling is dropped from sysfs_chmod_file(). Nobody uses it at
this point. sysfs_chmod_file_ns() can be added later if necessary.
* Explicit @ns is propagated through class_{create|remove}_file_ns()
and netdev_class_{create|remove}_file_ns().
* driver/net/bonding which is currently the only user of attr
namespace is updated to use netdev_class_{create|remove}_file_ns()
with @bh->net as the ns tag instead of using the namespace callback.
This patch should be an equivalent conversion without any functional
difference. It makes the code easier to follow, reduces lines of code
a bit and helps proper separation and layering.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Kay Sievers <kay@vrfy.org>
---
drivers/base/class.c | 29 ++++--------
drivers/net/bonding/bond_sysfs.c | 14 ++----
fs/sysfs/file.c | 95 ++++++++++------------------------------
fs/sysfs/group.c | 7 +--
fs/sysfs/sysfs.h | 5 ++-
include/linux/device.h | 24 +++++++---
include/linux/netdevice.h | 16 ++++++-
include/linux/sysfs.h | 31 +++++++++----
net/core/net-sysfs.c | 14 +++---
9 files changed, 106 insertions(+), 129 deletions(-)
diff --git a/drivers/base/class.c b/drivers/base/class.c
index 8b7818b..f96f704 100644
--- a/drivers/base/class.c
+++ b/drivers/base/class.c
@@ -47,18 +47,6 @@ static ssize_t class_attr_store(struct kobject *kobj, struct attribute *attr,
return ret;
}
-static const void *class_attr_namespace(struct kobject *kobj,
- const struct attribute *attr)
-{
- struct class_attribute *class_attr = to_class_attr(attr);
- struct subsys_private *cp = to_subsys_private(kobj);
- const void *ns = NULL;
-
- if (class_attr->namespace)
- ns = class_attr->namespace(cp->class, class_attr);
- return ns;
-}
-
static void class_release(struct kobject *kobj)
{
struct subsys_private *cp = to_subsys_private(kobj);
@@ -86,7 +74,6 @@ static const struct kobj_ns_type_operations *class_child_ns_type(struct kobject
static const struct sysfs_ops class_sysfs_ops = {
.show = class_attr_show,
.store = class_attr_store,
- .namespace = class_attr_namespace,
};
static struct kobj_type class_ktype = {
@@ -99,21 +86,23 @@ static struct kobj_type class_ktype = {
static struct kset *class_kset;
-int class_create_file(struct class *cls, const struct class_attribute *attr)
+int class_create_file_ns(struct class *cls, const struct class_attribute *attr,
+ const void *ns)
{
int error;
if (cls)
- error = sysfs_create_file(&cls->p->subsys.kobj,
- &attr->attr);
+ error = sysfs_create_file_ns(&cls->p->subsys.kobj,
+ &attr->attr, ns);
else
error = -EINVAL;
return error;
}
-void class_remove_file(struct class *cls, const struct class_attribute *attr)
+void class_remove_file_ns(struct class *cls, const struct class_attribute *attr,
+ const void *ns)
{
if (cls)
- sysfs_remove_file(&cls->p->subsys.kobj, &attr->attr);
+ sysfs_remove_file_ns(&cls->p->subsys.kobj, &attr->attr, ns);
}
static struct class *class_get(struct class *cls)
@@ -600,8 +589,8 @@ int __init classes_init(void)
return 0;
}
-EXPORT_SYMBOL_GPL(class_create_file);
-EXPORT_SYMBOL_GPL(class_remove_file);
+EXPORT_SYMBOL_GPL(class_create_file_ns);
+EXPORT_SYMBOL_GPL(class_remove_file_ns);
EXPORT_SYMBOL_GPL(class_unregister);
EXPORT_SYMBOL_GPL(class_destroy);
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index eeab40b..72ad0a9 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -149,14 +149,6 @@ err_no_cmd:
return -EPERM;
}
-static const void *bonding_namespace(struct class *cls,
- const struct class_attribute *attr)
-{
- const struct bond_net *bn =
- container_of(attr, struct bond_net, class_attr_bonding_masters);
- return bn->net;
-}
-
/* class attribute for bond_masters file. This ends up in /sys/class/net */
static const struct class_attribute class_attr_bonding_masters = {
.attr = {
@@ -165,7 +157,6 @@ static const struct class_attribute class_attr_bonding_masters = {
},
.show = bonding_show_bonds,
.store = bonding_store_bonds,
- .namespace = bonding_namespace,
};
int bond_create_slave_symlinks(struct net_device *master,
@@ -1748,7 +1739,8 @@ int bond_create_sysfs(struct bond_net *bn)
bn->class_attr_bonding_masters = class_attr_bonding_masters;
sysfs_attr_init(&bn->class_attr_bonding_masters.attr);
- ret = netdev_class_create_file(&bn->class_attr_bonding_masters);
+ ret = netdev_class_create_file_ns(&bn->class_attr_bonding_masters,
+ bn->net);
/*
* Permit multiple loads of the module by ignoring failures to
* create the bonding_masters sysfs file. Bonding devices
@@ -1778,7 +1770,7 @@ int bond_create_sysfs(struct bond_net *bn)
*/
void bond_destroy_sysfs(struct bond_net *bn)
{
- netdev_class_remove_file(&bn->class_attr_bonding_masters);
+ netdev_class_remove_file_ns(&bn->class_attr_bonding_masters, bn->net);
}
/*
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 15ef5eb..e784340 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -485,58 +485,15 @@ const struct file_operations sysfs_file_operations = {
.poll = sysfs_poll,
};
-static int sysfs_attr_ns(struct kobject *kobj, const struct attribute *attr,
- const void **pns)
-{
- struct sysfs_dirent *dir_sd = kobj->sd;
- const struct sysfs_ops *ops;
- const void *ns = NULL;
- int err;
-
- if (!dir_sd) {
- WARN(1, KERN_ERR "sysfs: kobject %s without dirent\n",
- kobject_name(kobj));
- return -ENOENT;
- }
-
- err = 0;
- if (!sysfs_ns_type(dir_sd))
- goto out;
-
- err = -EINVAL;
- if (!kobj->ktype)
- goto out;
- ops = kobj->ktype->sysfs_ops;
- if (!ops)
- goto out;
- if (!ops->namespace)
- goto out;
-
- err = 0;
- ns = ops->namespace(kobj, attr);
-out:
- if (err) {
- WARN(1, KERN_ERR
- "missing sysfs namespace attribute operation for kobject: %s\n",
- kobject_name(kobj));
- }
- *pns = ns;
- return err;
-}
-
-int sysfs_add_file_mode(struct sysfs_dirent *dir_sd,
- const struct attribute *attr, int type, umode_t amode)
+int sysfs_add_file_mode_ns(struct sysfs_dirent *dir_sd,
+ const struct attribute *attr, int type,
+ umode_t amode, const void *ns)
{
umode_t mode = (amode & S_IALLUGO) | S_IFREG;
struct sysfs_addrm_cxt acxt;
struct sysfs_dirent *sd;
- const void *ns;
int rc;
- rc = sysfs_attr_ns(dir_sd->s_dir.kobj, attr, &ns);
- if (rc)
- return rc;
-
sd = sysfs_new_dirent(attr->name, mode, type);
if (!sd)
return -ENOMEM;
@@ -559,23 +516,25 @@ int sysfs_add_file_mode(struct sysfs_dirent *dir_sd,
int sysfs_add_file(struct sysfs_dirent *dir_sd, const struct attribute *attr,
int type)
{
- return sysfs_add_file_mode(dir_sd, attr, type, attr->mode);
+ return sysfs_add_file_mode_ns(dir_sd, attr, type, attr->mode, NULL);
}
-
/**
- * sysfs_create_file - create an attribute file for an object.
- * @kobj: object we're creating for.
- * @attr: attribute descriptor.
+ * sysfs_create_file_ns - create an attribute file for an object with custom ns
+ * @kobj: object we're creating for
+ * @attr: attribute descriptor
+ * @ns: namespace the new file should belong to
*/
-int sysfs_create_file(struct kobject *kobj, const struct attribute *attr)
+int sysfs_create_file_ns(struct kobject *kobj, const struct attribute *attr,
+ const void *ns)
{
BUG_ON(!kobj || !kobj->sd || !attr);
- return sysfs_add_file(kobj->sd, attr, SYSFS_KOBJ_ATTR);
+ return sysfs_add_file_mode_ns(kobj->sd, attr, SYSFS_KOBJ_ATTR,
+ attr->mode, ns);
}
-EXPORT_SYMBOL_GPL(sysfs_create_file);
+EXPORT_SYMBOL_GPL(sysfs_create_file_ns);
int sysfs_create_files(struct kobject *kobj, const struct attribute **ptr)
{
@@ -630,17 +589,12 @@ int sysfs_chmod_file(struct kobject *kobj, const struct attribute *attr,
{
struct sysfs_dirent *sd;
struct iattr newattrs;
- const void *ns;
int rc;
- rc = sysfs_attr_ns(kobj, attr, &ns);
- if (rc)
- return rc;
-
mutex_lock(&sysfs_mutex);
rc = -ENOENT;
- sd = sysfs_find_dirent(kobj->sd, ns, attr->name);
+ sd = sysfs_find_dirent(kobj->sd, NULL, attr->name);
if (!sd)
goto out;
@@ -655,22 +609,21 @@ int sysfs_chmod_file(struct kobject *kobj, const struct attribute *attr,
EXPORT_SYMBOL_GPL(sysfs_chmod_file);
/**
- * sysfs_remove_file - remove an object attribute.
- * @kobj: object we're acting for.
- * @attr: attribute descriptor.
+ * sysfs_remove_file_ns - remove an object attribute with a custom ns tag
+ * @kobj: object we're acting for
+ * @attr: attribute descriptor
+ * @ns: namespace tag of the file to remove
*
- * Hash the attribute name and kill the victim.
+ * Hash the attribute name and namespace tag and kill the victim.
*/
-void sysfs_remove_file(struct kobject *kobj, const struct attribute *attr)
+void sysfs_remove_file_ns(struct kobject *kobj, const struct attribute *attr,
+ const void *ns)
{
- const void *ns;
-
- if (sysfs_attr_ns(kobj, attr, &ns))
- return;
+ struct sysfs_dirent *dir_sd = kobj->sd;
- sysfs_hash_and_remove(kobj->sd, ns, attr->name);
+ sysfs_hash_and_remove(dir_sd, ns, attr->name);
}
-EXPORT_SYMBOL_GPL(sysfs_remove_file);
+EXPORT_SYMBOL_GPL(sysfs_remove_file_ns);
void sysfs_remove_files(struct kobject *kobj, const struct attribute **ptr)
{
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 5f92cd2..25c78f2 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -56,9 +56,10 @@ static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
if (!mode)
continue;
}
- error = sysfs_add_file_mode(dir_sd, *attr,
- SYSFS_KOBJ_ATTR,
- (*attr)->mode | mode);
+ error = sysfs_add_file_mode_ns(dir_sd, *attr,
+ SYSFS_KOBJ_ATTR,
+ (*attr)->mode | mode,
+ NULL);
if (unlikely(error))
break;
}
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index b6deca3..a96da25 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -230,8 +230,9 @@ extern const struct file_operations sysfs_file_operations;
int sysfs_add_file(struct sysfs_dirent *dir_sd,
const struct attribute *attr, int type);
-int sysfs_add_file_mode(struct sysfs_dirent *dir_sd,
- const struct attribute *attr, int type, umode_t amode);
+int sysfs_add_file_mode_ns(struct sysfs_dirent *dir_sd,
+ const struct attribute *attr, int type,
+ umode_t amode, const void *ns);
/*
* bin.c
*/
diff --git a/include/linux/device.h b/include/linux/device.h
index 2a9d6ed..ce690ea 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -427,8 +427,6 @@ struct class_attribute {
char *buf);
ssize_t (*store)(struct class *class, struct class_attribute *attr,
const char *buf, size_t count);
- const void *(*namespace)(struct class *class,
- const struct class_attribute *attr);
};
#define CLASS_ATTR(_name, _mode, _show, _store) \
@@ -438,10 +436,24 @@ struct class_attribute {
#define CLASS_ATTR_RO(_name) \
struct class_attribute class_attr_##_name = __ATTR_RO(_name)
-extern int __must_check class_create_file(struct class *class,
- const struct class_attribute *attr);
-extern void class_remove_file(struct class *class,
- const struct class_attribute *attr);
+extern int __must_check class_create_file_ns(struct class *class,
+ const struct class_attribute *attr,
+ const void *ns);
+extern void class_remove_file_ns(struct class *class,
+ const struct class_attribute *attr,
+ const void *ns);
+
+static inline int __must_check class_create_file(struct class *class,
+ const struct class_attribute *attr)
+{
+ return class_create_file_ns(class, attr, NULL);
+}
+
+static inline void class_remove_file(struct class *class,
+ const struct class_attribute *attr)
+{
+ return class_remove_file_ns(class, attr, NULL);
+}
/* Simple class attribute that is just a static string */
struct class_attribute_string {
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 041b42a..d551624 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2873,8 +2873,20 @@ extern int __init dev_proc_init(void);
#define dev_proc_init() 0
#endif
-extern int netdev_class_create_file(struct class_attribute *class_attr);
-extern void netdev_class_remove_file(struct class_attribute *class_attr);
+extern int netdev_class_create_file_ns(struct class_attribute *class_attr,
+ const void *ns);
+extern void netdev_class_remove_file_ns(struct class_attribute *class_attr,
+ const void *ns);
+
+static inline int netdev_class_create_file(struct class_attribute *class_attr)
+{
+ return netdev_class_create_file_ns(class_attr, NULL);
+}
+
+static inline void netdev_class_remove_file(struct class_attribute *class_attr)
+{
+ netdev_class_remove_file_ns(class_attr, NULL);
+}
extern struct kobj_ns_type_operations net_ns_type_operations;
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 11baec7..82f7fac 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -173,7 +173,6 @@ struct bin_attribute bin_attr_##_name = __BIN_ATTR_RW(_name, _size)
struct sysfs_ops {
ssize_t (*show)(struct kobject *, struct attribute *, char *);
ssize_t (*store)(struct kobject *, struct attribute *, const char *, size_t);
- const void *(*namespace)(struct kobject *, const struct attribute *);
};
struct sysfs_dirent;
@@ -189,13 +188,15 @@ int __must_check sysfs_rename_dir(struct kobject *kobj, const char *new_name);
int __must_check sysfs_move_dir(struct kobject *kobj,
struct kobject *new_parent_kobj);
-int __must_check sysfs_create_file(struct kobject *kobj,
- const struct attribute *attr);
+int __must_check sysfs_create_file_ns(struct kobject *kobj,
+ const struct attribute *attr,
+ const void *ns);
int __must_check sysfs_create_files(struct kobject *kobj,
const struct attribute **attr);
int __must_check sysfs_chmod_file(struct kobject *kobj,
const struct attribute *attr, umode_t mode);
-void sysfs_remove_file(struct kobject *kobj, const struct attribute *attr);
+void sysfs_remove_file_ns(struct kobject *kobj, const struct attribute *attr,
+ const void *ns);
void sysfs_remove_files(struct kobject *kobj, const struct attribute **attr);
int __must_check sysfs_create_bin_file(struct kobject *kobj,
@@ -277,8 +278,9 @@ static inline int sysfs_move_dir(struct kobject *kobj,
return 0;
}
-static inline int sysfs_create_file(struct kobject *kobj,
- const struct attribute *attr)
+static inline int sysfs_create_file_ns(struct kobject *kobj,
+ const struct attribute *attr,
+ const void *ns)
{
return 0;
}
@@ -295,8 +297,9 @@ static inline int sysfs_chmod_file(struct kobject *kobj,
return 0;
}
-static inline void sysfs_remove_file(struct kobject *kobj,
- const struct attribute *attr)
+static inline void sysfs_remove_file_ns(struct kobject *kobj,
+ const struct attribute *attr,
+ const void *ns)
{
}
@@ -435,4 +438,16 @@ static inline int __must_check sysfs_init(void)
#endif /* CONFIG_SYSFS */
+static inline int __must_check sysfs_create_file(struct kobject *kobj,
+ const struct attribute *attr)
+{
+ return sysfs_create_file_ns(kobj, attr, NULL);
+}
+
+static inline void sysfs_remove_file(struct kobject *kobj,
+ const struct attribute *attr)
+{
+ return sysfs_remove_file_ns(kobj, attr, NULL);
+}
+
#endif /* _SYSFS_H_ */
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index d954b56..325dee8 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -1344,17 +1344,19 @@ int netdev_register_kobject(struct net_device *net)
return error;
}
-int netdev_class_create_file(struct class_attribute *class_attr)
+int netdev_class_create_file_ns(struct class_attribute *class_attr,
+ const void *ns)
{
- return class_create_file(&net_class, class_attr);
+ return class_create_file_ns(&net_class, class_attr, ns);
}
-EXPORT_SYMBOL(netdev_class_create_file);
+EXPORT_SYMBOL(netdev_class_create_file_ns);
-void netdev_class_remove_file(struct class_attribute *class_attr)
+void netdev_class_remove_file_ns(struct class_attribute *class_attr,
+ const void *ns)
{
- class_remove_file(&net_class, class_attr);
+ class_remove_file_ns(&net_class, class_attr, ns);
}
-EXPORT_SYMBOL(netdev_class_remove_file);
+EXPORT_SYMBOL(netdev_class_remove_file_ns);
int netdev_kobject_init(void)
{
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 3/7] sysfs: remove ktype->namespace() invocations in directory code
2013-09-12 2:29 [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs Tejun Heo
2013-09-12 2:29 ` [PATCH 1/7] sysfs: drop semicolon from to_sysfs_dirent() definition Tejun Heo
2013-09-12 2:29 ` [PATCH 2/7] sysfs: make attr namespace interface less convoluted Tejun Heo
@ 2013-09-12 2:29 ` Tejun Heo
2013-09-12 2:29 ` [PATCH 4/7] sysfs: remove ktype->namespace() invocations in symlink code Tejun Heo
` (5 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Tejun Heo @ 2013-09-12 2:29 UTC (permalink / raw)
To: gregkh; +Cc: linux-kernel, kay, ebiederm, netdev, lizefan, Tejun Heo
For some unrecognizable reason, namespace information is communicated
to sysfs through ktype->namespace() callback when there's *nothing*
which needs the use of a callback. The whole sequence of operations
is completely synchronous and sysfs operations simply end up calling
back into the layer which just invoked it in order to find out the
namespace information, which is completely backwards, obfuscates
what's going on and unnecessarily tangles two separate layers.
This patch doesn't remove ktype->namespace() but shifts its handling
to kobject layer. We probably want to get rid of the callback in the
long term.
This patch adds an explicit param to sysfs_{create|rename|move}_dir()
and renames them to sysfs_{create|rename|move}_dir_ns(), respectively.
ktype->namespace() invocations are moved to the calling sites of the
above functions. A new helper kboject_namespace() is introduced which
directly tests kobj_ns_type_operations->type which should give the
same result as testing sysfs_fs_type(parent_sd) and returns @kobj's
namespace tag as necessary. kobject_namespace() is extern as it will
be used from another file in the following patches.
This patch should be an equivalent conversion without any functional
difference.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Kay Sievers <kay@vrfy.org>
---
fs/sysfs/dir.c | 23 ++++++++---------------
include/linux/kobject.h | 1 +
include/linux/sysfs.h | 20 ++++++++++++--------
lib/kobject.c | 28 ++++++++++++++++++++++++----
4 files changed, 45 insertions(+), 27 deletions(-)
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 834c64c..878ac3a 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -730,14 +730,14 @@ static enum kobj_ns_type sysfs_read_ns_type(struct kobject *kobj)
}
/**
- * sysfs_create_dir - create a directory for an object.
- * @kobj: object we're creating directory for.
+ * sysfs_create_dir_ns - create a directory for an object with a namespace tag
+ * @kobj: object we're creating directory for
+ * @ns: the namespace tag to use
*/
-int sysfs_create_dir(struct kobject *kobj)
+int sysfs_create_dir_ns(struct kobject *kobj, const void *ns)
{
enum kobj_ns_type type;
struct sysfs_dirent *parent_sd, *sd;
- const void *ns = NULL;
int error = 0;
BUG_ON(!kobj);
@@ -750,8 +750,6 @@ int sysfs_create_dir(struct kobject *kobj)
if (!parent_sd)
return -ENOENT;
- if (sysfs_ns_type(parent_sd))
- ns = kobj->ktype->namespace(kobj);
type = sysfs_read_ns_type(kobj);
error = create_dir(kobj, parent_sd, type, ns, kobject_name(kobj), &sd);
@@ -909,26 +907,21 @@ int sysfs_rename(struct sysfs_dirent *sd,
return error;
}
-int sysfs_rename_dir(struct kobject *kobj, const char *new_name)
+int sysfs_rename_dir_ns(struct kobject *kobj, const char *new_name,
+ const void *new_ns)
{
struct sysfs_dirent *parent_sd = kobj->sd->s_parent;
- const void *new_ns = NULL;
-
- if (sysfs_ns_type(parent_sd))
- new_ns = kobj->ktype->namespace(kobj);
return sysfs_rename(kobj->sd, parent_sd, new_ns, new_name);
}
-int sysfs_move_dir(struct kobject *kobj, struct kobject *new_parent_kobj)
+int sysfs_move_dir_ns(struct kobject *kobj, struct kobject *new_parent_kobj,
+ const void *new_ns)
{
struct sysfs_dirent *sd = kobj->sd;
struct sysfs_dirent *new_parent_sd;
- const void *new_ns = NULL;
BUG_ON(!sd->s_parent);
- if (sysfs_ns_type(sd->s_parent))
- new_ns = kobj->ktype->namespace(kobj);
new_parent_sd = new_parent_kobj && new_parent_kobj->sd ?
new_parent_kobj->sd : &sysfs_root;
diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index de6dcbcc..e7ba650 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -107,6 +107,7 @@ extern int __must_check kobject_move(struct kobject *, struct kobject *);
extern struct kobject *kobject_get(struct kobject *kobj);
extern void kobject_put(struct kobject *kobj);
+extern const void *kobject_namespace(struct kobject *kobj);
extern char *kobject_get_path(struct kobject *kobj, gfp_t flag);
struct kobj_type {
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 82f7fac..7f56bad 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -182,11 +182,13 @@ struct sysfs_dirent;
int sysfs_schedule_callback(struct kobject *kobj, void (*func)(void *),
void *data, struct module *owner);
-int __must_check sysfs_create_dir(struct kobject *kobj);
+int __must_check sysfs_create_dir_ns(struct kobject *kobj, const void *ns);
void sysfs_remove_dir(struct kobject *kobj);
-int __must_check sysfs_rename_dir(struct kobject *kobj, const char *new_name);
-int __must_check sysfs_move_dir(struct kobject *kobj,
- struct kobject *new_parent_kobj);
+int __must_check sysfs_rename_dir_ns(struct kobject *kobj, const char *new_name,
+ const void *new_ns);
+int __must_check sysfs_move_dir_ns(struct kobject *kobj,
+ struct kobject *new_parent_kobj,
+ const void *new_ns);
int __must_check sysfs_create_file_ns(struct kobject *kobj,
const struct attribute *attr,
@@ -258,7 +260,7 @@ static inline int sysfs_schedule_callback(struct kobject *kobj,
return -ENOSYS;
}
-static inline int sysfs_create_dir(struct kobject *kobj)
+static inline int sysfs_create_dir_ns(struct kobject *kobj, const void *ns)
{
return 0;
}
@@ -267,13 +269,15 @@ static inline void sysfs_remove_dir(struct kobject *kobj)
{
}
-static inline int sysfs_rename_dir(struct kobject *kobj, const char *new_name)
+static inline int sysfs_rename_dir_ns(struct kobject *kobj,
+ const char *new_name, const void *new_ns)
{
return 0;
}
-static inline int sysfs_move_dir(struct kobject *kobj,
- struct kobject *new_parent_kobj)
+static inline int sysfs_move_dir_ns(struct kobject *kobj,
+ struct kobject *new_parent_kobj,
+ const void *new_ns)
{
return 0;
}
diff --git a/lib/kobject.c b/lib/kobject.c
index 9621751..85fb3a1 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -18,6 +18,24 @@
#include <linux/stat.h>
#include <linux/slab.h>
+/**
+ * kobject_namespace - return @kobj's namespace tag
+ * @kobj: kobject in question
+ *
+ * Returns namespace tag of @kobj if its parent has namespace ops enabled
+ * and thus @kobj should have a namespace tag associated with it. Returns
+ * %NULL otherwise.
+ */
+const void *kobject_namespace(struct kobject *kobj)
+{
+ const struct kobj_ns_type_operations *ns_ops = kobj_ns_ops(kobj);
+
+ if (!ns_ops || ns_ops->type == KOBJ_NS_TYPE_NONE)
+ return NULL;
+
+ return kobj->ktype->namespace(kobj);
+}
+
/*
* populate_dir - populate directory with attributes.
* @kobj: object we're working on.
@@ -46,8 +64,9 @@ static int populate_dir(struct kobject *kobj)
static int create_dir(struct kobject *kobj)
{
- int error = 0;
- error = sysfs_create_dir(kobj);
+ int error;
+
+ error = sysfs_create_dir_ns(kobj, kobject_namespace(kobj));
if (!error) {
error = populate_dir(kobj);
if (error)
@@ -428,7 +447,7 @@ int kobject_rename(struct kobject *kobj, const char *new_name)
goto out;
}
- error = sysfs_rename_dir(kobj, new_name);
+ error = sysfs_rename_dir_ns(kobj, new_name, kobject_namespace(kobj));
if (error)
goto out;
@@ -472,6 +491,7 @@ int kobject_move(struct kobject *kobj, struct kobject *new_parent)
if (kobj->kset)
new_parent = kobject_get(&kobj->kset->kobj);
}
+
/* old object path */
devpath = kobject_get_path(kobj, GFP_KERNEL);
if (!devpath) {
@@ -486,7 +506,7 @@ int kobject_move(struct kobject *kobj, struct kobject *new_parent)
sprintf(devpath_string, "DEVPATH_OLD=%s", devpath);
envp[0] = devpath_string;
envp[1] = NULL;
- error = sysfs_move_dir(kobj, new_parent);
+ error = sysfs_move_dir_ns(kobj, new_parent, kobject_namespace(kobj));
if (error)
goto out;
old_parent = kobj->parent;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 4/7] sysfs: remove ktype->namespace() invocations in symlink code
2013-09-12 2:29 [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs Tejun Heo
` (2 preceding siblings ...)
2013-09-12 2:29 ` [PATCH 3/7] sysfs: remove ktype->namespace() invocations in directory code Tejun Heo
@ 2013-09-12 2:29 ` Tejun Heo
2013-09-12 2:29 ` [PATCH 5/7] sysfs: drop kobj_ns_type handling Tejun Heo
` (4 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Tejun Heo @ 2013-09-12 2:29 UTC (permalink / raw)
To: gregkh; +Cc: linux-kernel, kay, ebiederm, netdev, lizefan, Tejun Heo
There's no reason for sysfs to be calling ktype->namespace(). It is
backwards, obfuscates what's going on and unnecessarily tangles two
separate layers.
There are two places where symlink code calls ktype->namespace().
* sysfs_do_create_link_sd() calls it to find out the namespace tag of
the target directory. Unless symlinking races with cross-namespace
renaming, this equals @target_sd->s_ns.
* sysfs_rename_link() uses it to find out the new namespace to rename
to and the new namespace can be different from the existing one.
The function is renamed to sysfs_rename_link_ns() with an explicit
@ns argument and the ktype->namespace() invocation is shifted to the
device layer.
While this patch replaces ktype->namespace() invocation with the
recorded result in @target_sd, this shouldn't result in any behvior
difference.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Kay Sievers <kay@vrfy.org>
---
drivers/base/core.c | 8 +++++---
fs/sysfs/symlink.c | 16 +++++++---------
include/linux/sysfs.h | 16 ++++++++++++----
3 files changed, 24 insertions(+), 16 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index c7cfadc..3335000 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1881,6 +1881,7 @@ EXPORT_SYMBOL_GPL(device_destroy);
*/
int device_rename(struct device *dev, const char *new_name)
{
+ struct kobject *kobj = &dev->kobj;
char *old_device_name = NULL;
int error;
@@ -1898,13 +1899,14 @@ int device_rename(struct device *dev, const char *new_name)
}
if (dev->class) {
- error = sysfs_rename_link(&dev->class->p->subsys.kobj,
- &dev->kobj, old_device_name, new_name);
+ error = sysfs_rename_link_ns(&dev->class->p->subsys.kobj,
+ kobj, old_device_name,
+ new_name, kobject_namespace(kobj));
if (error)
goto out;
}
- error = kobject_rename(&dev->kobj, new_name);
+ error = kobject_rename(kobj, new_name);
if (error)
goto out;
diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index 2dd4507..12d58ad 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -52,7 +52,7 @@ static int sysfs_do_create_link_sd(struct sysfs_dirent *parent_sd,
ns_type = sysfs_ns_type(parent_sd);
if (ns_type)
- sd->s_ns = target->ktype->namespace(target);
+ sd->s_ns = target_sd->s_ns;
sd->s_symlink.target_sd = target_sd;
target_sd = NULL; /* reference is now owned by the symlink */
@@ -181,19 +181,20 @@ void sysfs_remove_link(struct kobject *kobj, const char *name)
EXPORT_SYMBOL_GPL(sysfs_remove_link);
/**
- * sysfs_rename_link - rename symlink in object's directory.
+ * sysfs_rename_link_ns - rename symlink in object's directory.
* @kobj: object we're acting for.
* @targ: object we're pointing to.
* @old: previous name of the symlink.
* @new: new name of the symlink.
+ * @new_ns: new namespace of the symlink.
*
* A helper function for the common rename symlink idiom.
*/
-int sysfs_rename_link(struct kobject *kobj, struct kobject *targ,
- const char *old, const char *new)
+int sysfs_rename_link_ns(struct kobject *kobj, struct kobject *targ,
+ const char *old, const char *new, const void *new_ns)
{
struct sysfs_dirent *parent_sd, *sd = NULL;
- const void *old_ns = NULL, *new_ns = NULL;
+ const void *old_ns = NULL;
int result;
if (!kobj)
@@ -215,16 +216,13 @@ int sysfs_rename_link(struct kobject *kobj, struct kobject *targ,
if (sd->s_symlink.target_sd->s_dir.kobj != targ)
goto out;
- if (sysfs_ns_type(parent_sd))
- new_ns = targ->ktype->namespace(targ);
-
result = sysfs_rename(sd, parent_sd, new_ns, new);
out:
sysfs_put(sd);
return result;
}
-EXPORT_SYMBOL_GPL(sysfs_rename_link);
+EXPORT_SYMBOL_GPL(sysfs_rename_link_ns);
static int sysfs_get_target_path(struct sysfs_dirent *parent_sd,
struct sysfs_dirent *target_sd, char *path)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 7f56bad..c792f73 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -213,8 +213,9 @@ int __must_check sysfs_create_link_nowarn(struct kobject *kobj,
const char *name);
void sysfs_remove_link(struct kobject *kobj, const char *name);
-int sysfs_rename_link(struct kobject *kobj, struct kobject *target,
- const char *old_name, const char *new_name);
+int sysfs_rename_link_ns(struct kobject *kobj, struct kobject *target,
+ const char *old_name, const char *new_name,
+ const void *new_ns);
void sysfs_delete_link(struct kobject *dir, struct kobject *targ,
const char *name);
@@ -340,8 +341,9 @@ static inline void sysfs_remove_link(struct kobject *kobj, const char *name)
{
}
-static inline int sysfs_rename_link(struct kobject *k, struct kobject *t,
- const char *old_name, const char *new_name)
+static inline int sysfs_rename_link_ns(struct kobject *k, struct kobject *t,
+ const char *old_name,
+ const char *new_name, const void *ns)
{
return 0;
}
@@ -454,4 +456,10 @@ static inline void sysfs_remove_file(struct kobject *kobj,
return sysfs_remove_file_ns(kobj, attr, NULL);
}
+static inline int sysfs_rename_link(struct kobject *kobj, struct kobject *target,
+ const char *old_name, const char *new_name)
+{
+ return sysfs_rename_link_ns(kobj, target, old_name, new_name, NULL);
+}
+
#endif /* _SYSFS_H_ */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 5/7] sysfs: drop kobj_ns_type handling
2013-09-12 2:29 [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs Tejun Heo
` (3 preceding siblings ...)
2013-09-12 2:29 ` [PATCH 4/7] sysfs: remove ktype->namespace() invocations in symlink code Tejun Heo
@ 2013-09-12 2:29 ` Tejun Heo
2013-09-12 2:29 ` [PATCH 6/7] sysfs: clean up sysfs_get_dirent() Tejun Heo
` (3 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Tejun Heo @ 2013-09-12 2:29 UTC (permalink / raw)
To: gregkh; +Cc: linux-kernel, kay, ebiederm, netdev, lizefan, Tejun Heo
The way namespace tags are implemented in sysfs is more complicated
than necessary. As each tag is a pointer value and required to be
non-NULL under a namespace enabled parent, there's no need to record
separately what type each tag is or where namespace is enabled.
If multiple namespace types are needed, which currently aren't, we can
simply compare the tag to a set of allowed tags in the superblock
assuming that the tags, being pointers, won't have the same value
across multiple types. Also, whether to filter by namespace tag or
not can be trivially determined by whether the node has any tagged
children or not.
This patch rips out kobj_ns_type handling from sysfs. sysfs no longer
cares whether specific type of namespace is enabled or not. If a
sysfs_dirent has a non-NULL tag, the parent is marked as needing
namespace filtering and the value is tested against the allowed set of
tags for the superblock (currently only one but increasing this number
isn't difficult) and the sysfs_dirent is ignored if it doesn't match.
This removes most kobject namespace knowledge from sysfs proper which
will enable proper separation and layering of sysfs. The namespace
sanity checks in fs/sysfs/dir.c are replaced by the new sanity check
in kobject_namespace(). As this is the only place ktype->namespace()
is called for sysfs, this doesn't weaken the sanity check
significantly. I omitted converting the sanity check in
sysfs_do_create_link_sd(). While the check can be shifted to upper
layer, mistakes there are well contained and should be easily visible
anyway.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Kay Sievers <kay@vrfy.org>
---
fs/sysfs/dir.c | 90 +++++++++++++++---------------------------------------
fs/sysfs/mount.c | 24 ++++-----------
fs/sysfs/symlink.c | 27 ++++------------
fs/sysfs/sysfs.h | 25 +++++----------
lib/kobject.c | 5 ++-
5 files changed, 48 insertions(+), 123 deletions(-)
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 878ac3a..1dfb4aa 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -111,6 +111,11 @@ static int sysfs_link_sibling(struct sysfs_dirent *sd)
/* add new node and rebalance the tree */
rb_link_node(&sd->s_rb, parent, node);
rb_insert_color(&sd->s_rb, &sd->s_parent->s_dir.children);
+
+ /* if @sd has ns tag, mark the parent to enable ns filtering */
+ if (sd->s_ns)
+ sd->s_parent->s_flags |= SYSFS_FLAG_HAS_NS;
+
return 0;
}
@@ -130,6 +135,13 @@ static void sysfs_unlink_sibling(struct sysfs_dirent *sd)
sd->s_parent->s_dir.subdirs--;
rb_erase(&sd->s_rb, &sd->s_parent->s_dir.children);
+
+ /*
+ * Either all or none of the children have tags. Clearing HAS_NS
+ * when there's no child left is enough to keep the flag synced.
+ */
+ if (RB_EMPTY_ROOT(&sd->s_parent->s_dir.children))
+ sd->s_parent->s_flags &= ~SYSFS_FLAG_HAS_NS;
}
#ifdef CONFIG_DEBUG_LOCK_ALLOC
@@ -297,7 +309,6 @@ static int sysfs_dentry_delete(const struct dentry *dentry)
static int sysfs_dentry_revalidate(struct dentry *dentry, unsigned int flags)
{
struct sysfs_dirent *sd;
- int type;
if (flags & LOOKUP_RCU)
return -ECHILD;
@@ -318,13 +329,8 @@ static int sysfs_dentry_revalidate(struct dentry *dentry, unsigned int flags)
goto out_bad;
/* The sysfs dirent has been moved to a different namespace */
- type = KOBJ_NS_TYPE_NONE;
- if (sd->s_parent) {
- type = sysfs_ns_type(sd->s_parent);
- if (type != KOBJ_NS_TYPE_NONE &&
- sysfs_info(dentry->d_sb)->ns[type] != sd->s_ns)
- goto out_bad;
- }
+ if (sd->s_ns && sd->s_ns != sysfs_info(dentry->d_sb)->ns)
+ goto out_bad;
mutex_unlock(&sysfs_mutex);
out_valid:
@@ -445,13 +451,6 @@ int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
struct sysfs_inode_attrs *ps_iattr;
int ret;
- if (!!sysfs_ns_type(acxt->parent_sd) != !!sd->s_ns) {
- WARN(1, KERN_WARNING "sysfs: ns %s in '%s' for '%s'\n",
- sysfs_ns_type(acxt->parent_sd) ? "required" : "invalid",
- acxt->parent_sd->s_name, sd->s_name);
- return -EINVAL;
- }
-
sd->s_hash = sysfs_name_hash(sd->s_ns, sd->s_name);
sd->s_parent = sysfs_get(acxt->parent_sd);
@@ -613,13 +612,6 @@ struct sysfs_dirent *sysfs_find_dirent(struct sysfs_dirent *parent_sd,
struct rb_node *node = parent_sd->s_dir.children.rb_node;
unsigned int hash;
- if (!!sysfs_ns_type(parent_sd) != !!ns) {
- WARN(1, KERN_WARNING "sysfs: ns %s in '%s' for '%s'\n",
- sysfs_ns_type(parent_sd) ? "required" : "invalid",
- parent_sd->s_name, name);
- return NULL;
- }
-
hash = sysfs_name_hash(ns, name);
while (node) {
struct sysfs_dirent *sd;
@@ -667,8 +659,7 @@ struct sysfs_dirent *sysfs_get_dirent(struct sysfs_dirent *parent_sd,
EXPORT_SYMBOL_GPL(sysfs_get_dirent);
static int create_dir(struct kobject *kobj, struct sysfs_dirent *parent_sd,
- enum kobj_ns_type type, const void *ns, const char *name,
- struct sysfs_dirent **p_sd)
+ const void *ns, const char *name, struct sysfs_dirent **p_sd)
{
umode_t mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
struct sysfs_addrm_cxt acxt;
@@ -680,7 +671,6 @@ static int create_dir(struct kobject *kobj, struct sysfs_dirent *parent_sd,
if (!sd)
return -ENOMEM;
- sd->s_flags |= (type << SYSFS_NS_TYPE_SHIFT);
sd->s_ns = ns;
sd->s_dir.kobj = kobj;
@@ -700,33 +690,7 @@ static int create_dir(struct kobject *kobj, struct sysfs_dirent *parent_sd,
int sysfs_create_subdir(struct kobject *kobj, const char *name,
struct sysfs_dirent **p_sd)
{
- return create_dir(kobj, kobj->sd,
- KOBJ_NS_TYPE_NONE, NULL, name, p_sd);
-}
-
-/**
- * sysfs_read_ns_type: return associated ns_type
- * @kobj: the kobject being queried
- *
- * Each kobject can be tagged with exactly one namespace type
- * (i.e. network or user). Return the ns_type associated with
- * this object if any
- */
-static enum kobj_ns_type sysfs_read_ns_type(struct kobject *kobj)
-{
- const struct kobj_ns_type_operations *ops;
- enum kobj_ns_type type;
-
- ops = kobj_child_ns_ops(kobj);
- if (!ops)
- return KOBJ_NS_TYPE_NONE;
-
- type = ops->type;
- BUG_ON(type <= KOBJ_NS_TYPE_NONE);
- BUG_ON(type >= KOBJ_NS_TYPES);
- BUG_ON(!kobj_ns_type_registered(type));
-
- return type;
+ return create_dir(kobj, kobj->sd, NULL, name, p_sd);
}
/**
@@ -736,7 +700,6 @@ static enum kobj_ns_type sysfs_read_ns_type(struct kobject *kobj)
*/
int sysfs_create_dir_ns(struct kobject *kobj, const void *ns)
{
- enum kobj_ns_type type;
struct sysfs_dirent *parent_sd, *sd;
int error = 0;
@@ -750,9 +713,7 @@ int sysfs_create_dir_ns(struct kobject *kobj, const void *ns)
if (!parent_sd)
return -ENOENT;
- type = sysfs_read_ns_type(kobj);
-
- error = create_dir(kobj, parent_sd, type, ns, kobject_name(kobj), &sd);
+ error = create_dir(kobj, parent_sd, ns, kobject_name(kobj), &sd);
if (!error)
kobj->sd = sd;
return error;
@@ -766,13 +727,12 @@ static struct dentry *sysfs_lookup(struct inode *dir, struct dentry *dentry,
struct sysfs_dirent *parent_sd = parent->d_fsdata;
struct sysfs_dirent *sd;
struct inode *inode;
- enum kobj_ns_type type;
- const void *ns;
+ const void *ns = NULL;
mutex_lock(&sysfs_mutex);
- type = sysfs_ns_type(parent_sd);
- ns = sysfs_info(dir->i_sb)->ns[type];
+ if (parent_sd->s_flags & SYSFS_FLAG_HAS_NS)
+ ns = sysfs_info(dir->i_sb)->ns;
sd = sysfs_find_dirent(parent_sd, ns, dentry->d_name.name);
@@ -995,15 +955,15 @@ static int sysfs_readdir(struct file *file, struct dir_context *ctx)
struct dentry *dentry = file->f_path.dentry;
struct sysfs_dirent *parent_sd = dentry->d_fsdata;
struct sysfs_dirent *pos = file->private_data;
- enum kobj_ns_type type;
- const void *ns;
-
- type = sysfs_ns_type(parent_sd);
- ns = sysfs_info(dentry->d_sb)->ns[type];
+ const void *ns = NULL;
if (!dir_emit_dots(file, ctx))
return 0;
mutex_lock(&sysfs_mutex);
+
+ if (parent_sd->s_flags & SYSFS_FLAG_HAS_NS)
+ ns = sysfs_info(dentry->d_sb)->ns;
+
for (pos = sysfs_dir_pos(ns, parent_sd, ctx->pos, pos);
pos;
pos = sysfs_dir_next_pos(ns, parent_sd, ctx->pos, pos)) {
diff --git a/fs/sysfs/mount.c b/fs/sysfs/mount.c
index 834ec2c..8c24bce 100644
--- a/fs/sysfs/mount.c
+++ b/fs/sysfs/mount.c
@@ -36,7 +36,7 @@ static const struct super_operations sysfs_ops = {
struct sysfs_dirent sysfs_root = {
.s_name = "",
.s_count = ATOMIC_INIT(1),
- .s_flags = SYSFS_DIR | (KOBJ_NS_TYPE_NONE << SYSFS_NS_TYPE_SHIFT),
+ .s_flags = SYSFS_DIR,
.s_mode = S_IFDIR | S_IRUGO | S_IXUGO,
.s_ino = 1,
};
@@ -77,14 +77,8 @@ static int sysfs_test_super(struct super_block *sb, void *data)
{
struct sysfs_super_info *sb_info = sysfs_info(sb);
struct sysfs_super_info *info = data;
- enum kobj_ns_type type;
- int found = 1;
- for (type = KOBJ_NS_TYPE_NONE; type < KOBJ_NS_TYPES; type++) {
- if (sb_info->ns[type] != info->ns[type])
- found = 0;
- }
- return found;
+ return sb_info->ns == info->ns;
}
static int sysfs_set_super(struct super_block *sb, void *data)
@@ -98,9 +92,7 @@ static int sysfs_set_super(struct super_block *sb, void *data)
static void free_sysfs_super_info(struct sysfs_super_info *info)
{
- int type;
- for (type = KOBJ_NS_TYPE_NONE; type < KOBJ_NS_TYPES; type++)
- kobj_ns_drop(type, info->ns[type]);
+ kobj_ns_drop(KOBJ_NS_TYPE_NET, info->ns);
kfree(info);
}
@@ -108,7 +100,6 @@ static struct dentry *sysfs_mount(struct file_system_type *fs_type,
int flags, const char *dev_name, void *data)
{
struct sysfs_super_info *info;
- enum kobj_ns_type type;
struct super_block *sb;
int error;
@@ -116,18 +107,15 @@ static struct dentry *sysfs_mount(struct file_system_type *fs_type,
if (!capable(CAP_SYS_ADMIN) && !fs_fully_visible(fs_type))
return ERR_PTR(-EPERM);
- for (type = KOBJ_NS_TYPE_NONE; type < KOBJ_NS_TYPES; type++) {
- if (!kobj_ns_current_may_mount(type))
- return ERR_PTR(-EPERM);
- }
+ if (!kobj_ns_current_may_mount(KOBJ_NS_TYPE_NET))
+ return ERR_PTR(-EPERM);
}
info = kzalloc(sizeof(*info), GFP_KERNEL);
if (!info)
return ERR_PTR(-ENOMEM);
- for (type = KOBJ_NS_TYPE_NONE; type < KOBJ_NS_TYPES; type++)
- info->ns[type] = kobj_ns_grab_current(type);
+ info->ns = kobj_ns_grab_current(KOBJ_NS_TYPE_NET);
sb = sget(fs_type, sysfs_test_super, sysfs_set_super, flags, info);
if (IS_ERR(sb) || sb->s_fs_info != info)
diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index 12d58ad..7d981ce 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -28,7 +28,6 @@ static int sysfs_do_create_link_sd(struct sysfs_dirent *parent_sd,
struct sysfs_dirent *target_sd = NULL;
struct sysfs_dirent *sd = NULL;
struct sysfs_addrm_cxt acxt;
- enum kobj_ns_type ns_type;
int error;
BUG_ON(!name || !parent_sd);
@@ -50,29 +49,15 @@ static int sysfs_do_create_link_sd(struct sysfs_dirent *parent_sd,
if (!sd)
goto out_put;
- ns_type = sysfs_ns_type(parent_sd);
- if (ns_type)
- sd->s_ns = target_sd->s_ns;
+ sd->s_ns = target_sd->s_ns;
sd->s_symlink.target_sd = target_sd;
target_sd = NULL; /* reference is now owned by the symlink */
sysfs_addrm_start(&acxt, parent_sd);
- /* Symlinks must be between directories with the same ns_type */
- if (!ns_type ||
- (ns_type == sysfs_ns_type(sd->s_symlink.target_sd->s_parent))) {
- if (warn)
- error = sysfs_add_one(&acxt, sd);
- else
- error = __sysfs_add_one(&acxt, sd);
- } else {
- error = -EINVAL;
- WARN(1, KERN_WARNING
- "sysfs: symlink across ns_types %s/%s -> %s/%s\n",
- parent_sd->s_name,
- sd->s_name,
- sd->s_symlink.target_sd->s_parent->s_name,
- sd->s_symlink.target_sd->s_name);
- }
+ if (warn)
+ error = sysfs_add_one(&acxt, sd);
+ else
+ error = __sysfs_add_one(&acxt, sd);
sysfs_addrm_finish(&acxt);
if (error)
@@ -156,7 +141,7 @@ void sysfs_delete_link(struct kobject *kobj, struct kobject *targ,
{
const void *ns = NULL;
spin_lock(&sysfs_assoc_lock);
- if (targ->sd && sysfs_ns_type(kobj->sd))
+ if (targ->sd)
ns = targ->sd->s_ns;
spin_unlock(&sysfs_assoc_lock);
sysfs_hash_and_remove(kobj->sd, ns, name);
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index a96da25..7664d1b 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -93,11 +93,8 @@ struct sysfs_dirent {
#define SYSFS_COPY_NAME (SYSFS_DIR | SYSFS_KOBJ_LINK)
#define SYSFS_ACTIVE_REF (SYSFS_KOBJ_ATTR | SYSFS_KOBJ_BIN_ATTR)
-/* identify any namespace tag on sysfs_dirents */
-#define SYSFS_NS_TYPE_MASK 0xf00
-#define SYSFS_NS_TYPE_SHIFT 8
-
-#define SYSFS_FLAG_MASK ~(SYSFS_NS_TYPE_MASK|SYSFS_TYPE_MASK)
+#define SYSFS_FLAG_MASK ~SYSFS_TYPE_MASK
+#define SYSFS_FLAG_HAS_NS 0x01000
#define SYSFS_FLAG_REMOVED 0x02000
static inline unsigned int sysfs_type(struct sysfs_dirent *sd)
@@ -105,15 +102,6 @@ static inline unsigned int sysfs_type(struct sysfs_dirent *sd)
return sd->s_flags & SYSFS_TYPE_MASK;
}
-/*
- * Return any namespace tags on this dirent.
- * enum kobj_ns_type is defined in linux/kobject.h
- */
-static inline enum kobj_ns_type sysfs_ns_type(struct sysfs_dirent *sd)
-{
- return (sd->s_flags & SYSFS_NS_TYPE_MASK) >> SYSFS_NS_TYPE_SHIFT;
-}
-
#ifdef CONFIG_DEBUG_LOCK_ALLOC
#define sysfs_dirent_init_lockdep(sd) \
do { \
@@ -141,12 +129,13 @@ struct sysfs_addrm_cxt {
*/
/*
- * Each sb is associated with a set of namespace tags (i.e.
- * the network namespace of the task which mounted this sysfs
- * instance).
+ * Each sb is associated with one namespace tag, currently the network
+ * namespace of the task which mounted this sysfs instance. If multiple
+ * tags become necessary, make the following an array and compare
+ * sysfs_dirent tag against every entry.
*/
struct sysfs_super_info {
- void *ns[KOBJ_NS_TYPES];
+ void *ns;
};
#define sysfs_info(SB) ((struct sysfs_super_info *)(SB->s_fs_info))
extern struct sysfs_dirent sysfs_root;
diff --git a/lib/kobject.c b/lib/kobject.c
index 85fb3a1..e769ee3 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -29,11 +29,14 @@
const void *kobject_namespace(struct kobject *kobj)
{
const struct kobj_ns_type_operations *ns_ops = kobj_ns_ops(kobj);
+ const void *ns;
if (!ns_ops || ns_ops->type == KOBJ_NS_TYPE_NONE)
return NULL;
- return kobj->ktype->namespace(kobj);
+ ns = kobj->ktype->namespace(kobj);
+ WARN_ON(!ns); /* @kobj in a namespace is required to have !NULL tag */
+ return ns;
}
/*
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 6/7] sysfs: clean up sysfs_get_dirent()
2013-09-12 2:29 [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs Tejun Heo
` (4 preceding siblings ...)
2013-09-12 2:29 ` [PATCH 5/7] sysfs: drop kobj_ns_type handling Tejun Heo
@ 2013-09-12 2:29 ` Tejun Heo
2013-09-12 3:22 ` [PATCH v2 " Tejun Heo
2013-09-12 2:29 ` [PATCH 7/7] sysfs: @name comes before @ns Tejun Heo
` (2 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Tejun Heo @ 2013-09-12 2:29 UTC (permalink / raw)
To: gregkh; +Cc: linux-kernel, kay, ebiederm, netdev, lizefan, Tejun Heo
The pre-existing sysfs interfaces which take explicit namespace
argument are weird in that they place the optional @ns in front of
@name which is contrary to the established convention. For example,
we end up forcing vast majority of sysfs_get_dirent() users to do
sysfs_get_dirent(parent, NULL, name), which is silly and error-prone
especially as @ns and @name may be interchanged without causing
compilation warning.
This renames sysfs_get_dirent() to sysfs_get_dirent_ns() and swap the
positions of @name and @ns, and sysfs_get_dirent() is now a wrapper
around sysfs_get_dirent_ns(). This makes confusions a lot less
likely.
There are other interfaces which take @ns before @name. They'll be
updated by following patches.
This patch doesn't introduce any functional changes.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Kay Sievers <kay@vrfy.org>
---
drivers/gpio/gpiolib.c | 2 +-
drivers/md/bitmap.c | 4 ++--
drivers/md/md.c | 2 +-
drivers/md/md.h | 2 +-
fs/sysfs/dir.c | 9 +++++----
fs/sysfs/file.c | 4 ++--
fs/sysfs/group.c | 10 +++++-----
fs/sysfs/symlink.c | 2 +-
fs/sysfs/sysfs.h | 3 ---
include/linux/sysfs.h | 19 ++++++++++++-------
10 files changed, 30 insertions(+), 27 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 86ef346..a094356 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -408,7 +408,7 @@ static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
if (!value_sd) {
- value_sd = sysfs_get_dirent(dev->kobj.sd, NULL, "value");
+ value_sd = sysfs_get_dirent(dev->kobj.sd, "value");
if (!value_sd) {
ret = -ENODEV;
goto err_out;
diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
index a7fd821..12dc29b 100644
--- a/drivers/md/bitmap.c
+++ b/drivers/md/bitmap.c
@@ -1654,9 +1654,9 @@ int bitmap_create(struct mddev *mddev)
bitmap->mddev = mddev;
if (mddev->kobj.sd)
- bm = sysfs_get_dirent(mddev->kobj.sd, NULL, "bitmap");
+ bm = sysfs_get_dirent(mddev->kobj.sd, "bitmap");
if (bm) {
- bitmap->sysfs_can_clear = sysfs_get_dirent(bm, NULL, "can_clear");
+ bitmap->sysfs_can_clear = sysfs_get_dirent(bm, "can_clear");
sysfs_put(bm);
} else
bitmap->sysfs_can_clear = NULL;
diff --git a/drivers/md/md.c b/drivers/md/md.c
index adf4d7e..8a0d762 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -3555,7 +3555,7 @@ level_store(struct mddev *mddev, const char *buf, size_t len)
printk(KERN_WARNING
"md: cannot register extra attributes for %s\n",
mdname(mddev));
- mddev->sysfs_action = sysfs_get_dirent(mddev->kobj.sd, NULL, "sync_action");
+ mddev->sysfs_action = sysfs_get_dirent(mddev->kobj.sd, "sync_action");
}
if (mddev->pers->sync_request != NULL &&
pers->sync_request == NULL) {
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 608050c..b0051f2 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -501,7 +501,7 @@ extern struct attribute_group md_bitmap_group;
static inline struct sysfs_dirent *sysfs_get_dirent_safe(struct sysfs_dirent *sd, char *name)
{
if (sd)
- return sysfs_get_dirent(sd, NULL, name);
+ return sysfs_get_dirent(sd, name);
return sd;
}
static inline void sysfs_notify_dirent_safe(struct sysfs_dirent *sd)
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 1dfb4aa..3dacce0 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -630,9 +630,10 @@ struct sysfs_dirent *sysfs_find_dirent(struct sysfs_dirent *parent_sd,
}
/**
- * sysfs_get_dirent - find and get sysfs_dirent with the given name
+ * sysfs_get_dirent_ns - find and get sysfs_dirent with the given name
* @parent_sd: sysfs_dirent to search under
* @name: name to look for
+ * @ns: the namespace tag to use
*
* Look for sysfs_dirent with name @name under @parent_sd and get
* it if found.
@@ -643,9 +644,9 @@ struct sysfs_dirent *sysfs_find_dirent(struct sysfs_dirent *parent_sd,
* RETURNS:
* Pointer to sysfs_dirent if found, NULL if not.
*/
-struct sysfs_dirent *sysfs_get_dirent(struct sysfs_dirent *parent_sd,
- const void *ns,
- const unsigned char *name)
+struct sysfs_dirent *sysfs_get_dirent_ns(struct sysfs_dirent *parent_sd,
+ const unsigned char *name,
+ const void *ns)
{
struct sysfs_dirent *sd;
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index e784340..0f3214a 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -563,7 +563,7 @@ int sysfs_add_file_to_group(struct kobject *kobj,
int error;
if (group)
- dir_sd = sysfs_get_dirent(kobj->sd, NULL, group);
+ dir_sd = sysfs_get_dirent(kobj->sd, group);
else
dir_sd = sysfs_get(kobj->sd);
@@ -645,7 +645,7 @@ void sysfs_remove_file_from_group(struct kobject *kobj,
struct sysfs_dirent *dir_sd;
if (group)
- dir_sd = sysfs_get_dirent(kobj->sd, NULL, group);
+ dir_sd = sysfs_get_dirent(kobj->sd, group);
else
dir_sd = sysfs_get(kobj->sd);
if (dir_sd) {
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 25c78f2..2110215 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -207,7 +207,7 @@ void sysfs_remove_group(struct kobject *kobj,
struct sysfs_dirent *sd;
if (grp->name) {
- sd = sysfs_get_dirent(dir_sd, NULL, grp->name);
+ sd = sysfs_get_dirent(dir_sd, grp->name);
if (!sd) {
WARN(!sd, KERN_WARNING
"sysfs group %p not found for kobject '%s'\n",
@@ -262,7 +262,7 @@ int sysfs_merge_group(struct kobject *kobj,
struct attribute *const *attr;
int i;
- dir_sd = sysfs_get_dirent(kobj->sd, NULL, grp->name);
+ dir_sd = sysfs_get_dirent(kobj->sd, grp->name);
if (!dir_sd)
return -ENOENT;
@@ -289,7 +289,7 @@ void sysfs_unmerge_group(struct kobject *kobj,
struct sysfs_dirent *dir_sd;
struct attribute *const *attr;
- dir_sd = sysfs_get_dirent(kobj->sd, NULL, grp->name);
+ dir_sd = sysfs_get_dirent(kobj->sd, grp->name);
if (dir_sd) {
for (attr = grp->attrs; *attr; ++attr)
sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
@@ -311,7 +311,7 @@ int sysfs_add_link_to_group(struct kobject *kobj, const char *group_name,
struct sysfs_dirent *dir_sd;
int error = 0;
- dir_sd = sysfs_get_dirent(kobj->sd, NULL, group_name);
+ dir_sd = sysfs_get_dirent(kobj->sd, group_name);
if (!dir_sd)
return -ENOENT;
@@ -333,7 +333,7 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
{
struct sysfs_dirent *dir_sd;
- dir_sd = sysfs_get_dirent(kobj->sd, NULL, group_name);
+ dir_sd = sysfs_get_dirent(kobj->sd, group_name);
if (dir_sd) {
sysfs_hash_and_remove(dir_sd, NULL, link_name);
sysfs_put(dir_sd);
diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index 7d981ce..c96b31a 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -191,7 +191,7 @@ int sysfs_rename_link_ns(struct kobject *kobj, struct kobject *targ,
old_ns = targ->sd->s_ns;
result = -ENOENT;
- sd = sysfs_get_dirent(parent_sd, old_ns, old);
+ sd = sysfs_get_dirent_ns(parent_sd, old, old_ns);
if (!sd)
goto out;
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index 7664d1b..6faacaf 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -164,9 +164,6 @@ void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt);
struct sysfs_dirent *sysfs_find_dirent(struct sysfs_dirent *parent_sd,
const void *ns,
const unsigned char *name);
-struct sysfs_dirent *sysfs_get_dirent(struct sysfs_dirent *parent_sd,
- const void *ns,
- const unsigned char *name);
struct sysfs_dirent *sysfs_new_dirent(const char *name, umode_t mode, int type);
void release_sysfs_dirent(struct sysfs_dirent *sd);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index c792f73..6695040 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -245,9 +245,9 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
void sysfs_notify(struct kobject *kobj, const char *dir, const char *attr);
void sysfs_notify_dirent(struct sysfs_dirent *sd);
-struct sysfs_dirent *sysfs_get_dirent(struct sysfs_dirent *parent_sd,
- const void *ns,
- const unsigned char *name);
+struct sysfs_dirent *sysfs_get_dirent_ns(struct sysfs_dirent *parent_sd,
+ const unsigned char *name,
+ const void *ns);
struct sysfs_dirent *sysfs_get(struct sysfs_dirent *sd);
void sysfs_put(struct sysfs_dirent *sd);
@@ -422,10 +422,9 @@ static inline void sysfs_notify(struct kobject *kobj, const char *dir,
static inline void sysfs_notify_dirent(struct sysfs_dirent *sd)
{
}
-static inline
-struct sysfs_dirent *sysfs_get_dirent(struct sysfs_dirent *parent_sd,
- const void *ns,
- const unsigned char *name)
+static inline struct sysfs_dirent *
+sysfs_get_dirent_ns(struct sysfs_dirent *parent_sd, const unsigned char *name,
+ const void *ns)
{
return NULL;
}
@@ -462,4 +461,10 @@ static inline int sysfs_rename_link(struct kobject *kobj, struct kobject *target
return sysfs_rename_link_ns(kobj, target, old_name, new_name, NULL);
}
+static inline struct sysfs_dirent *
+sysfs_get_dirent(struct sysfs_dirent *parent_sd, const unsigned char *name)
+{
+ return sysfs_get_dirent_ns(parent_sd, name, NULL);
+}
+
#endif /* _SYSFS_H_ */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 7/7] sysfs: @name comes before @ns
2013-09-12 2:29 [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs Tejun Heo
` (5 preceding siblings ...)
2013-09-12 2:29 ` [PATCH 6/7] sysfs: clean up sysfs_get_dirent() Tejun Heo
@ 2013-09-12 2:29 ` Tejun Heo
2013-09-12 3:39 ` Eric W. Biederman
2013-09-12 3:19 ` [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs Greg KH
2013-09-12 3:37 ` Eric W. Biederman
8 siblings, 1 reply; 21+ messages in thread
From: Tejun Heo @ 2013-09-12 2:29 UTC (permalink / raw)
To: gregkh; +Cc: linux-kernel, kay, ebiederm, netdev, lizefan, Tejun Heo
Some internal sysfs functions which take explicit namespace argument
are weird in that they place the optional @ns in front of @name which
is contrary to the established convention. This is confusing and
error-prone especially as @ns and @name may be interchanged without
causing compilation warning.
Swap the positions of @name and @ns in the following internal
functions.
sysfs_find_dirent()
sysfs_rename()
sysfs_hash_and_remove()
sysfs_name_hash()
sysfs_name_compare()
create_dir()
This patch doesn't introduce any functional changes.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Kay Sievers <kay@vrfy.org>
---
fs/sysfs/bin.c | 2 +-
fs/sysfs/dir.c | 45 +++++++++++++++++++++++----------------------
fs/sysfs/file.c | 10 +++++-----
fs/sysfs/group.c | 12 ++++++------
fs/sysfs/inode.c | 6 +++---
fs/sysfs/symlink.c | 6 +++---
fs/sysfs/sysfs.h | 10 +++++-----
7 files changed, 46 insertions(+), 45 deletions(-)
diff --git a/fs/sysfs/bin.c b/fs/sysfs/bin.c
index c590cab..d49e6ca 100644
--- a/fs/sysfs/bin.c
+++ b/fs/sysfs/bin.c
@@ -497,6 +497,6 @@ EXPORT_SYMBOL_GPL(sysfs_create_bin_file);
void sysfs_remove_bin_file(struct kobject *kobj,
const struct bin_attribute *attr)
{
- sysfs_hash_and_remove(kobj->sd, NULL, attr->attr.name);
+ sysfs_hash_and_remove(kobj->sd, attr->attr.name, NULL);
}
EXPORT_SYMBOL_GPL(sysfs_remove_bin_file);
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 3dacce0..d41b555 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -35,12 +35,12 @@ static DEFINE_IDA(sysfs_ino_ida);
/**
* sysfs_name_hash
- * @ns: Namespace tag to hash
* @name: Null terminated string to hash
+ * @ns: Namespace tag to hash
*
* Returns 31 bit hash of ns + name (so it fits in an off_t )
*/
-static unsigned int sysfs_name_hash(const void *ns, const char *name)
+static unsigned int sysfs_name_hash(const char *name, const void *ns)
{
unsigned long hash = init_name_hash();
unsigned int len = strlen(name);
@@ -56,8 +56,8 @@ static unsigned int sysfs_name_hash(const void *ns, const char *name)
return hash;
}
-static int sysfs_name_compare(unsigned int hash, const void *ns,
- const char *name, const struct sysfs_dirent *sd)
+static int sysfs_name_compare(unsigned int hash, const char *name,
+ const void *ns, const struct sysfs_dirent *sd)
{
if (hash != sd->s_hash)
return hash - sd->s_hash;
@@ -69,7 +69,7 @@ static int sysfs_name_compare(unsigned int hash, const void *ns,
static int sysfs_sd_compare(const struct sysfs_dirent *left,
const struct sysfs_dirent *right)
{
- return sysfs_name_compare(left->s_hash, left->s_ns, left->s_name,
+ return sysfs_name_compare(left->s_hash, left->s_name, left->s_ns,
right);
}
@@ -451,7 +451,7 @@ int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
struct sysfs_inode_attrs *ps_iattr;
int ret;
- sd->s_hash = sysfs_name_hash(sd->s_ns, sd->s_name);
+ sd->s_hash = sysfs_name_hash(sd->s_name, sd->s_ns);
sd->s_parent = sysfs_get(acxt->parent_sd);
ret = sysfs_link_sibling(sd);
@@ -596,6 +596,7 @@ void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt)
* sysfs_find_dirent - find sysfs_dirent with the given name
* @parent_sd: sysfs_dirent to search under
* @name: name to look for
+ * @ns: the namespace tag to use
*
* Look for sysfs_dirent with name @name under @parent_sd.
*
@@ -606,19 +607,19 @@ void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt)
* Pointer to sysfs_dirent if found, NULL if not.
*/
struct sysfs_dirent *sysfs_find_dirent(struct sysfs_dirent *parent_sd,
- const void *ns,
- const unsigned char *name)
+ const unsigned char *name,
+ const void *ns)
{
struct rb_node *node = parent_sd->s_dir.children.rb_node;
unsigned int hash;
- hash = sysfs_name_hash(ns, name);
+ hash = sysfs_name_hash(name, ns);
while (node) {
struct sysfs_dirent *sd;
int result;
sd = to_sysfs_dirent(node);
- result = sysfs_name_compare(hash, ns, name, sd);
+ result = sysfs_name_compare(hash, name, ns, sd);
if (result < 0)
node = node->rb_left;
else if (result > 0)
@@ -651,7 +652,7 @@ struct sysfs_dirent *sysfs_get_dirent_ns(struct sysfs_dirent *parent_sd,
struct sysfs_dirent *sd;
mutex_lock(&sysfs_mutex);
- sd = sysfs_find_dirent(parent_sd, ns, name);
+ sd = sysfs_find_dirent(parent_sd, name, ns);
sysfs_get(sd);
mutex_unlock(&sysfs_mutex);
@@ -660,7 +661,8 @@ struct sysfs_dirent *sysfs_get_dirent_ns(struct sysfs_dirent *parent_sd,
EXPORT_SYMBOL_GPL(sysfs_get_dirent);
static int create_dir(struct kobject *kobj, struct sysfs_dirent *parent_sd,
- const void *ns, const char *name, struct sysfs_dirent **p_sd)
+ const char *name, const void *ns,
+ struct sysfs_dirent **p_sd)
{
umode_t mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
struct sysfs_addrm_cxt acxt;
@@ -691,7 +693,7 @@ static int create_dir(struct kobject *kobj, struct sysfs_dirent *parent_sd,
int sysfs_create_subdir(struct kobject *kobj, const char *name,
struct sysfs_dirent **p_sd)
{
- return create_dir(kobj, kobj->sd, NULL, name, p_sd);
+ return create_dir(kobj, kobj->sd, name, NULL, p_sd);
}
/**
@@ -714,7 +716,7 @@ int sysfs_create_dir_ns(struct kobject *kobj, const void *ns)
if (!parent_sd)
return -ENOENT;
- error = create_dir(kobj, parent_sd, ns, kobject_name(kobj), &sd);
+ error = create_dir(kobj, parent_sd, kobject_name(kobj), ns, &sd);
if (!error)
kobj->sd = sd;
return error;
@@ -735,7 +737,7 @@ static struct dentry *sysfs_lookup(struct inode *dir, struct dentry *dentry,
if (parent_sd->s_flags & SYSFS_FLAG_HAS_NS)
ns = sysfs_info(dir->i_sb)->ns;
- sd = sysfs_find_dirent(parent_sd, ns, dentry->d_name.name);
+ sd = sysfs_find_dirent(parent_sd, dentry->d_name.name, ns);
/* no such entry */
if (!sd) {
@@ -823,9 +825,8 @@ void sysfs_remove_dir(struct kobject *kobj)
__sysfs_remove_dir(sd);
}
-int sysfs_rename(struct sysfs_dirent *sd,
- struct sysfs_dirent *new_parent_sd, const void *new_ns,
- const char *new_name)
+int sysfs_rename(struct sysfs_dirent *sd, struct sysfs_dirent *new_parent_sd,
+ const char *new_name, const void *new_ns)
{
int error;
@@ -837,7 +838,7 @@ int sysfs_rename(struct sysfs_dirent *sd,
goto out; /* nothing to rename */
error = -EEXIST;
- if (sysfs_find_dirent(new_parent_sd, new_ns, new_name))
+ if (sysfs_find_dirent(new_parent_sd, new_name, new_ns))
goto out;
/* rename sysfs_dirent */
@@ -858,7 +859,7 @@ int sysfs_rename(struct sysfs_dirent *sd,
sysfs_get(new_parent_sd);
sysfs_put(sd->s_parent);
sd->s_ns = new_ns;
- sd->s_hash = sysfs_name_hash(sd->s_ns, sd->s_name);
+ sd->s_hash = sysfs_name_hash(sd->s_name, sd->s_ns);
sd->s_parent = new_parent_sd;
sysfs_link_sibling(sd);
@@ -873,7 +874,7 @@ int sysfs_rename_dir_ns(struct kobject *kobj, const char *new_name,
{
struct sysfs_dirent *parent_sd = kobj->sd->s_parent;
- return sysfs_rename(kobj->sd, parent_sd, new_ns, new_name);
+ return sysfs_rename(kobj->sd, parent_sd, new_name, new_ns);
}
int sysfs_move_dir_ns(struct kobject *kobj, struct kobject *new_parent_kobj,
@@ -886,7 +887,7 @@ int sysfs_move_dir_ns(struct kobject *kobj, struct kobject *new_parent_kobj,
new_parent_sd = new_parent_kobj && new_parent_kobj->sd ?
new_parent_kobj->sd : &sysfs_root;
- return sysfs_rename(sd, new_parent_sd, new_ns, sd->s_name);
+ return sysfs_rename(sd, new_parent_sd, sd->s_name, new_ns);
}
/* Relationship between s_mode and the DT_xxx types */
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 0f3214a..4697019 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -466,9 +466,9 @@ void sysfs_notify(struct kobject *k, const char *dir, const char *attr)
mutex_lock(&sysfs_mutex);
if (sd && dir)
- sd = sysfs_find_dirent(sd, NULL, dir);
+ sd = sysfs_find_dirent(sd, dir, NULL);
if (sd && attr)
- sd = sysfs_find_dirent(sd, NULL, attr);
+ sd = sysfs_find_dirent(sd, attr, NULL);
if (sd)
sysfs_notify_dirent(sd);
@@ -594,7 +594,7 @@ int sysfs_chmod_file(struct kobject *kobj, const struct attribute *attr,
mutex_lock(&sysfs_mutex);
rc = -ENOENT;
- sd = sysfs_find_dirent(kobj->sd, NULL, attr->name);
+ sd = sysfs_find_dirent(kobj->sd, attr->name, NULL);
if (!sd)
goto out;
@@ -621,7 +621,7 @@ void sysfs_remove_file_ns(struct kobject *kobj, const struct attribute *attr,
{
struct sysfs_dirent *dir_sd = kobj->sd;
- sysfs_hash_and_remove(dir_sd, ns, attr->name);
+ sysfs_hash_and_remove(dir_sd, attr->name, ns);
}
EXPORT_SYMBOL_GPL(sysfs_remove_file_ns);
@@ -649,7 +649,7 @@ void sysfs_remove_file_from_group(struct kobject *kobj,
else
dir_sd = sysfs_get(kobj->sd);
if (dir_sd) {
- sysfs_hash_and_remove(dir_sd, NULL, attr->name);
+ sysfs_hash_and_remove(dir_sd, attr->name, NULL);
sysfs_put(dir_sd);
}
}
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 2110215..2dae55c 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -26,7 +26,7 @@ static void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
if (grp->attrs)
for (attr = grp->attrs; *attr; attr++)
- sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
+ sysfs_hash_and_remove(dir_sd, (*attr)->name, NULL);
if (grp->bin_attrs)
for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++)
sysfs_remove_bin_file(kobj, *bin_attr);
@@ -49,8 +49,8 @@ static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
* re-adding (if required) the file.
*/
if (update)
- sysfs_hash_and_remove(dir_sd, NULL,
- (*attr)->name);
+ sysfs_hash_and_remove(dir_sd, (*attr)->name,
+ NULL);
if (grp->is_visible) {
mode = grp->is_visible(kobj, *attr, i);
if (!mode)
@@ -270,7 +270,7 @@ int sysfs_merge_group(struct kobject *kobj,
error = sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
if (error) {
while (--i >= 0)
- sysfs_hash_and_remove(dir_sd, NULL, (*--attr)->name);
+ sysfs_hash_and_remove(dir_sd, (*--attr)->name, NULL);
}
sysfs_put(dir_sd);
@@ -292,7 +292,7 @@ void sysfs_unmerge_group(struct kobject *kobj,
dir_sd = sysfs_get_dirent(kobj->sd, grp->name);
if (dir_sd) {
for (attr = grp->attrs; *attr; ++attr)
- sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
+ sysfs_hash_and_remove(dir_sd, (*attr)->name, NULL);
sysfs_put(dir_sd);
}
}
@@ -335,7 +335,7 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
dir_sd = sysfs_get_dirent(kobj->sd, group_name);
if (dir_sd) {
- sysfs_hash_and_remove(dir_sd, NULL, link_name);
+ sysfs_hash_and_remove(dir_sd, link_name, NULL);
sysfs_put(dir_sd);
}
}
diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
index 963f910..07193d7 100644
--- a/fs/sysfs/inode.c
+++ b/fs/sysfs/inode.c
@@ -314,8 +314,8 @@ void sysfs_evict_inode(struct inode *inode)
sysfs_put(sd);
}
-int sysfs_hash_and_remove(struct sysfs_dirent *dir_sd, const void *ns,
- const char *name)
+int sysfs_hash_and_remove(struct sysfs_dirent *dir_sd, const char *name,
+ const void *ns)
{
struct sysfs_addrm_cxt acxt;
struct sysfs_dirent *sd;
@@ -328,7 +328,7 @@ int sysfs_hash_and_remove(struct sysfs_dirent *dir_sd, const void *ns,
sysfs_addrm_start(&acxt, dir_sd);
- sd = sysfs_find_dirent(dir_sd, ns, name);
+ sd = sysfs_find_dirent(dir_sd, name, ns);
if (sd)
sysfs_remove_one(&acxt, sd);
diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index c96b31a..88c8bc5 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -144,7 +144,7 @@ void sysfs_delete_link(struct kobject *kobj, struct kobject *targ,
if (targ->sd)
ns = targ->sd->s_ns;
spin_unlock(&sysfs_assoc_lock);
- sysfs_hash_and_remove(kobj->sd, ns, name);
+ sysfs_hash_and_remove(kobj->sd, name, ns);
}
/**
@@ -161,7 +161,7 @@ void sysfs_remove_link(struct kobject *kobj, const char *name)
else
parent_sd = kobj->sd;
- sysfs_hash_and_remove(parent_sd, NULL, name);
+ sysfs_hash_and_remove(parent_sd, name, NULL);
}
EXPORT_SYMBOL_GPL(sysfs_remove_link);
@@ -201,7 +201,7 @@ int sysfs_rename_link_ns(struct kobject *kobj, struct kobject *targ,
if (sd->s_symlink.target_sd->s_dir.kobj != targ)
goto out;
- result = sysfs_rename(sd, parent_sd, new_ns, new);
+ result = sysfs_rename(sd, parent_sd, new, new_ns);
out:
sysfs_put(sd);
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index 6faacaf..ee44fde 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -162,8 +162,8 @@ void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt);
struct sysfs_dirent *sysfs_find_dirent(struct sysfs_dirent *parent_sd,
- const void *ns,
- const unsigned char *name);
+ const unsigned char *name,
+ const void *ns);
struct sysfs_dirent *sysfs_new_dirent(const char *name, umode_t mode, int type);
void release_sysfs_dirent(struct sysfs_dirent *sd);
@@ -173,7 +173,7 @@ int sysfs_create_subdir(struct kobject *kobj, const char *name,
void sysfs_remove_subdir(struct sysfs_dirent *sd);
int sysfs_rename(struct sysfs_dirent *sd, struct sysfs_dirent *new_parent_sd,
- const void *ns, const char *new_name);
+ const char *new_name, const void *new_ns);
static inline struct sysfs_dirent *__sysfs_get(struct sysfs_dirent *sd)
{
@@ -204,8 +204,8 @@ int sysfs_getattr(struct vfsmount *mnt, struct dentry *dentry,
struct kstat *stat);
int sysfs_setxattr(struct dentry *dentry, const char *name, const void *value,
size_t size, int flags);
-int sysfs_hash_and_remove(struct sysfs_dirent *dir_sd, const void *ns,
- const char *name);
+int sysfs_hash_and_remove(struct sysfs_dirent *dir_sd, const char *name,
+ const void *ns);
int sysfs_inode_init(void);
/*
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs
2013-09-12 2:29 [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs Tejun Heo
` (6 preceding siblings ...)
2013-09-12 2:29 ` [PATCH 7/7] sysfs: @name comes before @ns Tejun Heo
@ 2013-09-12 3:19 ` Greg KH
2013-09-12 3:23 ` Tejun Heo
2013-09-12 3:38 ` Eric W. Biederman
2013-09-12 3:37 ` Eric W. Biederman
8 siblings, 2 replies; 21+ messages in thread
From: Greg KH @ 2013-09-12 3:19 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-kernel, kay, ebiederm, netdev, lizefan
On Wed, Sep 11, 2013 at 10:29:02PM -0400, Tejun Heo wrote:
> Hello,
>
> I'll send out multiple patchsets to separate out sysfs from driver
> core and kobject. The eventual goal is making sysfs modular enough so
> that cgroup can replace its nightmarish cgroupfs implementation which
> repeated and worsened all the past mistakes of sysfs. This patchset
> is first of the effort and separates out kobject namespace handling
> from sysfs.
>
> I never really understood why namespace support was added the way it
> was added.
I just took the patches and didn't ask questions :)
> Namespace information is communicated to sysfs via
> callbacks and back-queries to upper layer, which is a very unusual and
> weird thing to do when all the involved operations are synchronous.
> For example, a tagged attribute creation looks like the following.
>
> driver code driver callback
> v ^
> netdev_class_create_file() |
> v class_attr->namespace()
> class_create_file() class_attr_namespace()
> v |
> sysfs_create_file() |
> v |
> sysfs_attr_ns() -------------> sysfs_ops->namespace()
>
> This is an absurd thing to do. It significantly obfuscates what's
> going on and adds unnecessary uncertainties - for example, can
> namespace() return value disagree with the recorded s_ns value without
> being renamed? If so, how should that be handled? If not, what
> guarantees that? Even the basic placements of callbacks don't make
> much, if any, sense. Why is per-directory namespace() callback in
> kobj_type while per-attr namespace() callback is in sysfs_ops? What
> does this even mean?
>
> Maybe there's some grand design scheme governing all this but it isn't
> obvious at all and the whole thing looks like a hodgepodge of
> short-sighted hacks.
>
> There is absolutely *nothing* which requires this convolution. NS tag
> can simply be passed down the stack just like any other type of
> information and adding an extra argument or variant of interface to
> pass down the extra information is way more straight-forward and
> apparently even takes less amount of code, so let's please stop the
> insanity.
>
> This patchset contains the following seven patches.
>
> 0001-sysfs-drop-semicolon-from-to_sysfs_dirent-definition.patch
> 0002-sysfs-make-attr-namespace-interface-less-convoluted.patch
> 0003-sysfs-remove-ktype-namespace-invocations-in-director.patch
> 0004-sysfs-remove-ktype-namespace-invocations-in-symlink-.patch
> 0005-sysfs-drop-kobj_ns_type-handling.patch
> 0006-sysfs-clean-up-sysfs_get_dirent.patch
> 0007-sysfs-name-comes-before-ns.patch
>
> 0001 is a minor prep patch.
>
> 0002 removes the attr namespace callback.
>
> 0003-0004 push the dir namespace callback invocations from sysfs to
> kobjct layer. Eventually, this callback should go too.
>
> 0005 simplifies sysfs ns support such that sysfs doesn't have any
> specific knowledge of kobj namespaces. It now purely deals with
> pointer tags. Combined with the previous changes, this makes sysfs ns
> support mostly modular.
>
> 0006-0007 are cleanup patches to make param orders conventional and
> consistent - optional param after mandatory one; otherwise, things get
> extremely confusing with different variants of interfaces which take
> or don't take optional params. No idea how/why this was done the
> wrong way.
>
> This patchset is based on top of the current master
> c2d95729e3094ecdd8c54e856bbe971adbbd7f48 ("Merge branch 'akpm'
> (patches from Andrew Morton)") and available in the following git
> branch.
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git review-sysfs-separate-out-ns
Nice job with these. Do you want me to add them to my tree for 3.13, or
do you want to take them through yours as you will be building on top of
them?
If yours, feel free to add:
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To all of these.
And thanks for cleaning this up, it looks much better now.
greg k-h
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 6/7] sysfs: clean up sysfs_get_dirent()
2013-09-12 2:29 ` [PATCH 6/7] sysfs: clean up sysfs_get_dirent() Tejun Heo
@ 2013-09-12 3:22 ` Tejun Heo
0 siblings, 0 replies; 21+ messages in thread
From: Tejun Heo @ 2013-09-12 3:22 UTC (permalink / raw)
To: gregkh; +Cc: linux-kernel, kay, ebiederm, netdev, lizefan
>From 5a4b7340199b2d6ff15b6fc551b0ea3f2cc19b6e Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj@kernel.org>
Date: Wed, 11 Sep 2013 23:19:13 -0400
The pre-existing sysfs interfaces which take explicit namespace
argument are weird in that they place the optional @ns in front of
@name which is contrary to the established convention. For example,
we end up forcing vast majority of sysfs_get_dirent() users to do
sysfs_get_dirent(parent, NULL, name), which is silly and error-prone
especially as @ns and @name may be interchanged without causing
compilation warning.
This renames sysfs_get_dirent() to sysfs_get_dirent_ns() and swap the
positions of @name and @ns, and sysfs_get_dirent() is now a wrapper
around sysfs_get_dirent_ns(). This makes confusions a lot less
likely.
There are other interfaces which take @ns before @name. They'll be
updated by following patches.
This patch doesn't introduce any functional changes.
v2: EXPORT_SYMBOL_GPL() wasn't updated leading to undefined symbol
error on module builds. Reported by build test robot. Fixed.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Kay Sievers <kay@vrfy.org>
Cc: Fengguang Wu <fengguang.wu@intel.com>
---
drivers/gpio/gpiolib.c | 2 +-
drivers/md/bitmap.c | 4 ++--
drivers/md/md.c | 2 +-
drivers/md/md.h | 2 +-
fs/sysfs/dir.c | 11 ++++++-----
fs/sysfs/file.c | 4 ++--
fs/sysfs/group.c | 10 +++++-----
fs/sysfs/symlink.c | 2 +-
fs/sysfs/sysfs.h | 3 ---
include/linux/sysfs.h | 19 ++++++++++++-------
10 files changed, 31 insertions(+), 28 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 86ef346..a094356 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -408,7 +408,7 @@ static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
if (!value_sd) {
- value_sd = sysfs_get_dirent(dev->kobj.sd, NULL, "value");
+ value_sd = sysfs_get_dirent(dev->kobj.sd, "value");
if (!value_sd) {
ret = -ENODEV;
goto err_out;
diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
index a7fd821..12dc29b 100644
--- a/drivers/md/bitmap.c
+++ b/drivers/md/bitmap.c
@@ -1654,9 +1654,9 @@ int bitmap_create(struct mddev *mddev)
bitmap->mddev = mddev;
if (mddev->kobj.sd)
- bm = sysfs_get_dirent(mddev->kobj.sd, NULL, "bitmap");
+ bm = sysfs_get_dirent(mddev->kobj.sd, "bitmap");
if (bm) {
- bitmap->sysfs_can_clear = sysfs_get_dirent(bm, NULL, "can_clear");
+ bitmap->sysfs_can_clear = sysfs_get_dirent(bm, "can_clear");
sysfs_put(bm);
} else
bitmap->sysfs_can_clear = NULL;
diff --git a/drivers/md/md.c b/drivers/md/md.c
index adf4d7e..8a0d762 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -3555,7 +3555,7 @@ level_store(struct mddev *mddev, const char *buf, size_t len)
printk(KERN_WARNING
"md: cannot register extra attributes for %s\n",
mdname(mddev));
- mddev->sysfs_action = sysfs_get_dirent(mddev->kobj.sd, NULL, "sync_action");
+ mddev->sysfs_action = sysfs_get_dirent(mddev->kobj.sd, "sync_action");
}
if (mddev->pers->sync_request != NULL &&
pers->sync_request == NULL) {
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 608050c..b0051f2 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -501,7 +501,7 @@ extern struct attribute_group md_bitmap_group;
static inline struct sysfs_dirent *sysfs_get_dirent_safe(struct sysfs_dirent *sd, char *name)
{
if (sd)
- return sysfs_get_dirent(sd, NULL, name);
+ return sysfs_get_dirent(sd, name);
return sd;
}
static inline void sysfs_notify_dirent_safe(struct sysfs_dirent *sd)
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 1dfb4aa..fee19d1 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -630,9 +630,10 @@ struct sysfs_dirent *sysfs_find_dirent(struct sysfs_dirent *parent_sd,
}
/**
- * sysfs_get_dirent - find and get sysfs_dirent with the given name
+ * sysfs_get_dirent_ns - find and get sysfs_dirent with the given name
* @parent_sd: sysfs_dirent to search under
* @name: name to look for
+ * @ns: the namespace tag to use
*
* Look for sysfs_dirent with name @name under @parent_sd and get
* it if found.
@@ -643,9 +644,9 @@ struct sysfs_dirent *sysfs_find_dirent(struct sysfs_dirent *parent_sd,
* RETURNS:
* Pointer to sysfs_dirent if found, NULL if not.
*/
-struct sysfs_dirent *sysfs_get_dirent(struct sysfs_dirent *parent_sd,
- const void *ns,
- const unsigned char *name)
+struct sysfs_dirent *sysfs_get_dirent_ns(struct sysfs_dirent *parent_sd,
+ const unsigned char *name,
+ const void *ns)
{
struct sysfs_dirent *sd;
@@ -656,7 +657,7 @@ struct sysfs_dirent *sysfs_get_dirent(struct sysfs_dirent *parent_sd,
return sd;
}
-EXPORT_SYMBOL_GPL(sysfs_get_dirent);
+EXPORT_SYMBOL_GPL(sysfs_get_dirent_ns);
static int create_dir(struct kobject *kobj, struct sysfs_dirent *parent_sd,
const void *ns, const char *name, struct sysfs_dirent **p_sd)
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index e784340..0f3214a 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -563,7 +563,7 @@ int sysfs_add_file_to_group(struct kobject *kobj,
int error;
if (group)
- dir_sd = sysfs_get_dirent(kobj->sd, NULL, group);
+ dir_sd = sysfs_get_dirent(kobj->sd, group);
else
dir_sd = sysfs_get(kobj->sd);
@@ -645,7 +645,7 @@ void sysfs_remove_file_from_group(struct kobject *kobj,
struct sysfs_dirent *dir_sd;
if (group)
- dir_sd = sysfs_get_dirent(kobj->sd, NULL, group);
+ dir_sd = sysfs_get_dirent(kobj->sd, group);
else
dir_sd = sysfs_get(kobj->sd);
if (dir_sd) {
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 25c78f2..2110215 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -207,7 +207,7 @@ void sysfs_remove_group(struct kobject *kobj,
struct sysfs_dirent *sd;
if (grp->name) {
- sd = sysfs_get_dirent(dir_sd, NULL, grp->name);
+ sd = sysfs_get_dirent(dir_sd, grp->name);
if (!sd) {
WARN(!sd, KERN_WARNING
"sysfs group %p not found for kobject '%s'\n",
@@ -262,7 +262,7 @@ int sysfs_merge_group(struct kobject *kobj,
struct attribute *const *attr;
int i;
- dir_sd = sysfs_get_dirent(kobj->sd, NULL, grp->name);
+ dir_sd = sysfs_get_dirent(kobj->sd, grp->name);
if (!dir_sd)
return -ENOENT;
@@ -289,7 +289,7 @@ void sysfs_unmerge_group(struct kobject *kobj,
struct sysfs_dirent *dir_sd;
struct attribute *const *attr;
- dir_sd = sysfs_get_dirent(kobj->sd, NULL, grp->name);
+ dir_sd = sysfs_get_dirent(kobj->sd, grp->name);
if (dir_sd) {
for (attr = grp->attrs; *attr; ++attr)
sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
@@ -311,7 +311,7 @@ int sysfs_add_link_to_group(struct kobject *kobj, const char *group_name,
struct sysfs_dirent *dir_sd;
int error = 0;
- dir_sd = sysfs_get_dirent(kobj->sd, NULL, group_name);
+ dir_sd = sysfs_get_dirent(kobj->sd, group_name);
if (!dir_sd)
return -ENOENT;
@@ -333,7 +333,7 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
{
struct sysfs_dirent *dir_sd;
- dir_sd = sysfs_get_dirent(kobj->sd, NULL, group_name);
+ dir_sd = sysfs_get_dirent(kobj->sd, group_name);
if (dir_sd) {
sysfs_hash_and_remove(dir_sd, NULL, link_name);
sysfs_put(dir_sd);
diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index 7d981ce..c96b31a 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -191,7 +191,7 @@ int sysfs_rename_link_ns(struct kobject *kobj, struct kobject *targ,
old_ns = targ->sd->s_ns;
result = -ENOENT;
- sd = sysfs_get_dirent(parent_sd, old_ns, old);
+ sd = sysfs_get_dirent_ns(parent_sd, old, old_ns);
if (!sd)
goto out;
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index 7664d1b..6faacaf 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -164,9 +164,6 @@ void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt);
struct sysfs_dirent *sysfs_find_dirent(struct sysfs_dirent *parent_sd,
const void *ns,
const unsigned char *name);
-struct sysfs_dirent *sysfs_get_dirent(struct sysfs_dirent *parent_sd,
- const void *ns,
- const unsigned char *name);
struct sysfs_dirent *sysfs_new_dirent(const char *name, umode_t mode, int type);
void release_sysfs_dirent(struct sysfs_dirent *sd);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index c792f73..6695040 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -245,9 +245,9 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
void sysfs_notify(struct kobject *kobj, const char *dir, const char *attr);
void sysfs_notify_dirent(struct sysfs_dirent *sd);
-struct sysfs_dirent *sysfs_get_dirent(struct sysfs_dirent *parent_sd,
- const void *ns,
- const unsigned char *name);
+struct sysfs_dirent *sysfs_get_dirent_ns(struct sysfs_dirent *parent_sd,
+ const unsigned char *name,
+ const void *ns);
struct sysfs_dirent *sysfs_get(struct sysfs_dirent *sd);
void sysfs_put(struct sysfs_dirent *sd);
@@ -422,10 +422,9 @@ static inline void sysfs_notify(struct kobject *kobj, const char *dir,
static inline void sysfs_notify_dirent(struct sysfs_dirent *sd)
{
}
-static inline
-struct sysfs_dirent *sysfs_get_dirent(struct sysfs_dirent *parent_sd,
- const void *ns,
- const unsigned char *name)
+static inline struct sysfs_dirent *
+sysfs_get_dirent_ns(struct sysfs_dirent *parent_sd, const unsigned char *name,
+ const void *ns)
{
return NULL;
}
@@ -462,4 +461,10 @@ static inline int sysfs_rename_link(struct kobject *kobj, struct kobject *target
return sysfs_rename_link_ns(kobj, target, old_name, new_name, NULL);
}
+static inline struct sysfs_dirent *
+sysfs_get_dirent(struct sysfs_dirent *parent_sd, const unsigned char *name)
+{
+ return sysfs_get_dirent_ns(parent_sd, name, NULL);
+}
+
#endif /* _SYSFS_H_ */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs
2013-09-12 3:19 ` [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs Greg KH
@ 2013-09-12 3:23 ` Tejun Heo
2013-09-12 3:33 ` Greg KH
2013-09-12 3:38 ` Eric W. Biederman
1 sibling, 1 reply; 21+ messages in thread
From: Tejun Heo @ 2013-09-12 3:23 UTC (permalink / raw)
To: Greg KH; +Cc: linux-kernel, kay, ebiederm, netdev, lizefan
Hello, Greg.
> Nice job with these. Do you want me to add them to my tree for 3.13, or
> do you want to take them through yours as you will be building on top of
> them?
I think it'll be best to route them through your tree but maybe we
want to wait a bit before applying them?
Thanks!
--
tejun
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs
2013-09-12 3:23 ` Tejun Heo
@ 2013-09-12 3:33 ` Greg KH
2013-09-12 3:34 ` Tejun Heo
0 siblings, 1 reply; 21+ messages in thread
From: Greg KH @ 2013-09-12 3:33 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-kernel, kay, ebiederm, netdev, lizefan
On Wed, Sep 11, 2013 at 11:23:16PM -0400, Tejun Heo wrote:
> Hello, Greg.
>
> > Nice job with these. Do you want me to add them to my tree for 3.13, or
> > do you want to take them through yours as you will be building on top of
> > them?
>
> I think it'll be best to route them through your tree but maybe we
> want to wait a bit before applying them?
I have to wait for 3.12-rc1 to come out before applying anything, and
then we will be at LinuxCon/Plumbers drinking^Wworking all next week, so
it will be a bit before any of this could hit my tree.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs
2013-09-12 3:33 ` Greg KH
@ 2013-09-12 3:34 ` Tejun Heo
0 siblings, 0 replies; 21+ messages in thread
From: Tejun Heo @ 2013-09-12 3:34 UTC (permalink / raw)
To: Greg KH; +Cc: linux-kernel, kay, ebiederm, netdev, lizefan
On Wed, Sep 11, 2013 at 08:33:44PM -0700, Greg KH wrote:
> I have to wait for 3.12-rc1 to come out before applying anything, and
> then we will be at LinuxCon/Plumbers drinking^Wworking all next week, so
> it will be a bit before any of this could hit my tree.
Heh, sounds good. See you soon in New Orleans! :)
--
tejun
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs
2013-09-12 2:29 [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs Tejun Heo
` (7 preceding siblings ...)
2013-09-12 3:19 ` [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs Greg KH
@ 2013-09-12 3:37 ` Eric W. Biederman
2013-09-12 3:47 ` Tejun Heo
8 siblings, 1 reply; 21+ messages in thread
From: Eric W. Biederman @ 2013-09-12 3:37 UTC (permalink / raw)
To: Tejun Heo; +Cc: gregkh, linux-kernel, kay, netdev, lizefan
Tejun Heo <tj@kernel.org> writes:
> Hello,
>
> I'll send out multiple patchsets to separate out sysfs from driver
> core and kobject. The eventual goal is making sysfs modular enough so
> that cgroup can replace its nightmarish cgroupfs implementation which
> repeated and worsened all the past mistakes of sysfs. This patchset
> is first of the effort and separates out kobject namespace handling
> from sysfs.
At a practical level you probably just want to copy the good parts of
the structure of sysfs, instead of attempting to share code.
Sharing code is likely to get you into all kinds of problems with short
term hacks.
> I never really understood why namespace support was added the way it
> was added. Namespace information is communicated to sysfs via
> callbacks and back-queries to upper layer, which is a very unusual and
> weird thing to do when all the involved operations are synchronous.
> For example, a tagged attribute creation looks like the following.
Then please ask.
I don't have the time or energy to review these right now, and given the
sweeping nature of the patches, and the dismissive attitude of the
original design there is almost at least one stupid bug if not something
worse.
So until I have the energy to review these.
Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
I am sorry but I don't have time to clean up after any more people
touching sysfs when the break something. It does look like there are so
possibly good things going on but..
Eric
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs
2013-09-12 3:19 ` [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs Greg KH
2013-09-12 3:23 ` Tejun Heo
@ 2013-09-12 3:38 ` Eric W. Biederman
2013-09-12 4:17 ` Greg KH
1 sibling, 1 reply; 21+ messages in thread
From: Eric W. Biederman @ 2013-09-12 3:38 UTC (permalink / raw)
To: Greg KH; +Cc: Tejun Heo, linux-kernel, kay, netdev, lizefan
Greg KH <gregkh@linuxfoundation.org> writes:
> On Wed, Sep 11, 2013 at 10:29:02PM -0400, Tejun Heo wrote:
>> Hello,
>>
>> I'll send out multiple patchsets to separate out sysfs from driver
>> core and kobject. The eventual goal is making sysfs modular enough so
>> that cgroup can replace its nightmarish cgroupfs implementation which
>> repeated and worsened all the past mistakes of sysfs. This patchset
>> is first of the effort and separates out kobject namespace handling
>> from sysfs.
>>
>> I never really understood why namespace support was added the way it
>> was added.
>
> I just took the patches and didn't ask questions :)
Greg you don't get to play dumb you asked questions and required most of
the current strucuture of the code.
The code is convoluted by your request.
Eric
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 7/7] sysfs: @name comes before @ns
2013-09-12 2:29 ` [PATCH 7/7] sysfs: @name comes before @ns Tejun Heo
@ 2013-09-12 3:39 ` Eric W. Biederman
2013-09-12 3:49 ` Tejun Heo
0 siblings, 1 reply; 21+ messages in thread
From: Eric W. Biederman @ 2013-09-12 3:39 UTC (permalink / raw)
To: Tejun Heo; +Cc: gregkh, linux-kernel, kay, netdev, lizefan
Tejun Heo <tj@kernel.org> writes:
> Some internal sysfs functions which take explicit namespace argument
> are weird in that they place the optional @ns in front of @name which
> is contrary to the established convention. This is confusing and
> error-prone especially as @ns and @name may be interchanged without
> causing compilation warning.
>
> Swap the positions of @name and @ns in the following internal
> functions.
Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
@ns is more significant so it should come first.
Where do we have the backwards convention of putting @name first?
Eric
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs
2013-09-12 3:37 ` Eric W. Biederman
@ 2013-09-12 3:47 ` Tejun Heo
0 siblings, 0 replies; 21+ messages in thread
From: Tejun Heo @ 2013-09-12 3:47 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: gregkh, linux-kernel, kay, netdev, lizefan
On Wed, Sep 11, 2013 at 08:37:23PM -0700, Eric W. Biederman wrote:
> At a practical level you probably just want to copy the good parts of
> the structure of sysfs, instead of attempting to share code.
>
> Sharing code is likely to get you into all kinds of problems with short
> term hacks.
What? Are you kidding me? This is one of the most basic principles
which should be followed. The problem is not doing proper modular
design from the get-go. You have it completely backwards.
> > I never really understood why namespace support was added the way it
> > was added. Namespace information is communicated to sysfs via
> > callbacks and back-queries to upper layer, which is a very unusual and
> > weird thing to do when all the involved operations are synchronous.
> > For example, a tagged attribute creation looks like the following.
>
> Then please ask.
I asked the first time around and the answer I got was basically "I
didn't want to go around updating differnet layers and adding stuffing
it in the ops struct is more convenient". Do you have a better reason
now?
> I don't have the time or energy to review these right now, and given the
> sweeping nature of the patches, and the dismissive attitude of the
> original design there is almost at least one stupid bug if not something
> worse.
>
> So until I have the energy to review these.
>
> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
No, you don't get to have a say without actually doing the review.
You sure can ask for more time to review stuff as long as it's
reasonable. Why are things like this still not clear to you? You
have been doing this long enough.
> I am sorry but I don't have time to clean up after any more people
> touching sysfs when the break something. It does look like there are so
> possibly good things going on but..
If you wanna object, do it technically through proper review. That's
how it works.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 7/7] sysfs: @name comes before @ns
2013-09-12 3:39 ` Eric W. Biederman
@ 2013-09-12 3:49 ` Tejun Heo
2013-09-26 22:32 ` Greg KH
0 siblings, 1 reply; 21+ messages in thread
From: Tejun Heo @ 2013-09-12 3:49 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: gregkh, linux-kernel, kay, netdev, lizefan
Hello, Eric.
On Wed, Sep 11, 2013 at 08:39:27PM -0700, Eric W. Biederman wrote:
> @ns is more significant so it should come first.
>
> Where do we have the backwards convention of putting @name first?
Because @ns is optional and you end up with stupid stuff like
sysfs_xxx_ns(@param, @ns, @name)
sysfs_xxx(@param, @name)
You put optional params after the mandatory ones. It may be difficult
to accept for you but @ns is a *clearly* optional thing for sysfs.
--
tejun
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs
2013-09-12 3:38 ` Eric W. Biederman
@ 2013-09-12 4:17 ` Greg KH
0 siblings, 0 replies; 21+ messages in thread
From: Greg KH @ 2013-09-12 4:17 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: Tejun Heo, linux-kernel, kay, netdev, lizefan
On Wed, Sep 11, 2013 at 08:38:32PM -0700, Eric W. Biederman wrote:
> Greg KH <gregkh@linuxfoundation.org> writes:
>
> > On Wed, Sep 11, 2013 at 10:29:02PM -0400, Tejun Heo wrote:
> >> Hello,
> >>
> >> I'll send out multiple patchsets to separate out sysfs from driver
> >> core and kobject. The eventual goal is making sysfs modular enough so
> >> that cgroup can replace its nightmarish cgroupfs implementation which
> >> repeated and worsened all the past mistakes of sysfs. This patchset
> >> is first of the effort and separates out kobject namespace handling
> >> from sysfs.
> >>
> >> I never really understood why namespace support was added the way it
> >> was added.
> >
> > I just took the patches and didn't ask questions :)
>
> Greg you don't get to play dumb you asked questions and required most of
> the current strucuture of the code.
I did? Ok, I'll buy that, but please review this code for what it is,
nice cleanups and a sanity-check for the ns support in sysfs. It saves
100+ lines of code, what's wrong with that?
> The code is convoluted by your request.
I really asked for the existing callback chain to look like that? I did
want ns to be "minimal" in sysfs as I didn't think it was going to be a
good idea, and hoped it would go away. Much like all other kernel
maintainers did for cgroups. Turns out, we were wrong, and now Tejun is
stepping up and fixing it.
So, if I asked for this before, I apologize, I was wrong. Just like I
was wrong about containers and cgroups, and I'll probably be wrong about
something else in the future.
greg k-h
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/7] sysfs: make attr namespace interface less convoluted
2013-09-12 2:29 ` [PATCH 2/7] sysfs: make attr namespace interface less convoluted Tejun Heo
@ 2013-09-12 19:46 ` David Miller
0 siblings, 0 replies; 21+ messages in thread
From: David Miller @ 2013-09-12 19:46 UTC (permalink / raw)
To: tj; +Cc: gregkh, linux-kernel, kay, ebiederm, netdev, lizefan
From: Tejun Heo <tj@kernel.org>
Date: Wed, 11 Sep 2013 22:29:04 -0400
> sysfs ns (namespace) implementation became more convoluted than
> necessary while trying to hide ns information from visible interface.
> The relatively recent attr ns support is a good example.
>
> * attr ns tag is determined by sysfs_ops->namespace() callback while
> dir tag is determined by kobj_type->namespace(). The placement is
> arbitrary.
>
> * Instead of performing operations with explicit ns tag, the namespace
> callback is routed through sysfs_attr_ns(), sysfs_ops->namespace(),
> class_attr_namespace(), class_attr->namespace(). It's not simpler
> in any sense. The only thing this convolution does is traversing
> the whole stack backwards.
>
> The namespace callbacks are unncessary because the operations involved
> are inherently synchronous. The information can be provided in in
> straight-forward top-down direction and reversing that direction is
> unnecessary and against basic design principles.
>
> This backward interface is unnecessarily convoluted and hinders
> properly separating out sysfs from driver model / kobject for proper
> layering. This patch updates attr ns support such that
>
> * sysfs_ops->namespace() and class_attr->namespace() are dropped.
>
> * sysfs_{create|remove}_file_ns(), which take explicit @ns param, are
> added and sysfs_{create|remove}_file() are now simple wrappers
> around the ns aware functions.
>
> * ns handling is dropped from sysfs_chmod_file(). Nobody uses it at
> this point. sysfs_chmod_file_ns() can be added later if necessary.
>
> * Explicit @ns is propagated through class_{create|remove}_file_ns()
> and netdev_class_{create|remove}_file_ns().
>
> * driver/net/bonding which is currently the only user of attr
> namespace is updated to use netdev_class_{create|remove}_file_ns()
> with @bh->net as the ns tag instead of using the namespace callback.
>
> This patch should be an equivalent conversion without any functional
> difference. It makes the code easier to follow, reduces lines of code
> a bit and helps proper separation and layering.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
For networking bits:
Acked-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 7/7] sysfs: @name comes before @ns
2013-09-12 3:49 ` Tejun Heo
@ 2013-09-26 22:32 ` Greg KH
0 siblings, 0 replies; 21+ messages in thread
From: Greg KH @ 2013-09-26 22:32 UTC (permalink / raw)
To: Tejun Heo; +Cc: Eric W. Biederman, linux-kernel, kay, netdev, lizefan
On Wed, Sep 11, 2013 at 11:49:43PM -0400, Tejun Heo wrote:
> Hello, Eric.
>
> On Wed, Sep 11, 2013 at 08:39:27PM -0700, Eric W. Biederman wrote:
> > @ns is more significant so it should come first.
> >
> > Where do we have the backwards convention of putting @name first?
>
> Because @ns is optional and you end up with stupid stuff like
>
> sysfs_xxx_ns(@param, @ns, @name)
> sysfs_xxx(@param, @name)
>
> You put optional params after the mandatory ones. It may be difficult
> to accept for you but @ns is a *clearly* optional thing for sysfs.
Sorry Eric, but I agree with Tejun here, the optional part is @ns, not
name, so it should go at the end.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2013-09-26 22:32 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-12 2:29 [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs Tejun Heo
2013-09-12 2:29 ` [PATCH 1/7] sysfs: drop semicolon from to_sysfs_dirent() definition Tejun Heo
2013-09-12 2:29 ` [PATCH 2/7] sysfs: make attr namespace interface less convoluted Tejun Heo
2013-09-12 19:46 ` David Miller
2013-09-12 2:29 ` [PATCH 3/7] sysfs: remove ktype->namespace() invocations in directory code Tejun Heo
2013-09-12 2:29 ` [PATCH 4/7] sysfs: remove ktype->namespace() invocations in symlink code Tejun Heo
2013-09-12 2:29 ` [PATCH 5/7] sysfs: drop kobj_ns_type handling Tejun Heo
2013-09-12 2:29 ` [PATCH 6/7] sysfs: clean up sysfs_get_dirent() Tejun Heo
2013-09-12 3:22 ` [PATCH v2 " Tejun Heo
2013-09-12 2:29 ` [PATCH 7/7] sysfs: @name comes before @ns Tejun Heo
2013-09-12 3:39 ` Eric W. Biederman
2013-09-12 3:49 ` Tejun Heo
2013-09-26 22:32 ` Greg KH
2013-09-12 3:19 ` [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs Greg KH
2013-09-12 3:23 ` Tejun Heo
2013-09-12 3:33 ` Greg KH
2013-09-12 3:34 ` Tejun Heo
2013-09-12 3:38 ` Eric W. Biederman
2013-09-12 4:17 ` Greg KH
2013-09-12 3:37 ` Eric W. Biederman
2013-09-12 3:47 ` Tejun Heo
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).