Re: [PATCH] [ConfigFS]: Add structconfigfs_item_operations->check_link()

[Louis Rilling]

Hi,

On 03/04/09  1:25 -0700, Nicholas A. Bellinger wrote:
> Hi Joel and co,
> 
> This patch adds struct configfs_item_operations->check_link() and
> changes fs/configfs/symlink.c:configfs_unlink() so that
> when (*check_link) is present, an ConfigFS unlink will fail, based upon
> input by said symlinked struct config_item *parent_item.
> 
> If a non zero return is returned from (*check_link), said non zero value is
> expected to use include/asm-generic/errno* values, and the failure is returned
> to userspace via the unlink(2) system call.
> 
> Please consider this patch for v2.6.30.  It requires no changes to existing consumers
> of ConfigFS like fs/ocfs2, and I have tested it with running LIO-Target v3.0 code.
> 
> Many thanks for your most valuable of time,

I can't judge the actual need for that since I don't really know your usecase
(I've seen the second patch). However check_link() without target_item as
parameter looks a bit restrictive for no valuable reason.

See inline for a concern about the error returned.
Other than that, the patch looks ok.

Louis

> 
> --nab
> 
> Signed-off-by: Nicholas A. Bellinger <nab@xxxxxxxxxxxxxxx>
> ---
>  fs/configfs/symlink.c    |   13 +++++++++++++
>  include/linux/configfs.h |    1 +
>  2 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
> index 932a92b..a5dede6 100644
> --- a/fs/configfs/symlink.c
> +++ b/fs/configfs/symlink.c
> @@ -202,6 +202,19 @@ int configfs_unlink(struct inode *dir, struct dentry *dentry)
>  	parent_item = configfs_get_config_item(dentry->d_parent);
>  	type = parent_item->ci_type;
>  
> +	/*
> +	 * See if the underlying struct config_item has dependent
> +	 * symlinks, and should return -EACCES here.
> +	 */

I think that -EPERM is more natural than -EACCES. check_link() actually checks
that the operation is permitted.

> +	if (type && type->ct_item_ops &&
> +	    type->ct_item_ops->check_link) {
> +		ret = type->ct_item_ops->check_link(parent_item);
> +		if (ret != 0) {
> +			config_item_put(parent_item);
> +			goto out;
> +		}
> +	}
> +
>  	spin_lock(&configfs_dirent_lock);
>  	list_del_init(&sd->s_sibling);
>  	spin_unlock(&configfs_dirent_lock);
> diff --git a/include/linux/configfs.h b/include/linux/configfs.h
> index 7f62777..b026f16 100644
> --- a/include/linux/configfs.h
> +++ b/include/linux/configfs.h
> @@ -226,6 +226,7 @@ struct configfs_item_operations {
>  	ssize_t	(*show_attribute)(struct config_item *, struct configfs_attribute *,char *);
>  	ssize_t	(*store_attribute)(struct config_item *,struct configfs_attribute *,const char *, size_t);
>  	int (*allow_link)(struct config_item *src, struct config_item *target);
> +	int (*check_link)(struct config_item *src);
>  	int (*drop_link)(struct config_item *src, struct config_item *target);
>  };
>  
> -- 
> 1.5.4.1
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Dr Louis Rilling			Kerlabs
Skype: louis.rilling			Batiment Germanium
Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
http://www.kerlabs.com/			35700 Rennes

Attachment: signature.asc
Description: Digital signature