Re: [patch 4/7] vfs: mountinfo: add mount peer group ID

[Al Viro <viro@ZenIV.linux.org.uk>]

On Wed, Mar 26, 2008 at 10:11:35PM +0100, Miklos Szeredi wrote:
> +static int mnt_alloc_group_id(struct vfsmount *mnt)
> +{
> +	int res;
> +
> +	WARN_ON(mnt->mnt_group_id != 0);
> + retry:
> +	res = ida_get_new_above(&mnt_group_ida, 1, &mnt->mnt_group_id);
> +	if (res == -EAGAIN) {
> +		if (ida_pre_get(&mnt_group_ida, GFP_KERNEL))
> +			goto retry;
> +		res = -ENOMEM;
> +	}
> +	return res;
> +}
> +

Same comment on goto retry

> +/*
> + * Release a peer group ID
> + */
> +void mnt_release_group_id(struct vfsmount *mnt)
> +{
> +	ida_remove(&mnt_group_ida, mnt->mnt_group_id);
> +	mnt->mnt_group_id = 0;
> +}

Erm...  What if that sucker has zero group ID to start with?  You
do call it that way later.

> +static void cleanup_group_ids(struct vfsmount *mnt, struct vfsmount *end)
> +{
> +	struct vfsmount *p;
> +
> +	for (p = mnt; p != end; p = next_mnt(p, mnt)) {
> +		if (list_empty(&p->mnt_share) && !IS_MNT_SHARED(p))
> +			mnt_release_group_id(p);
> +	}
> +}

I'd make that p->mnt_group_id && !IS_MNT_SHARED(p), actually...

> +static int invent_group_ids(struct vfsmount *mnt, bool recurse)
> +{
> +	struct vfsmount *p;
> +
> +	for (p = mnt; p; p = recurse ? next_mnt(p, mnt) : NULL) {
> +		if (list_empty(&p->mnt_share) && !IS_MNT_SHARED(p)) {
> +			int err = mnt_alloc_group_id(p);
> +			if (err) {
> +				cleanup_group_ids(mnt, p);
> +				return err;
> +			}
> +		}
> +	}

Same comment

> @@ -58,9 +62,9 @@ static int do_make_slave(struct vfsmount
>  		list_splice(&mnt->mnt_slave_list, master->mnt_slave_list.prev);
>  		INIT_LIST_HEAD(&mnt->mnt_slave_list);
>  	} else {
> -		struct list_head *p = &mnt->mnt_slave_list;
> -		while (!list_empty(p)) {
> -                        slave_mnt = list_first_entry(p,
> +		struct list_head *slaves = &mnt->mnt_slave_list;
> +		while (!list_empty(slaves)) {
> +                        slave_mnt = list_first_entry(slaves,
>  					struct vfsmount, mnt_slave);
>  			list_del_init(&slave_mnt->mnt_slave);
>  			slave_mnt->mnt_master = NULL;

What is that renaming doing here?