Re: [PATCH -V5 2/8] vfs: Add name to file handle conversion support

["Aneesh Kumar K. V" <aneesh.kumar@linux.vnet.ibm.com>]

On Tue, 27 Apr 2010 11:19:49 +1000, Neil Brown <neilb@suse.de> wrote:
> On Mon, 26 Apr 2010 23:04:00 +0530
> "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> wrote:
> 
> > Acked-by: Serge Hallyn <serue@us.ibm.com>
> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> > ---
> >  fs/open.c          |  100 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/fs.h |   17 +++++++++
> >  2 files changed, 117 insertions(+), 0 deletions(-)
> > 
> > diff --git a/fs/open.c b/fs/open.c
> > index 74e5cd9..e9aae5c 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -30,6 +30,8 @@
> >  #include <linux/falloc.h>
> >  #include <linux/fs_struct.h>
> >  #include <linux/ima.h>
> > +#include <linux/exportfs.h>
> > +#include <linux/mnt_namespace.h>
> >  
> >  #include "internal.h"
> >  
> > @@ -1206,3 +1208,101 @@ int nonseekable_open(struct inode *inode, struct file *filp)
> >  }
> >  
> >  EXPORT_SYMBOL(nonseekable_open);
> > +
> > +/* limit the handle size to some value */
> > +#define MAX_HANDLE_SZ 4096
> > +static long do_sys_name_to_handle(struct path *path,
> > +			struct file_handle __user *ufh)
> > +{
> > +	int retval;
> > +	int handle_size;
> > +	struct super_block *sb;
> > +	struct uuid *this_fs_id;
> > +	struct file_handle f_handle;
> > +	struct file_handle *handle = NULL;
> > +
> > +	if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle)))
> > +		return -EFAULT;
> > +
> > +	if (f_handle.handle_size > MAX_HANDLE_SZ)
> > +		return -EINVAL;
> > +
> > +	handle = kmalloc(sizeof(struct file_handle) + f_handle.handle_size,
> > +			GFP_KERNEL);
> > +	if (!handle) {
> > +		retval = -ENOMEM;
> > +		goto err_out;
> > +	}
> 
> You are mixing your patterns here.
> In some cases you do:
>    if (something fails)
>            return -ERROR
> other times you do
> 
>    if (something fails) {
>             retval = -ERROR;
>             goto err_out;
>    }
> 
> which has exactly the same fix.  Being consistent is best and I would
> recommend using the goto option - it is more maintainable.
> I would actually prefer:
> 
>     retval = -ERROR;
>     if (something fails)
>           goto err_out;
> 
> but leave the choice to you.  Consistency is the important thing.


Fixed as per you suggestion.

> 
> 
> 
> > +	handle_size = f_handle.handle_size;
> > +
> > +	/* we ask for a non connected handle */
> > +	retval = exportfs_encode_fh(path->dentry,
> > +				(struct fid *)handle->f_handle,
> > +				&handle_size,  0);
> > +	/* convert handle size to bytes */
> > +	handle_size *= sizeof(u32);
> > +	handle->handle_type = retval;
> > +	handle->handle_size = handle_size;
> > +	if (handle_size <= f_handle.handle_size) {
> > +		/* get the uuid */
> > +		sb = path->mnt->mnt_sb;
> > +		if (sb->s_op->get_fsid) {
> > +			this_fs_id = sb->s_op->get_fsid(sb);
> > +			memcpy(handle->fsid.uuid, this_fs_id->uuid,
> > +				sizeof(handle->fsid.uuid));
> > +		} else
> > +			memset(handle->fsid.uuid, 0, sizeof(handle->fsid.uuid));
> > +		retval = 0;
> > +	} else {
> > +		/*
> > +		 * set the handle_size to zero so we copy only
> > +		 * non variable part of the file_handle
> > +		 */
> > +		handle_size = 0;
> > +		retval = -EOVERFLOW;
> > +	}
> > +	if (copy_to_user(ufh, handle,
> > +				sizeof(struct file_handle) + handle_size))
> > +		retval = -EFAULT;
> > +
> > +	kfree(handle);
> > +err_out:
> > +	return retval;
> > +}
> > +
> > +SYSCALL_DEFINE2(name_to_handle, const char __user *, name,
> > +		struct file_handle __user *, handle)
> > +{
> > +	long ret;
> > +	struct path path;
> 
> *Surely* you want name_to_handle_at, or name_at_to_handle here.
> i.e. include an int 'dfd' to specify the starting directory.
> 
> Then you can make use of AT_SYMLINK_NOFOLLOW and so avoid the need for
> 2 syscalls.
> 


Done. Droped the lname_to_handle syscall


> 
> > +
> > +	/* Follow links */
> > +	ret = user_path(name, &path);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = do_sys_name_to_handle(&path, handle);
> > +	path_put(&path);
> > +
> > +	/* avoid REGPARM breakage on x86: */
> > +	asmlinkage_protect(2, ret, name, handle);
> > +	return ret;
> > +}
> > +
> > +SYSCALL_DEFINE2(lname_to_handle, const char __user *, name,
> > +		struct file_handle __user *, handle)
> > +{
> > +	long ret;
> > +	struct path path;
> > +
> > +	ret = user_lpath(name, &path);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = do_sys_name_to_handle(&path, handle);
> > +	path_put(&path);
> > +
> > +	/* avoid REGPARM breakage on x86: */
> > +	asmlinkage_protect(2, ret, name, handle);
> > +	return ret;
> > +}
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 39d57bc..da28b80 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -948,6 +948,20 @@ struct file {
> >  	unsigned long f_mnt_write_state;
> >  #endif
> >  };
> > +
> > +struct uuid {
> > +	char uuid[16];
> 
> unsigned char??? just a thought.
> 

Fixed

> 
> > +};
> > +
> > +struct file_handle {
> > +	int handle_size;
> > +	int handle_type;
> > +	/* File system identifier */
> > +	struct uuid fsid;
> > +	/* file identifier */
> > +	char f_handle[0];


I also changed this to unsigned char f_handle[0];


> > +};
> > +
> >  extern spinlock_t files_lock;
> >  #define file_list_lock() spin_lock(&files_lock);
> >  #define file_list_unlock() spin_unlock(&files_lock);
> > @@ -1580,6 +1594,7 @@ struct super_operations {
> >  	ssize_t (*quota_write)(struct super_block *, int, const char *, size_t, loff_t);
> >  #endif
> >  	int (*bdev_try_to_free_page)(struct super_block*, struct page*, gfp_t);
> > +	struct uuid *(*get_fsid)(struct super_block *);
> 
> Assuming this was actually a good idea (which I am not yet sold on),
> I think you would do better to pass in the address of a uuid to be filled in:
> 
>        int (*get_fsid)(struct super_block *, struct uuid *)
> 
> just incase the filesystem stored something in a slightly different format.
> 
> And as XFS has a 'nouuid' mount option, it might be best to allow an error
> return to say "there is no uuid" - maybe expect -ENOENT ??

Fixed

Thanks for the review
-aneesh