* [RFC PATCH v9 0/2] Btrfs: get/set label for a mounted file system
@ 2013-01-05 2:47 Jeff Liu
2013-01-05 2:48 ` [RFC PATCH v9 1/2] Btrfs: Add a new ioctl to get the label of " Jeff Liu
2013-01-05 2:48 ` [RFC PATCH v9 2/2] Btrfs: set/change " Jeff Liu
0 siblings, 2 replies; 8+ messages in thread
From: Jeff Liu @ 2013-01-05 2:47 UTC (permalink / raw)
To: linux-btrfs; +Cc: anand.jain, miaox, Goffredo Baroncelli, dsterba
Hello,
There have a couple of changes in this version which were shown as following:
- Don't reserve an unit of transactions for set filesystem label.
- Replace the old warning message to "btrfs: label is too long, return the first %zu bytes"
in btrfs_ioc_get_fslabel() is the existing label length is longer than than
BTRFS_LABEL_SIZE - 1. Otherwise, checkpatch.pl would give a warning as:
"WARNING: quoted string split across lines".
- btrfs_ioc_set_fslabel(): drop an error message if the input label length is too long for
debugging purpose.
The old versions can be found at:
v8:
http://permalink.gmane.org/gmane.comp.file-systems.btrfs/21906
v7:
http://www.spinics.net/lists/linux-btrfs/msg20994.html
v6:
http://www.spinics.net/lists/linux-btrfs/msg20922.html
v5:
http://www.spinics.net/lists/linux-btrfs/msg20888.html
v4:
http://permalink.gmane.org/gmane.comp.file-systems.btrfs/21618
v3:
https://patchwork.kernel.org/patch/1124642/
v2:
http://permalink.gmane.org/gmane.comp.file-systems.btrfs/12877
v1:
http://permalink.gmane.org/gmane.comp.file-systems.btrfs/12872
Thanks,
-Jeff
^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC PATCH v9 1/2] Btrfs: Add a new ioctl to get the label of a mounted file system
2013-01-05 2:47 [RFC PATCH v9 0/2] Btrfs: get/set label for a mounted file system Jeff Liu
@ 2013-01-05 2:48 ` Jeff Liu
2013-01-06 18:44 ` Goffredo Baroncelli
2013-01-05 2:48 ` [RFC PATCH v9 2/2] Btrfs: set/change " Jeff Liu
1 sibling, 1 reply; 8+ messages in thread
From: Jeff Liu @ 2013-01-05 2:48 UTC (permalink / raw)
To: linux-btrfs; +Cc: anand.jain, miaox, Goffredo Baroncelli, dsterba
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;
}
+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
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [RFC PATCH v9 2/2] Btrfs: set/change the label of a mounted file system
2013-01-05 2:47 [RFC PATCH v9 0/2] Btrfs: get/set label for a mounted file system Jeff Liu
2013-01-05 2:48 ` [RFC PATCH v9 1/2] Btrfs: Add a new ioctl to get the label of " Jeff Liu
@ 2013-01-05 2:48 ` Jeff Liu
2013-01-06 18:32 ` Goffredo Baroncelli
1 sibling, 1 reply; 8+ messages in thread
From: Jeff Liu @ 2013-01-05 2:48 UTC (permalink / raw)
To: linux-btrfs; +Cc: anand.jain, miaox, Goffredo Baroncelli, dsterba
With this new ioctl(2) BTRFS_IOC_SET_FSLABEL, we can set/change the label of a mounted file system.
Signed-off-by: Jie Liu <jeff.liu@oracle.com>
Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
Reviewed-by: Goffredo Baroncelli <kreijack@inwind.it>
Reviewed-by: David Sterba <dsterba@suse.cz>
---
fs/btrfs/ioctl.c | 42 ++++++++++++++++++++++++++++++++++++++++++
fs/btrfs/ioctl.h | 2 ++
2 files changed, 44 insertions(+)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index ef2f55a..8e676c3 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3718,6 +3718,46 @@ static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg)
return ret ? -EFAULT : 0;
}
+static int btrfs_ioctl_set_fslabel(struct file *file, void __user *arg)
+{
+ struct btrfs_root *root = BTRFS_I(fdentry(file)->d_inode)->root;
+ struct btrfs_super_block *super_block = root->fs_info->super_copy;
+ struct btrfs_trans_handle *trans;
+ char label[BTRFS_LABEL_SIZE];
+ int ret;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ if (copy_from_user(label, arg, sizeof(label)))
+ return -EFAULT;
+
+ if (strnlen(label, BTRFS_LABEL_SIZE) == BTRFS_LABEL_SIZE) {
+ pr_err("btrfs: unable to set label with more than %d bytes\n",
+ BTRFS_LABEL_SIZE - 1);
+ return -EINVAL;
+ }
+
+ ret = mnt_want_write_file(file);
+ if (ret)
+ return ret;
+
+ mutex_lock(&root->fs_info->volume_mutex);
+ trans = btrfs_start_transaction(root, 0);
+ if (IS_ERR(trans)) {
+ ret = PTR_ERR(trans);
+ goto out_unlock;
+ }
+
+ strcpy(super_block->label, label);
+ ret = btrfs_end_transaction(trans, root);
+
+out_unlock:
+ mutex_unlock(&root->fs_info->volume_mutex);
+ mnt_drop_write_file(file);
+ return ret;
+}
+
long btrfs_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
{
@@ -3818,6 +3858,8 @@ long btrfs_ioctl(struct file *file, unsigned int
return btrfs_ioctl_qgroup_limit(root, argp);
case BTRFS_IOC_GET_FSLABEL:
return btrfs_ioctl_get_fslabel(file, argp);
+ case BTRFS_IOC_SET_FSLABEL:
+ return btrfs_ioctl_set_fslabel(file, argp);
}
return -ENOTTY;
diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
index 5b2cbef..2abe239 100644
--- a/fs/btrfs/ioctl.h
+++ b/fs/btrfs/ioctl.h
@@ -453,6 +453,8 @@ struct btrfs_ioctl_send_args {
struct btrfs_ioctl_qgroup_limit_args)
#define BTRFS_IOC_GET_FSLABEL _IOR(BTRFS_IOCTL_MAGIC, 49, \
char[BTRFS_LABEL_SIZE])
+#define BTRFS_IOC_SET_FSLABEL _IOW(BTRFS_IOCTL_MAGIC, 50, \
+ char[BTRFS_LABEL_SIZE])
#define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
struct btrfs_ioctl_get_dev_stats)
#endif
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v9 2/2] Btrfs: set/change the label of a mounted file system
2013-01-05 2:48 ` [RFC PATCH v9 2/2] Btrfs: set/change " Jeff Liu
@ 2013-01-06 18:32 ` Goffredo Baroncelli
0 siblings, 0 replies; 8+ messages in thread
From: Goffredo Baroncelli @ 2013-01-06 18:32 UTC (permalink / raw)
To: Jeff Liu; +Cc: linux-btrfs, anand.jain, miaox, dsterba
Reviewed-by: Goffredo Baroncelli <kreijack@inwind.it>
On 01/05/2013 03:48 AM, Jeff Liu wrote:
> With this new ioctl(2) BTRFS_IOC_SET_FSLABEL, we can set/change the label of a mounted file system.
>
> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
> Reviewed-by: Goffredo Baroncelli <kreijack@inwind.it>
> Reviewed-by: David Sterba <dsterba@suse.cz>
>
> ---
> fs/btrfs/ioctl.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> fs/btrfs/ioctl.h | 2 ++
> 2 files changed, 44 insertions(+)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index ef2f55a..8e676c3 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3718,6 +3718,46 @@ static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg)
> return ret ? -EFAULT : 0;
> }
>
> +static int btrfs_ioctl_set_fslabel(struct file *file, void __user *arg)
> +{
> + struct btrfs_root *root = BTRFS_I(fdentry(file)->d_inode)->root;
> + struct btrfs_super_block *super_block = root->fs_info->super_copy;
> + struct btrfs_trans_handle *trans;
> + char label[BTRFS_LABEL_SIZE];
> + int ret;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + if (copy_from_user(label, arg, sizeof(label)))
> + return -EFAULT;
> +
> + if (strnlen(label, BTRFS_LABEL_SIZE) == BTRFS_LABEL_SIZE) {
> + pr_err("btrfs: unable to set label with more than %d bytes\n",
> + BTRFS_LABEL_SIZE - 1);
> + return -EINVAL;
> + }
> +
> + ret = mnt_want_write_file(file);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&root->fs_info->volume_mutex);
> + trans = btrfs_start_transaction(root, 0);
> + if (IS_ERR(trans)) {
> + ret = PTR_ERR(trans);
> + goto out_unlock;
> + }
> +
> + strcpy(super_block->label, label);
> + ret = btrfs_end_transaction(trans, root);
> +
> +out_unlock:
> + mutex_unlock(&root->fs_info->volume_mutex);
> + mnt_drop_write_file(file);
> + return ret;
> +}
> +
> long btrfs_ioctl(struct file *file, unsigned int
> cmd, unsigned long arg)
> {
> @@ -3818,6 +3858,8 @@ long btrfs_ioctl(struct file *file, unsigned int
> return btrfs_ioctl_qgroup_limit(root, argp);
> case BTRFS_IOC_GET_FSLABEL:
> return btrfs_ioctl_get_fslabel(file, argp);
> + case BTRFS_IOC_SET_FSLABEL:
> + return btrfs_ioctl_set_fslabel(file, argp);
> }
>
> return -ENOTTY;
> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
> index 5b2cbef..2abe239 100644
> --- a/fs/btrfs/ioctl.h
> +++ b/fs/btrfs/ioctl.h
> @@ -453,6 +453,8 @@ struct btrfs_ioctl_send_args {
> struct btrfs_ioctl_qgroup_limit_args)
> #define BTRFS_IOC_GET_FSLABEL _IOR(BTRFS_IOCTL_MAGIC, 49, \
> char[BTRFS_LABEL_SIZE])
> +#define BTRFS_IOC_SET_FSLABEL _IOW(BTRFS_IOCTL_MAGIC, 50, \
> + char[BTRFS_LABEL_SIZE])
> #define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
> struct btrfs_ioctl_get_dev_stats)
> #endif
--
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v9 1/2] Btrfs: Add a new ioctl to get the label of a mounted file system
2013-01-05 2:48 ` [RFC PATCH v9 1/2] Btrfs: Add a new ioctl to get the label of " Jeff Liu
@ 2013-01-06 18:44 ` Goffredo Baroncelli
2013-01-07 6:24 ` Jeff Liu
0 siblings, 1 reply; 8+ messages in thread
From: Goffredo Baroncelli @ 2013-01-06 18:44 UTC (permalink / raw)
To: Jeff Liu; +Cc: linux-btrfs, anand.jain, miaox, dsterba
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
--
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v9 1/2] Btrfs: Add a new ioctl to get the label of a mounted file system
2013-01-06 18:44 ` Goffredo Baroncelli
@ 2013-01-07 6:24 ` Jeff Liu
2013-01-07 17:32 ` Goffredo Baroncelli
0 siblings, 1 reply; 8+ messages in thread
From: Jeff Liu @ 2013-01-07 6:24 UTC (permalink / raw)
To: kreijack; +Cc: Goffredo Baroncelli, linux-btrfs, anand.jain, miaox, dsterba
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
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v9 1/2] Btrfs: Add a new ioctl to get the label of a mounted file system
2013-01-07 6:24 ` Jeff Liu
@ 2013-01-07 17:32 ` Goffredo Baroncelli
2013-01-08 7:58 ` Jeff Liu
0 siblings, 1 reply; 8+ messages in thread
From: Goffredo Baroncelli @ 2013-01-07 17:32 UTC (permalink / raw)
To: Jeff Liu; +Cc: linux-btrfs, anand.jain, miaox, dsterba
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
>>
>>
>
>
--
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v9 1/2] Btrfs: Add a new ioctl to get the label of a mounted file system
2013-01-07 17:32 ` Goffredo Baroncelli
@ 2013-01-08 7:58 ` Jeff Liu
0 siblings, 0 replies; 8+ messages in thread
From: Jeff Liu @ 2013-01-08 7:58 UTC (permalink / raw)
To: kreijack; +Cc: Goffredo Baroncelli, linux-btrfs, anand.jain, miaox, dsterba
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
>>>
>>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-01-08 7:59 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-01-05 2:47 [RFC PATCH v9 0/2] Btrfs: get/set label for a mounted file system Jeff Liu
2013-01-05 2:48 ` [RFC PATCH v9 1/2] Btrfs: Add a new ioctl to get the label of " Jeff Liu
2013-01-06 18:44 ` Goffredo Baroncelli
2013-01-07 6:24 ` Jeff Liu
2013-01-07 17:32 ` Goffredo Baroncelli
2013-01-08 7:58 ` Jeff Liu
2013-01-05 2:48 ` [RFC PATCH v9 2/2] Btrfs: set/change " Jeff Liu
2013-01-06 18:32 ` Goffredo Baroncelli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).