Hi,
I recently tried to use configfs for configuring (virtual) network
interfaces on top of cfg80211. Here's what I noticed then and in later
thoughts about it.
Note that in this context I'm talking about 'virtual interfaces' but
this has nothing to do with namespace virtualisation, the 'virtual' here
comes from the fact that you have many 'virtual' interfaces associated
with a single wireless card.
(1) it is possible to have
| $ ls /config
| 02-example 02-example
Seems like that should be prohibited when registering the new
configfs subsystem.
(2) why is are there no show/store in struct configfs_attribute? That
leads to complications like this (straight from ocfs2):
struct o2nm_node_attribute {
struct configfs_attribute attr;
ssize_t (*show)(struct o2nm_node *, char *);
ssize_t (*store)(struct o2nm_node *, const char *, size_t);
};
static struct o2nm_node_attribute o2nm_node_attr_num = {
.attr = { .ca_owner = THIS_MODULE,
.ca_name = "num",
.ca_mode = S_IRUGO | S_IWUSR },
.show = o2nm_node_num_read,
.store = o2nm_node_num_write,
};
...
static ssize_t o2nm_node_show(struct config_item *item,
struct configfs_attribute *attr,
char *page)
{
struct o2nm_node *node = to_o2nm_node(item);
struct o2nm_node_attribute *o2nm_node_attr =
container_of(attr, struct o2nm_node_attribute, attr);
ssize_t ret = 0;
if (o2nm_node_attr->show)
ret = o2nm_node_attr->show(node, page);
return ret;
}
I suppose I could ask the same of sysfs, but there it actually seems to
make sense because it only needs to be implemented once per subsystem.
The same is true of configfs, however for configfs one usually has to
implement a new subsystem to get useful functionality...
(3) just thought about the pending/live thing a bit more. For one I
notice that it is not implemented, which is sad because I could really
use it well here. Secondly, and more importantly, I think the concept is
slightly flawed.
If I have virtual devices represented in configfs, they are all
net_devices at their core, of course. Assuming they are below
/config/cfg80211/wiphy0/, say eth0 is created as pending/eth0. Then, you
move it to live/eth0 at which point the netdevice is allocated and
registered (if it fails due to name collision you need to chose another
name by renaming it in pending).
This is all great, but say then I want to change a few parameters of the
interface. So I move it to pending again. At this point, we run into
problems. We can either (a) remove the net_device or (b) keep it live.
Both have problems. Case (a) would obviously be correct from a configfs
point of view, but not really usable. Tearing down the net_device etc.
isn't feasible for just changing the SSID for example... keeping it live
doesn't really work either though, if someone notices that 'eth0' exists
and belongs to wiphy0 (say through netlink) he would then proceed to
look at /config/cfg80211/wiphy0/live/eth0 which doesn't exist! This
problem happens because configfs is designed to be the primary namespace
for things which in the case of networking interfaces isn't true.
Hence, I think it should be slightly redesigned to allow for another
case, namely hardlinking the directory from live into pending (make sure
name stays the same) at which point it isn't removed. Then, moving it
from live to pending would be taken as an indication to actually remove
the net_device associated with it, and linking it just as a
reconfiguration request.
Then, moving it from pending to live over the existing one would
actually update the configuration.
Does that sound sane?
Thanks,
Johannes
On Tue, Aug 29, 2006 at 06:14:30PM +0200, Johannes Berg wrote:
> I recently tried to use configfs for configuring (virtual) network
> interfaces on top of cfg80211. Here's what I noticed then and in later
> thoughts about it.
Thanks! I always appreciate thoughts and comments.
> (1) it is possible to have
>
> | $ ls /config
> | 02-example 02-example
>
> Seems like that should be prohibited when registering the new
> configfs subsystem.
Hmm, yeah, this is a bug. I've added
http://oss.oracle.com/bugzilla/show_bug.cgi?id=755. Thanks for
reporting this!
> (2) why is are there no show/store in struct configfs_attribute? That
> leads to complications like this (straight from ocfs2):
>
> struct o2nm_node_attribute {
> struct configfs_attribute attr;
> ssize_t (*show)(struct o2nm_node *, char *);
> ssize_t (*store)(struct o2nm_node *, const char *, size_t);
> };
>
> static struct o2nm_node_attribute o2nm_node_attr_num = {
> .attr = { .ca_owner = THIS_MODULE,
> .ca_name = "num",
> .ca_mode = S_IRUGO | S_IWUSR },
> .show = o2nm_node_num_read,
> .store = o2nm_node_num_write,
> };
>
> ...
>
> I suppose I could ask the same of sysfs, but there it actually seems to
> make sense because it only needs to be implemented once per subsystem.
> The same is true of configfs, however for configfs one usually has to
> implement a new subsystem to get useful functionality...
I did get this straight from sysfs. You are right, there is
less sharing of such things in configfs compared to sysfs. Here's the
thing: this scheme allows the generic code (the ->show() and ->store()
methods) to take struct configfs_attribute, but the actual worker
functions take context appropriate objects, such a struct o2nm_node in
the example you placed. Without this indirection, every ->show() would
have to do its own conversion from configfs_attribute to the context
appropriate structures.
> (3) just thought about the pending/live thing a bit more. For one I
> notice that it is not implemented, which is sad because I could really
> use it well here. Secondly, and more importantly, I think the concept is
> slightly flawed.
Yeah, I'm sorry I haven't gotten to it yet.
> If I have virtual devices represented in configfs, they are all
> net_devices at their core, of course. Assuming they are below
> /config/cfg80211/wiphy0/, say eth0 is created as pending/eth0. Then, you
> move it to live/eth0 at which point the netdevice is allocated and
> registered (if it fails due to name collision you need to chose another
> name by renaming it in pending).
Also note that you can fail the callback in your driver,
preventing the rename(2) -- eg, if a particular option needs to be set
but isn't, etc.
> This is all great, but say then I want to change a few parameters of the
> interface. So I move it to pending again. At this point, we run into
> problems. We can either (a) remove the net_device or (b) keep it live.
The plan is (c). Build a new one in pending, and rename(2) it
on top of the current one.
> Hence, I think it should be slightly redesigned to allow for another
> case, namely hardlinking the directory from live into pending (make sure
> name stays the same) at which point it isn't removed. Then, moving it
> from live to pending would be taken as an indication to actually remove
> the net_device associated with it, and linking it just as a
> reconfiguration request.
Linux doesn't support directory hardlinks, for good reason.
Joel
--
Life's Little Instruction Book #313
"Never underestimate the power of love."
Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
On Tue, 2006-08-29 at 09:31 -0700, Joel Becker wrote:
> I did get this straight from sysfs. You are right, there is
> less sharing of such things in configfs compared to sysfs. Here's the
> thing: this scheme allows the generic code (the ->show() and ->store()
> methods) to take struct configfs_attribute, but the actual worker
> functions take context appropriate objects, such a struct o2nm_node in
> the example you placed. Without this indirection, every ->show() would
> have to do its own conversion from configfs_attribute to the context
> appropriate structures.
Yes, I know this, and I appreciate that sysfs does this generically in
order to get better type checking etc. Maybe it'd be possible to also
give each node a void* data pointer that get's passed around to the
store/show callbacks and hence it doesn't need to do any container_of
conversion but rather cast that data pointer. I'm not really sure, but I
think that the setup required for configfs is a bit too much since one
is essentially implementing what would in sysfs be a subsystem for every
node time.
> > If I have virtual devices represented in configfs, they are all
> > net_devices at their core, of course. Assuming they are below
> > /config/cfg80211/wiphy0/, say eth0 is created as pending/eth0. Then, you
> > move it to live/eth0 at which point the netdevice is allocated and
> > registered (if it fails due to name collision you need to chose another
> > name by renaming it in pending).
>
> Also note that you can fail the callback in your driver,
> preventing the rename(2) -- eg, if a particular option needs to be set
> but isn't, etc.
Yes, I know that.
> > This is all great, but say then I want to change a few parameters of the
> > interface. So I move it to pending again. At this point, we run into
> > problems. We can either (a) remove the net_device or (b) keep it live.
>
> The plan is (c). Build a new one in pending, and rename(2) it
> on top of the current one.
Ah, will that new one in pending start out with the same set of
parameters as the one in live? I guess there could be a callback when
something is added to pending so that it can be filled in to look the
same.
Another question. For this application we'll probably need something
like
/config/cfg80211/wiphy0/eth0/type <- type of interface: sta, monitor,...
/config/cfg80211/wiphy0/eth0/sta/ <- sta attributes, e.g. 'ssid'
but if the type changes, those sta attributes ought to disappear. It
seems that I should be able to do that, but I'm not sure how it would be
done in pending. That is, when in pending I have an object and someone
sets the 'sta' type then the 'sta' attributes directory must show up.
Thanks,
johannes
On Tue, Aug 29, 2006 at 09:31:36AM -0700, Joel Becker wrote:
> On Tue, Aug 29, 2006 at 06:14:30PM +0200, Johannes Berg wrote:
> > (1) it is possible to have
> >
> > | $ ls /config
> > | 02-example 02-example
> >
> > Seems like that should be prohibited when registering the new
> > configfs subsystem.
>
> Hmm, yeah, this is a bug. I've added
> http://oss.oracle.com/bugzilla/show_bug.cgi?id=755. Thanks for
> reporting this!
Here's a fix, if you can try it:
diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index df02545..816e8ef 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -86,6 +86,32 @@ static struct configfs_dirent *configfs_
return sd;
}
+/*
+ *
+ * Return -EEXIST if there is already a configfs element with the same
+ * name for the same parent.
+ *
+ * called with parent inode's i_mutex held
+ */
+int configfs_dirent_exists(struct configfs_dirent *parent_sd,
+ const unsigned char *new)
+{
+ struct configfs_dirent * sd;
+
+ list_for_each_entry(sd, &parent_sd->s_children, s_sibling) {
+ if (sd->s_element) {
+ const unsigned char *existing = configfs_get_name(sd);
+ if (strcmp(existing, new))
+ continue;
+ else
+ return -EEXIST;
+ }
+ }
+
+ return 0;
+}
+
+
int configfs_make_dirent(struct configfs_dirent * parent_sd,
struct dentry * dentry, void * element,
umode_t mode, int type)
@@ -136,8 +162,10 @@ static int create_dir(struct config_item
int error;
umode_t mode = S_IFDIR| S_IRWXU | S_IRUGO | S_IXUGO;
- error = configfs_make_dirent(p->d_fsdata, d, k, mode,
- CONFIGFS_DIR);
+ error = configfs_dirent_exists(p->d_fsdata, d->d_name.name);
+ if (!error)
+ error = configfs_make_dirent(p->d_fsdata, d, k, mode,
+ CONFIGFS_DIR);
if (!error) {
error = configfs_create(d, mode, init_dir);
if (!error) {
--
"Not everything that can be counted counts, and not everything
that counts can be counted."
- Albert Einstein
Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
On Wed, 2006-08-30 at 19:16 -0700, Joel Becker wrote:
> Here's a fix, if you can try it:
Looks good, I'll see if I can give it a spin tonight.
johannes