From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from userp1040.oracle.com ([156.151.31.81]:25145 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750923Ab3AHH7f (ORCPT ); Tue, 8 Jan 2013 02:59:35 -0500 Message-ID: <50EBD189.1050706@oracle.com> Date: Tue, 08 Jan 2013 15:58:01 +0800 From: Jeff Liu MIME-Version: 1.0 To: kreijack@inwind.it CC: Goffredo Baroncelli , linux-btrfs@vger.kernel.org, anand.jain@oracle.com, miaox@cn.fujitsu.com, dsterba@suse.cz Subject: Re: [RFC PATCH v9 1/2] Btrfs: Add a new ioctl to get the label of a mounted file system References: <50E79461.3090802@oracle.com> <50E9C617.7010303@gmail.com> <50EA6A03.4090900@oracle.com> <50EB0694.7070401@tiscalinet.it> In-Reply-To: <50EB0694.7070401@tiscalinet.it> Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-btrfs-owner@vger.kernel.org List-ID: Hi Goffredo, On 01/08/2013 01:32 AM, Goffredo Baroncelli wrote: > Hi Jeff, > > On 01/07/2013 07:24 AM, Jeff Liu wrote: >> Hi, >> On 01/07/2013 02:44 AM, Goffredo Baroncelli wrote: >>> Hi Jeff, >>> >>> On 01/05/2013 03:48 AM, Jeff Liu wrote: >>>> Add a new ioctl(2) BTRFS_IOC_GET_FSLABLE, so that we can get the label upon a mounted filesystem. >>>> >>>> Signed-off-by: Jie Liu >>>> Signed-off-by: Anand Jain >>>> Cc: Miao Xie >>>> Cc: Goffredo Baroncelli >>>> Cc: David Sterba >>>> >>>> --- >>>> fs/btrfs/ioctl.c | 21 +++++++++++++++++++++ >>>> fs/btrfs/ioctl.h | 2 ++ >>>> 2 files changed, 23 insertions(+) >>>> >>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c >>>> index 8fcf9a5..ef2f55a 100644 >>>> --- a/fs/btrfs/ioctl.c >>>> +++ b/fs/btrfs/ioctl.c >>>> @@ -3699,6 +3699,25 @@ out: >>>> return ret; >>>> } >>>> >>> >>> May be that it was already discussed, and I am missing something, but if >>> we check the label length we should terminate the string with a zero in >>> case this is too long... >>> >>> However this is a minor bug, please push this patch forward.. >> I don't think so. >> Why we need to terminate the string with a zero? >> We have already truncated the label string up to maximum 255 bytes if it >> was too long. >> >> Consider the normal conditions, i.e. the label string is less than 256, >> we just copy it back to the user without a terminated NUL. > > Sorry I don't understand the reason to terminate the string up to 255 > chars without adding a zero. Or we copy all the buffer (BTRFS_LABEL_SIZE > characters without furthers checks) or we copy BTRFS_LABEL_SIZE-1 > characters, adding a zero at the end. For normal cases, there should already has a NUL at the end of the input label string when performing btrfs_ioc_set_fslabel(), and we have also added a NUL for unmounted label set in btrfs-progs. In both cases, put it again on btrfs_ioc_get_fslabel() is redundant IMO. If we ran into a too long label, return the first 255 bytes is fine for both mounted and unmounted get_label() routines in btrfs-progs since I did memset(label, 0, sizeof(label)) at first for fetching mounted fs label. Also, I have not found any existing kernel code does similar things by adding a NUL to end up a char array, could you show me an example? """Or we copy all the buffer(BTRFS_LABEL_SIZE) characters without furthers checks""" If an array is capable of N chars, we can only full it with N - 1 chars. Thanks, -Jeff >>> >>> >>>> +static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg) >>>> +{ >>>> + struct btrfs_root *root = BTRFS_I(fdentry(file)->d_inode)->root; >>>> + const char *label = root->fs_info->super_copy->label; >>>> + size_t len = strnlen(label, BTRFS_LABEL_SIZE); >>>> + int ret; >>> + int label_is_too_long = 0; >> bool label_is_too_long = false; >>>> + >>>> + if (len == BTRFS_LABEL_SIZE) { >>>> + pr_warn("btrfs: label is too long, return the first %zu bytes\n", >>>> + --len); >>> >>> + label_is_too_long = 1; >> label_is_too_long = true; >>>> + } >>>> + >>>> + mutex_lock(&root->fs_info->volume_mutex); >>>> + ret = copy_to_user(arg, label, len); >>> + if (!ret && label_is_too_long) >>> + ret = copy_to_user(arg+BTRFS_LABEL_SIZE, "", 1); >> Hmm? ret = copy_to_user(arg + len, "", 1); >> > My idea was to put a zero at the end of the string, but I did a mistake > with the cut&paste.. sorry :-) > >>>> + mutex_unlock(&root->fs_info->volume_mutex); >> If you or anyone has objection to this patch, please just post yours, I >> won't follow it up again. > > Sorry I don't want to bother anybody, I know that reviewing a patch nine > times is a very havvy work. I (and I think more other peoples) really > appreciate your efforts. > > BR > G.Baroncelli > >>>> + >>>> + return ret ? -EFAULT : 0; >>>> +} >>>> + >>>> long btrfs_ioctl(struct file *file, unsigned int >>>> cmd, unsigned long arg) >>>> { >>>> @@ -3797,6 +3816,8 @@ long btrfs_ioctl(struct file *file, unsigned int >>>> return btrfs_ioctl_qgroup_create(root, argp); >>>> case BTRFS_IOC_QGROUP_LIMIT: >>>> return btrfs_ioctl_qgroup_limit(root, argp); >>>> + case BTRFS_IOC_GET_FSLABEL: >>>> + return btrfs_ioctl_get_fslabel(file, argp); >>>> } >>>> >>>> return -ENOTTY; >>>> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h >>>> index 731e287..5b2cbef 100644 >>>> --- a/fs/btrfs/ioctl.h >>>> +++ b/fs/btrfs/ioctl.h >>>> @@ -451,6 +451,8 @@ struct btrfs_ioctl_send_args { >>>> struct btrfs_ioctl_qgroup_create_args) >>>> #define BTRFS_IOC_QGROUP_LIMIT _IOR(BTRFS_IOCTL_MAGIC, 43, \ >>>> struct btrfs_ioctl_qgroup_limit_args) >>>> +#define BTRFS_IOC_GET_FSLABEL _IOR(BTRFS_IOCTL_MAGIC, 49, \ >>>> + char[BTRFS_LABEL_SIZE]) >>>> #define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \ >>>> struct btrfs_ioctl_get_dev_stats) >>>> #endif >>> >>> >> >> > >