| Message ID | 1400567808-9494-1-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 5/20/14, 1:36 AM, Anand Jain wrote: > From: Anand Jain <Anand.Jain@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.Jain@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 20, 2014 at 02:36:48PM +0800, Anand Jain wrote: > From: Anand Jain <Anand.Jain@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.Jain@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.Jain@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.Jain@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > (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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 21 May 2014 21:14:07 -0500 Eric Sandeen <sandeen@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
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.Jain@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.Jain@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 5/21/14, 11:14 PM, Roman Mamedov wrote: > On Wed, 21 May 2014 21:14:07 -0500 > Eric Sandeen <sandeen@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 Yeah, that just writes directly to the block device. :) But ok, I guess relabeling root is a fair case. - -Eric -----BEGIN PGP SIGNATURE----- Version: GnuPG/MacGPG2 v2.0.17 (Darwin) Comment: GPGTools - http://gpgtools.org Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJTfiCAAAoJECCuFpLhPd7gAHoP/AlFeFJ8Om0wKxcnuolG7o4G 9AHRu4Jto8JTHWToOwmtJAcWQFS+fJtUF009cl118eb+vUOoWFdTDK3H8UiC6uwM /OcFaeDRJOg4JEA8OBaXihCoG1JOPU2ygCezyg59mbHV6SFIYy4kDgqzfFm6qcaC 8vqenmPkGhYiIadvPeDYctIPGS6DquLjxKk/xI52gLwP6so2eJ9nH4jmO7K1gF6n dRa7zfCDpcMq5AqiJW5XZWBfmuNH5etGgJW8J4JpuOSzYPECwuHzcXP3LZny/xUP t15LxzUR+0SZg04zJYXY5YoytZaz6mfk0jOksggeY9GiDenlWUZhDi/QexIhc2ZL ZuHt8WDWDtIqt4D84GE/9QlI5tVU+F8FkJBnPSS+ybP8YSShNWmIdKLVtn+4g8/x YVC6PnUyYvLj/1ZSJTGUQtJZkSjsfkRISk3ygVz1NJQG75euRL2gxmZkn11497rU rc4a+yZQ3Gxe0cOn1J8RzN9dDQ3iIQnM7Fn25vFdn/uN1dPuQuDP3zy9MG4WJD8e 3xX1HNiXePgZGN5bhaCLXqAER6E4Xigd5mcdjuXZvmkexEkTRn3xOcXuoDhU8v2i Oloroh09tpMmzw4sNyaeqw11oIkBcUEvY/7ggqYEtZcOAtnFuyot9y5LUIgNmtBM Sas6/mxzXeTPCApF1o1a =sUri -----END PGP SIGNATURE----- -- 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/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); + if ((pos = strchr(label, '\n'))) + *pos = '\0'; + + if (strlen(label) >= BTRFS_LABEL_SIZE) { 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); if (!ret) return len;