Quoting Kirill Korotaev (dev@sw.ru):
> Cedric Le Goater wrote:
> > all,
> > 
> > 'pspace' sounds wrong when you know about the other namespaces :
> > 
> > struct nsproxy {
> > 	atomic_t count;
> > 	spinlock_t nslock;
> > 	struct uts_namespace *uts_ns;
> > 	struct ipc_namespace *ipc_ns;
> > 	struct user_namespace *user_ns;
> > 	struct namespace *namespace;
> > };
> > 
> > 'proc_namespace' might be confusing, what about 'task_namespace' ?
> yes, I also wanted to point to this, but probably missed in a hurry.
> task_ns/task_namespace looks fine, doesn't it?

I still think pid_ns is more correct, but task_ns sounds nicer and is
at any rate unambiguous.

> > 'namespace' should probably be renamed to something like 'mnt_namespace' ?
> struct: mnt_namespace
> fields: mnt_ns
> 
> is the patch below ok for you?

Based on a cursory glance, looks good to me.

-serge

> 
> ---
> 
> --- ./fs/afs/mntpt.c.mntr	2006-07-14 19:08:29.000000000 +0400
> +++ ./fs/afs/mntpt.c	2006-09-07 18:47:27.000000000 +0400
> @@ -18,7 +18,7 @@
>  #include <linux/pagemap.h>
>  #include <linux/mount.h>
>  #include <linux/namei.h>
> -#include <linux/namespace.h>
> +#include <linux/mnt_namespace.h>
>  #include "super.h"
>  #include "cell.h"
>  #include "volume.h"
> --- ./fs/namespace.c.mntr	2006-07-14 19:11:05.000000000 +0400
> +++ ./fs/namespace.c	2006-09-07 19:28:05.000000000 +0400
> @@ -20,7 +20,7 @@
>  #include <linux/module.h>
>  #include <linux/sysfs.h>
>  #include <linux/seq_file.h>
> -#include <linux/namespace.h>
> +#include <linux/mnt_namespace.h>
>  #include <linux/namei.h>
>  #include <linux/security.h>
>  #include <linux/mount.h>
> @@ -134,10 +134,10 @@ struct vfsmount *lookup_mnt(struct vfsmo
>  
>  static inline int check_mnt(struct vfsmount *mnt)
>  {
> -	return mnt->mnt_namespace == current->nsproxy->namespace;
> +	return mnt->mnt_ns == current->nsproxy->mnt_ns;
>  }
>  
> -static void touch_namespace(struct namespace *ns)
> +static void touch_mnt_namespace(struct mnt_namespace *ns)
>  {
>  	if (ns) {
>  		ns->event = ++event;
> @@ -145,7 +145,7 @@ static void touch_namespace(struct names
>  	}
>  }
>  
> -static void __touch_namespace(struct namespace *ns)
> +static void __touch_mnt_namespace(struct mnt_namespace *ns)
>  {
>  	if (ns && ns->event != event) {
>  		ns->event = event;
> @@ -188,19 +188,19 @@ static void commit_tree(struct vfsmount 
>  	struct vfsmount *parent = mnt->mnt_parent;
>  	struct vfsmount *m;
>  	LIST_HEAD(head);
> -	struct namespace *n = parent->mnt_namespace;
> +	struct mnt_namespace *n = parent->mnt_ns;
>  
>  	BUG_ON(parent == mnt);
>  
>  	list_add_tail(&head, &mnt->mnt_list);
>  	list_for_each_entry(m, &head, mnt_list)
> -		m->mnt_namespace = n;
> +		m->mnt_ns = n;
>  	list_splice(&head, n->list.prev);
>  
>  	list_add_tail(&mnt->mnt_hash, mount_hashtable +
>  				hash(parent, mnt->mnt_mountpoint));
>  	list_add_tail(&mnt->mnt_child, &parent->mnt_mounts);
> -	touch_namespace(n);
> +	touch_mnt_namespace(n);
>  }
>  
>  static struct vfsmount *next_mnt(struct vfsmount *p, struct vfsmount *root)
> @@ -321,7 +321,7 @@ EXPORT_SYMBOL(mnt_unpin);
>  /* iterator */
>  static void *m_start(struct seq_file *m, loff_t *pos)
>  {
> -	struct namespace *n = m->private;
> +	struct mnt_namespace *n = m->private;
>  	struct list_head *p;
>  	loff_t l = *pos;
>  
> @@ -334,7 +334,7 @@ static void *m_start(struct seq_file *m,
>  
>  static void *m_next(struct seq_file *m, void *v, loff_t *pos)
>  {
> -	struct namespace *n = m->private;
> +	struct mnt_namespace *n = m->private;
>  	struct list_head *p = ((struct vfsmount *)v)->mnt_list.next;
>  	(*pos)++;
>  	return p == &n->list ? NULL : list_entry(p, struct vfsmount, mnt_list);
> @@ -527,8 +527,8 @@ void umount_tree(struct vfsmount *mnt, i
>  	list_for_each_entry(p, kill, mnt_hash) {
>  		list_del_init(&p->mnt_expire);
>  		list_del_init(&p->mnt_list);
> -		__touch_namespace(p->mnt_namespace);
> -		p->mnt_namespace = NULL;
> +		__touch_mnt_namespace(p->mnt_ns);
> +		p->mnt_ns = NULL;
>  		list_del_init(&p->mnt_child);
>  		if (p->mnt_parent != p)
>  			p->mnt_mountpoint->d_mounted--;
> @@ -831,7 +831,7 @@ static int attach_recursive_mnt(struct v
>  	if (parent_nd) {
>  		detach_mnt(source_mnt, parent_nd);
>  		attach_mnt(source_mnt, nd);
> -		touch_namespace(current->nsproxy->namespace);
> +		touch_mnt_namespace(current->nsproxy->mnt_ns);
>  	} else {
>  		mnt_set_mountpoint(dest_mnt, dest_dentry, source_mnt);
>  		commit_tree(source_mnt);
> @@ -1146,9 +1146,9 @@ static void expire_mount(struct vfsmount
>  	 */
>  	if (!propagate_mount_busy(mnt, 2)) {
>  		/* delete from the namespace */
> -		touch_namespace(mnt->mnt_namespace);
> +		touch_mnt_namespace(mnt->mnt_ns);
>  		list_del_init(&mnt->mnt_list);
> -		mnt->mnt_namespace = NULL;
> +		mnt->mnt_ns = NULL;
>  		umount_tree(mnt, 1, umounts);
>  		spin_unlock(&vfsmount_lock);
>  	} else {
> @@ -1169,7 +1169,7 @@ static void expire_mount(struct vfsmount
>   */
>  static void expire_mount_list(struct list_head *graveyard, struct list_head *mounts)
>  {
> -	struct namespace *namespace;
> +	struct mnt_namespace *ns;
>  	struct vfsmount *mnt;
>  
>  	while (!list_empty(graveyard)) {
> @@ -1179,10 +1179,10 @@ static void expire_mount_list(struct lis
>  
>  		/* don't do anything if the namespace is dead - all the
>  		 * vfsmounts from it are going away anyway */
> -		namespace = mnt->mnt_namespace;
> -		if (!namespace || !namespace->root)
> +		ns = mnt->mnt_ns;
> +		if (!ns || !ns->root)
>  			continue;
> -		get_namespace(namespace);
> +		get_mnt_ns(ns);
>  
>  		spin_unlock(&vfsmount_lock);
>  		down_write(&namespace_sem);
> @@ -1190,7 +1190,7 @@ static void expire_mount_list(struct lis
>  		up_write(&namespace_sem);
>  		release_mounts(&umounts);
>  		mntput(mnt);
> -		put_namespace(namespace);
> +		put_mnt_ns(ns);
>  		spin_lock(&vfsmount_lock);
>  	}
>  }
> @@ -1440,14 +1440,15 @@ dput_out:
>   * Allocate a new namespace structure and populate it with contents
>   * copied from the namespace of the passed in task structure.
>   */
> -struct namespace *dup_namespace(struct task_struct *tsk, struct fs_struct *fs)
> +struct mnt_namespace *dup_mnt_ns(struct task_struct *tsk,
> +		struct fs_struct *fs)
>  {
> -	struct namespace *namespace = tsk->nsproxy->namespace;
> -	struct namespace *new_ns;
> +	struct mnt_namespace *mnt_ns = tsk->nsproxy->mnt_ns;
> +	struct mnt_namespace *new_ns;
>  	struct vfsmount *rootmnt = NULL, *pwdmnt = NULL, *altrootmnt = NULL;
>  	struct vfsmount *p, *q;
>  
> -	new_ns = kmalloc(sizeof(struct namespace), GFP_KERNEL);
> +	new_ns = kmalloc(sizeof(struct mnt_namespace), GFP_KERNEL);
>  	if (!new_ns)
>  		return NULL;
>  
> @@ -1458,7 +1459,7 @@ struct namespace *dup_namespace(struct t
>  
>  	down_write(&namespace_sem);
>  	/* First pass: copy the tree topology */
> -	new_ns->root = copy_tree(namespace->root, namespace->root->mnt_root,
> +	new_ns->root = copy_tree(mnt_ns->root, mnt_ns->root->mnt_root,
>  					CL_COPY_ALL | CL_EXPIRE);
>  	if (!new_ns->root) {
>  		up_write(&namespace_sem);
> @@ -1474,10 +1475,10 @@ struct namespace *dup_namespace(struct t
>  	 * as belonging to new namespace.  We have already acquired a private
>  	 * fs_struct, so tsk->fs->lock is not needed.
>  	 */
> -	p = namespace->root;
> +	p = mnt_ns->root;
>  	q = new_ns->root;
>  	while (p) {
> -		q->mnt_namespace = new_ns;
> +		q->mnt_ns = new_ns;
>  		if (fs) {
>  			if (p == fs->rootmnt) {
>  				rootmnt = p;
> @@ -1492,7 +1493,7 @@ struct namespace *dup_namespace(struct t
>  				fs->altrootmnt = mntget(q);
>  			}
>  		}
> -		p = next_mnt(p, namespace->root);
> +		p = next_mnt(p, mnt_ns->root);
>  		q = next_mnt(q, new_ns->root);
>  	}
>  	up_write(&namespace_sem);
> @@ -1507,16 +1508,16 @@ struct namespace *dup_namespace(struct t
>  	return new_ns;
>  }
>  
> -int copy_namespace(int flags, struct task_struct *tsk)
> +int copy_mnt_ns(int flags, struct task_struct *tsk)
>  {
> -	struct namespace *namespace = tsk->nsproxy->namespace;
> -	struct namespace *new_ns;
> +	struct mnt_namespace *ns = tsk->nsproxy->mnt_ns;
> +	struct mnt_namespace *new_ns;
>  	int err = 0;
>  
> -	if (!namespace)
> +	if (!ns)
>  		return 0;
>  
> -	get_namespace(namespace);
> +	get_mnt_ns(ns);
>  
>  	if (!(flags & CLONE_NEWNS))
>  		return 0;
> @@ -1526,16 +1527,16 @@ int copy_namespace(int flags, struct tas
>  		goto out;
>  	}
>  
> -	new_ns = dup_namespace(tsk, tsk->fs);
> +	new_ns = dup_mnt_ns(tsk, tsk->fs);
>  	if (!new_ns) {
>  		err = -ENOMEM;
>  		goto out;
>  	}
>  
> -	tsk->nsproxy->namespace = new_ns;
> +	tsk->nsproxy->mnt_ns = new_ns;
>  
>  out:
> -	put_namespace(namespace);
> +	put_mnt_ns(ns);
>  	return err;
>  }
>  
> @@ -1755,7 +1756,7 @@ asmlinkage l

On Thu, Sep 07, 2006 at 10:41:52AM -0500, Serge E. Hallyn wrote:
> Quoting Kirill Korotaev (dev@sw.ru):
> > Cedric Le Goater wrote:
> > > all,
> > > 
> > > 'pspace' sounds wrong when you know about the other namespaces :
> > > 
> > > struct nsproxy {
> > > 	atomic_t count;
> > > 	spinlock_t nslock;
> > > 	struct uts_namespace *uts_ns;
> > > 	struct ipc_namespace *ipc_ns;
> > > 	struct user_namespace *user_ns;
> > > 	struct namespace *namespace;
> > > };
> > > 
> > > 'proc_namespace' might be confusing, what about 'task_namespace' ?
> > yes, I also wanted to point to this, but probably missed in a hurry.
> > task_ns/task_namespace looks fine, doesn't it?
> 
> I still think pid_ns is more correct, but task_ns sounds nicer and is
> at any rate unambiguous.

I'd prefer pid_n{s,amespace} too, as it resembles the
function more closely ...

best,
Herbert

> > > 'namespace' should probably be renamed to something like 'mnt_namespace' ?
> > struct: mnt_namespace
> > fields: mnt_ns
> > 
> > is the patch below ok for you?
> 
> Based on a cursory glance, looks good to me.
> 
> -serge
> 
> > 
> > ---
> > 
> > --- ./fs/afs/mntpt.c.mntr	2006-07-14 19:08:29.000000000 +0400
> > +++ ./fs/afs/mntpt.c	2006-09-07 18:47:27.000000000 +0400
> > @@ -18,7 +18,7 @@
> >  #include <linux/pagemap.h>
> >  #include <linux/mount.h>
> >  #include <linux/namei.h>
> > -#include <linux/namespace.h>
> > +#include <linux/mnt_namespace.h>
> >  #include "super.h"
> >  #include "cell.h"
> >  #include "volume.h"
> > --- ./fs/namespace.c.mntr	2006-07-14 19:11:05.000000000 +0400
> > +++ ./fs/namespace.c	2006-09-07 19:28:05.000000000 +0400
> > @@ -20,7 +20,7 @@
> >  #include <linux/module.h>
> >  #include <linux/sysfs.h>
> >  #include <linux/seq_file.h>
> > -#include <linux/namespace.h>
> > +#include <linux/mnt_namespace.h>
> >  #include <linux/namei.h>
> >  #include <linux/security.h>
> >  #include <linux/mount.h>
> > @@ -134,10 +134,10 @@ struct vfsmount *lookup_mnt(struct vfsmo
> >  
> >  static inline int check_mnt(struct vfsmount *mnt)
> >  {
> > -	return mnt->mnt_namespace == current->nsproxy->namespace;
> > +	return mnt->mnt_ns == current->nsproxy->mnt_ns;
> >  }
> >  
> > -static void touch_namespace(struct namespace *ns)
> > +static void touch_mnt_namespace(struct mnt_namespace *ns)
> >  {
> >  	if (ns) {
> >  		ns->event = ++event;
> > @@ -145,7 +145,7 @@ static void touch_namespace(struct names
> >  	}
> >  }
> >  
> > -static void __touch_namespace(struct namespace *ns)
> > +static void __touch_mnt_namespace(struct mnt_namespace *ns)
> >  {
> >  	if (ns && ns->event != event) {
> >  		ns->event = event;
> > @@ -188,19 +188,19 @@ static void commit_tree(struct vfsmount 
> >  	struct vfsmount *parent = mnt->mnt_parent;
> >  	struct vfsmount *m;
> >  	LIST_HEAD(head);
> > -	struct namespace *n = parent->mnt_namespace;
> > +	struct mnt_namespace *n = parent->mnt_ns;
> >  
> >  	BUG_ON(parent == mnt);
> >  
> >  	list_add_tail(&head, &mnt->mnt_list);
> >  	list_for_each_entry(m, &head, mnt_list)
> > -		m->mnt_namespace = n;
> > +		m->mnt_ns = n;
> >  	list_splice(&head, n->list.prev);
> >  
> >  	list_add_tail(&mnt->mnt_hash, mount_hashtable +
> >  				hash(parent, mnt->mnt_mountpoint));
> >  	list_add_tail(&mnt->mnt_child, &parent->mnt_mounts);
> > -	touch_namespace(n);
> > +	touch_mnt_namespace(n);
> >  }
> >  
> >  static struct vfsmount *next_mnt(struct vfsmount *p, struct vfsmount *root)
> > @@ -321,7 +321,7 @@ EXPORT_SYMBOL(mnt_unpin);
> >  /* iterator */
> >  static void *m_start(struct seq_file *m, loff_t *pos)
> >  {
> > -	struct namespace *n = m->private;
> > +	struct mnt_namespace *n = m->private;
> >  	struct list_head *p;
> >  	loff_t l = *pos;
> >  
> > @@ -334,7 +334,7 @@ static void *m_start(struct seq_file *m,
> >  
> >  static void *m_next(struct seq_file *m, void *v, loff_t *pos)
> >  {
> > -	struct namespace *n = m->private;
> > +	struct mnt_namespace *n = m->private;
> >  	struct list_head *p = ((struct vfsmount *)v)->mnt_list.next;
> >  	(*pos)++;
> >  	return p == &n->list ? NULL : list_entry(p, struct vfsmount, mnt_list);
> > @@ -527,8 +527,8 @@ void umount_tree(struct vfsmount *mnt, i
> >  	list_for_each_entry(p, kill, mnt_hash) {
> >  		list_del_init(&p->mnt_expire);
> >  		list_del_init(&p->mnt_list);
> > -		__touch_namespace(p->mnt_namespace);
> > -		p->mnt_namespace = NULL;
> > +		__touch_mnt_namespace(p->mnt_ns);
> > +		p->mnt_ns = NULL;
> >  		list_del_init(&p->mnt_child);
> >  		if (p->mnt_parent != p)
> >  			p->mnt_mountpoint->d_mounted--;
> > @@ -831,7 +831,7 @@ static int attach_recursive_mnt(struct v
> >  	if (parent_nd) {
> >  		detach_mnt(source_mnt, parent_nd);
> >  		attach_mnt(source_mnt, nd);
> > -		touch_namespace(current->nsproxy->namespace);
> > +		touch_mnt_namespace(current->nsproxy->mnt_ns);
> >  	} else {
> >  		mnt_set_mountpoint(dest_mnt, dest_dentry, source_mnt);
> >  		commit_tree(source_mnt);
> > @@ -1146,9 +1146,9 @@ static void expire_mount(struct vfsmount
> >  	 */
> >  	if (!propagate_mount_busy(mnt, 2)) {
> >  		/* delete from the namespace */
> > -		touch_namespace(mnt->mnt_namespace);
> > +		touch_mnt_namespace(mnt->mnt_ns);
> >  		list_del_init(&mnt->mnt_list);
> > -		mnt->mnt_namespace = NULL;
> > +		mnt->mnt_ns = NULL;
> >  		umount_tree(mnt, 1, umounts);
> >  		spin_unlock(&vfsmount_lock);
> >  	} else {
> > @@ -1169,7 +1169,7 @@ static void expire_mount(struct vfsmount
> >   */
> >  static void expire_mount_list(struct list_head *graveyard, struct list_head *mounts)
> >  {
> > -	struct namespace *namespace;
> > +	struct mnt_namespace *ns;
> >  	struct vfsmount *mnt;
> >  
> >  	while (!list_empty(graveyard)) {
> > @@ -1179,10 +1179,10 @@ static void expire_mount_list(struct lis
> >  
> >  		/* don't do anything if the namespace is dead - all the
> >  		 * vfsmounts from it are going away anyway */
> > -		namespace = mnt->mnt_namespace;
> > -		if (!namespace || !namespace->root)
> > +		ns = mnt->mnt_ns;
> > +		if (!ns || !ns->root)
> >  			continue;
> > -		get_namespace(namespace);
> > +		get_mnt_ns(ns);
> >  
> >  		spin_unlock(&vfsmount_lock);
> >  		down_write(&namespace_sem);
> > @@ -1190,7 +1190,7 @@ static void expire_mount_list(struct lis
> >  		up_write(&namespace_sem);
> >  		release_mounts(&umounts);
> >  		mntput(mnt);
> > -		put_namespace(namespace);
> > +		put_mnt_ns(ns);
> >  		spin_lock(&vfsmount_lock);
> >  	}
> >  }
> > @@ -1440,14 +1440,15 @@ dput_out:
> >   * Allocate a new namespace structure and populate it with contents
> >   * copied from the namespace of the passed in task structure.
> >   */
> > -struct namespace *dup_namespace(struct task_struct *tsk, struct fs_struct *fs)
> > +struct mnt_namespace *dup_mnt_ns(struct task_struct *tsk,
> > +		struct fs_struct *fs)
> >  {
> > -	struct namespace *namespace = tsk->nsproxy->namespace;
> > -	struct namespace *new_ns;
> > +	struct mnt_namespace *mnt_ns = tsk->nsproxy->mnt_ns;
> > +	struct mnt_namespace *new_ns;
> >  	struct vfsmount *rootmnt = NULL, *pwdmnt = NULL, *altrootmnt = NULL;
> >  	struct vfsmount *p, *q;
> >  
> > -	new_ns = kmalloc(sizeof(struct namespace), GFP_KERNEL);
> > +	new_ns = kmalloc(sizeof(struct mnt_namespace), GFP_KERNEL);
> >  	if (!new_ns)
> >  		return NULL;
> >  
> > @@ -1458,7 +1459,7 @@ struct namespace *dup_namespace(struct t
> >  
> >  	down_write(&namespace_sem);
> >  	/* First pass: copy the tree topology */
> > -	new_ns->root = copy_tree(namespace->root, namespace->root->mnt_root,
> > +	new_ns->root = copy_tree(mnt_ns->root, mnt_ns->root->mnt_root,
> >  					CL_COPY_ALL | CL_EXPIRE);
> >  	if (!new_ns->root) {
> >  		up_write(&namespace_sem);
> > @@ -1474,10 +1475,10 @@ struct namespace *dup_namespace(struct t
> >  	 * as belonging to new namespace.  We have already acquired a private
> >  	 * fs_struct, so tsk->fs->lock is not needed.
> >  	 */
> > -	p = namespace->root;
> > +	p = mnt_ns->root;
> >  	q = new_ns->root;
> >  	while (p) {
> > -		q->mnt_namespace = new_ns;
> > +		q->mnt_ns = new_ns;
> >  		if (fs) {
> >  			if (p == fs->rootmnt) {
> >  				rootmnt = p;
> > @@ -1492,7 +1493,7 @@ struct namespace *dup_namespace(struct t
> >  				fs->altrootmnt = mntget(q);
> >  			}
> >  		}
> > -		p = next_mnt(p,

Serge E. Hallyn wrote:
> Quoting Kirill Korotaev (dev@sw.ru):
> 
>>Cedric Le Goater wrote:
>>
>>>all,
>>>
>>>'pspace' sounds wrong when you know about the other namespaces :
>>>
>>>struct nsproxy {
>>>	atomic_t count;
>>>	spinlock_t nslock;
>>>	struct uts_namespace *uts_ns;
>>>	struct ipc_namespace *ipc_ns;
>>>	struct user_namespace *user_ns;
>>>	struct namespace *namespace;
>>>};
>>>
>>>'proc_namespace' might be confusing, what about 'task_namespace' ?
>>
>>yes, I also wanted to point to this, but probably missed in a hurry.
>>task_ns/task_namespace looks fine, doesn't it?
> 
> 
> I still think pid_ns is more correct, but task_ns sounds nicer and is
> at any rate unambiguous.
I'm pretty sure the further we go the more things it will incorporate
(not related to pids :) ).

>>>'namespace' should probably be renamed to something like 'mnt_namespace' ?
>>
>>struct: mnt_namespace
>>fields: mnt_ns
>>
>>is the patch below ok for you?
> 
> 
> Based on a cursory glance, looks good to me.
thanks!

Kirill
_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers