On Thu, Sep 01, 2011 at 04:47:47PM +0800, Jeff Liu wrote: > Hello, > > I'd like to introduce a new ioctl to set file system label. > With this feature, we can execute `btrfs filesystem label [label] > [path]` through btrfs tools to set or change the label. > > Signed-off-by: Jie Liu <jeff....@oracle.com> > > --- > fs/btrfs/ctree.h | 6 ++++++ > fs/btrfs/ioctl.c | 37 +++++++++++++++++++++++++++++++++++++ > fs/btrfs/ioctl.h | 2 ++ > 3 files changed, 45 insertions(+), 0 deletions(-) > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index 03912c5..2889b5e 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -1259,6 +1259,12 @@ struct btrfs_ioctl_defrag_range_args { > }; > > > +struct btrfs_ioctl_fs_label_args { > + /* label length in bytes */ > + __u32 len; > + char label[BTRFS_LABEL_SIZE]; > +};
Why do we need to provide a length here? Simply force a zero byte at the end of the string when you copy it into kernel space, and then use strcpy(). Then you have no need to test for length at all. > /* > * inode items have the data typically returned from stat and store other > * info about object characteristics. There is one for every file > and dir in > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 970977a..9e01628 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -268,6 +268,41 @@ static int btrfs_ioctl_getversion(struct file > *file, int __user *arg) > return put_user(inode->i_generation, arg); > } > > +static int btrfs_ioctl_fs_setlabel(struct btrfs_root *root, void > __user *arg) > +{ > + struct btrfs_super_block *super_block = &(root->fs_info->super_copy); > + struct btrfs_ioctl_fs_label_args *label_args; > + struct btrfs_trans_handle *trans; > + > + if (!capable(CAP_SYS_ADMIN)) > + return -EPERM; > + > + if (btrfs_root_readonly(root)) > + return -EROFS; > + > + label_args = memdup_user(arg, sizeof(*label_args)); > + if (IS_ERR(label_args)) > + return PTR_ERR(label_args); > + > + if (label_args->len >= sizeof(label_args->label)) > + return -EINVAL; Memory leak... you're not freeing label_args > + mutex_lock(&root->fs_info->volume_mutex); > + trans = btrfs_start_transaction(root, 0); > + if (IS_ERR(trans)) > + return PTR_ERR(trans); and here -- and you're leaving the mutex locked > + if (snprintf(super_block->label, BTRFS_LABEL_SIZE, "%s", > + label_args->label) != label_args->len) > + return -EFAULT; and here -- plus the transaction is still running > + btrfs_end_transaction(trans, root); > + mutex_unlock(&root->fs_info->volume_mutex); > + > + kfree(label_args); > + return 0; > +} > + > static noinline int btrfs_ioctl_fitrim(struct file *file, void > __user *arg) > { > struct btrfs_root *root = fdentry(file)->d_sb->s_fs_info; > @@ -2876,6 +2911,8 @@ long btrfs_ioctl(struct file *file, unsigned int > return btrfs_ioctl_fs_info(root, argp); > case BTRFS_IOC_DEV_INFO: > return btrfs_ioctl_dev_info(root, argp); > + case BTRFS_IOC_FS_SETLABEL: > + return btrfs_ioctl_fs_setlabel(root, argp); > case BTRFS_IOC_BALANCE: > return btrfs_balance(root->fs_info->dev_root); > case BTRFS_IOC_CLONE: > diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h > index ad1ea78..1e0ca2a 100644 > --- a/fs/btrfs/ioctl.h > +++ b/fs/btrfs/ioctl.h > @@ -248,4 +248,6 @@ struct btrfs_ioctl_space_args { > struct btrfs_ioctl_dev_info_args) > #define BTRFS_IOC_FS_INFO _IOR(BTRFS_IOCTL_MAGIC, 31, \ > struct btrfs_ioctl_fs_info_args) > +#define BTRFS_IOC_FS_SETLABEL _IOW(BTRFS_IOCTL_MAGIC, 32, \ > + struct btrfs_ioctl_fs_label_args) Could you use an unassigned number from [1], and update the wiki page, please? (The page only went up yesterday, but it's been needed for a while). > #endif Will there be a userspace patch for this along shortly? Hugo. [1] https://btrfs.wiki.kernel.org/index.php/Project_ideas#Development_notes.2C_please_read -- === Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk === PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk --- You can't expect a boy to be depraved until he's gone to --- a good school.
signature.asc
Description: Digital signature