Re: [PATCH 1/2 v2] btrfs: label should not contain return char
On 21/05/14 00:33, David Sterba wrote:
On Tue, May 20, 2014 at 02:36:48PM +0800, Anand Jain wrote:
From: Anand Jain anand.j...@oracle.com
generally if you use
echo test /sys/fs/btrfs/fsid/label
it would introduce return char at the end and it can not
be part of the label. The correct command is
echo -n test /sys/fs/btrfs/fsid/label
This patch will check for this user error
Signed-off-by: Anand Jain anand.j...@oracle.com
---
v2: accepts review comments. Thanks Eric and Roman
fs/btrfs/sysfs.c | 20 +---
1 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index c5eb214..ca63fcd 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -373,22 +373,36 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
struct btrfs_trans_handle *trans;
struct btrfs_root *root = fs_info-fs_root;
int ret;
+ char *label;
+ char *pos;
- if (len = BTRFS_LABEL_SIZE) {
+ label = kzalloc(len, GFP_NOFS);
You can avoid allocating the buffer entirely:
- search for '\n', if found, use only that amount of bytes
- check for maximum size, copy to label if ok
Thats too good.
Thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v2] btrfs: label should not contain return char
(Random aside: why does btrfs support online fs relabeling, anyway?) -Eric Online you mean when mounted ? But I had an opinion that should we support label store from the sysfs interface when the (sysfs) interface can't communicate the module's specific errors back to the user.? Thanks -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v2] btrfs: label should not contain return char
On 5/21/14, 9:05 PM, Anand Jain wrote: (Random aside: why does btrfs support online fs relabeling, anyway?) -Eric Online you mean when mounted ? Yep - I'm just not sure who would ever want to do that. Aren't labels primarly used for mounting, during the mount process? So changing it while mounted seems like a feature looking for a usecase... -Eric -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v2] btrfs: label should not contain return char
On Wed, 21 May 2014 21:14:07 -0500 Eric Sandeen sand...@redhat.com wrote: (Random aside: why does btrfs support online fs relabeling, anyway?) -Eric Online you mean when mounted ? Yep - I'm just not sure who would ever want to do that. Aren't labels primarly used for mounting, during the mount process? Well if you want to change the label of your root filesystem, how else would you go about that? If Btrfs did not support this, then only while booted from a rescue LiveCD? That's quite a bit more involved and not even always feasible in case of remote machines with no IPMI or similar management. Extfs supports online change of the volume label as well, albeit probably not via a sysfs file, but with the tune2fs utility: tune2fs -L name /dev/blockdevice -- With respect, Roman signature.asc Description: PGP signature
Re: [PATCH 1/2 v2] btrfs: label should not contain return char
On 5/20/14, 1:36 AM, Anand Jain wrote:
From: Anand Jain anand.j...@oracle.com
generally if you use
echo test /sys/fs/btrfs/fsid/label
it would introduce return char at the end and it can not
be part of the label. The correct command is
echo -n test /sys/fs/btrfs/fsid/label
This patch will check for this user error
Signed-off-by: Anand Jain anand.j...@oracle.com
---
v2: accepts review comments. Thanks Eric and Roman
fs/btrfs/sysfs.c | 20 +---
1 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index c5eb214..ca63fcd 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -373,22 +373,36 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
struct btrfs_trans_handle *trans;
struct btrfs_root *root = fs_info-fs_root;
int ret;
+ char *label;
+ char *pos;
- if (len = BTRFS_LABEL_SIZE) {
+ label = kzalloc(len, GFP_NOFS);
+ if (!label)
+ return -ENOMEM;
+
+ memcpy(label, buf, len);
+ strlcpy(label, buf, len);
would ensure that the resulting string is null-terminated... I don't know
if buf comes in 0-terminated or not, or if len includes \0. *shrug*
these are strings after all...
+ if ((pos = strchr(label, '\n')))
+ *pos = '\0';
label = strstrip(label); might be simpler/better?
(this would strip all leading trailing whitespace, I presume that'd
be desirable, but then maybe someone really does want mylabel \t ?)
+
+ if (strlen(label) = BTRFS_LABEL_SIZE) {
hm, strlen doesn't include the \0 right? so if we had 256 chars + \0, this
would pass, and the subsequent strcpy will copy 257 bytes into a 256-byte
buffer, right?
(I'm terrible at string handling in C, so I might be wrong... you all
can point and laugh if I am)
pr_err(BTRFS: unable to set label with more than %d bytes\n,
BTRFS_LABEL_SIZE - 1);
+ kfree(label);
return -EINVAL;
}
trans = btrfs_start_transaction(root, 0);
- if (IS_ERR(trans))
+ if (IS_ERR(trans)) {
+ kfree(label);
return PTR_ERR(trans);
+ }
spin_lock(root-fs_info-super_lock);
- strcpy(fs_info-super_copy-label, buf);
+ strcpy(fs_info-super_copy-label, label);
spin_unlock(root-fs_info-super_lock);
ret = btrfs_commit_transaction(trans, root);
+ kfree(label);
after the 3rd kfree() maybe an
out:
target would be better
(Random aside: why does btrfs support online fs relabeling, anyway?)
-Eric
if (!ret)
return len;
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v2] btrfs: label should not contain return char
On Tue, May 20, 2014 at 02:36:48PM +0800, Anand Jain wrote:
From: Anand Jain anand.j...@oracle.com
generally if you use
echo test /sys/fs/btrfs/fsid/label
it would introduce return char at the end and it can not
be part of the label. The correct command is
echo -n test /sys/fs/btrfs/fsid/label
This patch will check for this user error
Signed-off-by: Anand Jain anand.j...@oracle.com
---
v2: accepts review comments. Thanks Eric and Roman
fs/btrfs/sysfs.c | 20 +---
1 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index c5eb214..ca63fcd 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -373,22 +373,36 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
struct btrfs_trans_handle *trans;
struct btrfs_root *root = fs_info-fs_root;
int ret;
+ char *label;
+ char *pos;
- if (len = BTRFS_LABEL_SIZE) {
+ label = kzalloc(len, GFP_NOFS);
You can avoid allocating the buffer entirely:
- search for '\n', if found, use only that amount of bytes
- check for maximum size, copy to label if ok
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v2] btrfs: label should not contain return char
On 5/20/14, 11:33 AM, David Sterba wrote:
On Tue, May 20, 2014 at 02:36:48PM +0800, Anand Jain wrote:
From: Anand Jain anand.j...@oracle.com
generally if you use
echo test /sys/fs/btrfs/fsid/label
it would introduce return char at the end and it can not
be part of the label. The correct command is
echo -n test /sys/fs/btrfs/fsid/label
This patch will check for this user error
Signed-off-by: Anand Jain anand.j...@oracle.com
---
v2: accepts review comments. Thanks Eric and Roman
fs/btrfs/sysfs.c | 20 +---
1 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index c5eb214..ca63fcd 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -373,22 +373,36 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
struct btrfs_trans_handle *trans;
struct btrfs_root *root = fs_info-fs_root;
int ret;
+char *label;
+char *pos;
-if (len = BTRFS_LABEL_SIZE) {
+label = kzalloc(len, GFP_NOFS);
You can avoid allocating the buffer entirely:
- search for '\n', if found, use only that amount of bytes
- check for maximum size, copy to label if ok
that's probably better than my strstrip idea, which requires
writable memory. Odds of finding \t are slim. ;)
-Eric
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
