| Message ID | 50E79461.3090802@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <jeff.liu@oracle.com> > Signed-off-by: Anand Jain <anand.jain@oracle.com> > Cc: Miao Xie <miaox@cn.fujitsu.com> > Cc: Goffredo Baroncelli <kreijack@inwind.it> > Cc: David Sterba <dsterba@suse.cz> > > --- > 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.. > +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; > + > + if (len == BTRFS_LABEL_SIZE) { > + pr_warn("btrfs: label is too long, return the first %zu bytes\n", > + --len); + label_is_too_long = 1; > + } > + > + 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); > + mutex_unlock(&root->fs_info->volume_mutex); > + > + 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
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 <jeff.liu@oracle.com> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> Cc: Miao Xie <miaox@cn.fujitsu.com> >> Cc: Goffredo Baroncelli <kreijack@inwind.it> >> Cc: David Sterba <dsterba@suse.cz> >> >> --- >> 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. > > >> +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); >> + 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. >> + >> + 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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <jeff.liu@oracle.com> >>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >>> Cc: Miao Xie <miaox@cn.fujitsu.com> >>> Cc: Goffredo Baroncelli <kreijack@inwind.it> >>> Cc: David Sterba <dsterba@suse.cz> >>> >>> --- >>> 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. >> >> >>> +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 >> >> > >
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 <jeff.liu@oracle.com> >>>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >>>> Cc: Miao Xie <miaox@cn.fujitsu.com> >>>> Cc: Goffredo Baroncelli <kreijack@inwind.it> >>>> Cc: David Sterba <dsterba@suse.cz> >>>> >>>> --- >>>> 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 >>> >>> >> >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; } +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; + + if (len == BTRFS_LABEL_SIZE) { + pr_warn("btrfs: label is too long, return the first %zu bytes\n", + --len); + } + + mutex_lock(&root->fs_info->volume_mutex); + ret = copy_to_user(arg, label, len); + mutex_unlock(&root->fs_info->volume_mutex); + + 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