* [PATCH 2/2] btrfs: usage error should not be logged into system log
2014-05-19 17:04 [PATCH 1/2] btrfs: label should not contain return char Anand Jain
@ 2014-05-19 17:04 ` Anand Jain
2014-05-20 6:38 ` [PATCH 2/2 v2] " Anand Jain
2014-05-19 17:16 ` [PATCH 1/2] btrfs: label should not contain return char Eric Sandeen
` (4 subsequent siblings)
5 siblings, 1 reply; 30+ messages in thread
From: Anand Jain @ 2014-05-19 17:04 UTC (permalink / raw)
To: linux-btrfs
From: Anand Jain <Anand.Jain@oracle.com>
I have an opinion that system logs /var/log/messages are
valuable info to investigate the real system issues at
the data center. People handling data center issues
do spend a lot time and efforts analyzing messages
files. Having usage error logged into /var/log/messages
is something we should avoid.
Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
---
fs/btrfs/sysfs.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 63c2907..f729199 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -374,11 +374,8 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
struct btrfs_root *root = fs_info->fs_root;
int ret;
- if (len >= BTRFS_LABEL_SIZE || strchr(buf, '\n')) {
- pr_err("BTRFS: unable to set label with more than %d bytes\n",
- BTRFS_LABEL_SIZE - 1);
+ if (len >= BTRFS_LABEL_SIZE || strchr(buf, '\n'))
return -EINVAL;
- }
trans = btrfs_start_transaction(root, 0);
if (IS_ERR(trans))
--
1.8.5.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/2 v2] btrfs: usage error should not be logged into system log
2014-05-19 17:04 ` [PATCH 2/2] btrfs: usage error should not be logged into system log Anand Jain
@ 2014-05-20 6:38 ` Anand Jain
2014-05-20 16:36 ` David Sterba
0 siblings, 1 reply; 30+ messages in thread
From: Anand Jain @ 2014-05-20 6:38 UTC (permalink / raw)
To: linux-btrfs
From: Anand Jain <Anand.Jain@oracle.com>
I have an opinion that system logs /var/log/messages are
valuable info to investigate the real system issues at
the data center. People handling data center issues
do spend a lot time and efforts analyzing messages
files. Having usage error logged into /var/log/messages
is something we should avoid.
Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
---
v2: rebase
fs/btrfs/sysfs.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index ca63fcd..36fae0f 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -385,8 +385,6 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
*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;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2 v2] btrfs: usage error should not be logged into system log
2014-05-20 6:38 ` [PATCH 2/2 v2] " Anand Jain
@ 2014-05-20 16:36 ` David Sterba
0 siblings, 0 replies; 30+ messages in thread
From: David Sterba @ 2014-05-20 16:36 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs
On Tue, May 20, 2014 at 02:38:11PM +0800, Anand Jain wrote:
> From: Anand Jain <Anand.Jain@oracle.com>
>
> I have an opinion that system logs /var/log/messages are
> valuable info to investigate the real system issues at
> the data center. People handling data center issues
> do spend a lot time and efforts analyzing messages
> files. Having usage error logged into /var/log/messages
> is something we should avoid.
>
> Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
Reviewed-by: David Sterba <dsterba@suse.cz>
In this case the source of the error is only one and the label
constraints are simple, so the syslog message does not add much.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] btrfs: label should not contain return char
2014-05-19 17:04 [PATCH 1/2] btrfs: label should not contain return char Anand Jain
2014-05-19 17:04 ` [PATCH 2/2] btrfs: usage error should not be logged into system log Anand Jain
@ 2014-05-19 17:16 ` Eric Sandeen
2014-05-20 6:40 ` Anand Jain
2014-05-19 17:19 ` Roman Mamedov
` (3 subsequent siblings)
5 siblings, 1 reply; 30+ messages in thread
From: Eric Sandeen @ 2014-05-19 17:16 UTC (permalink / raw)
To: Anand Jain, linux-btrfs
On 5/19/14, 12:04 PM, 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
Wouldn't it be a lot better to just strip the "\n" if it
exists?
> Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
> ---
> fs/btrfs/sysfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index c5eb214..63c2907 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -374,7 +374,7 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
> struct btrfs_root *root = fs_info->fs_root;
> int ret;
>
> - if (len >= BTRFS_LABEL_SIZE) {
> + if (len >= BTRFS_LABEL_SIZE || strchr(buf, '\n')) {
> pr_err("BTRFS: unable to set label with more than %d bytes\n",
> BTRFS_LABEL_SIZE - 1);
so if I do:
# echo "mylabel" > /sys/fs/btrfs/<fsid>/label
I'll get:
BTRFS: unable to set label with more than 255 bytes"
which would be pretty confusing, IMHO, given the short
label I tried to create.
Just strip out the \n ...
-Eric
> return -EINVAL;
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] btrfs: label should not contain return char
2014-05-19 17:16 ` [PATCH 1/2] btrfs: label should not contain return char Eric Sandeen
@ 2014-05-20 6:40 ` Anand Jain
0 siblings, 0 replies; 30+ messages in thread
From: Anand Jain @ 2014-05-20 6:40 UTC (permalink / raw)
To: Eric Sandeen, linux-btrfs
On 20/05/14 01:16, Eric Sandeen wrote:
> On 5/19/14, 12:04 PM, 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
>
> Wouldn't it be a lot better to just strip the "\n" if it
> exists?
yes. Thanks Eric.
>> Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
>> ---
>> fs/btrfs/sysfs.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
>> index c5eb214..63c2907 100644
>> --- a/fs/btrfs/sysfs.c
>> +++ b/fs/btrfs/sysfs.c
>> @@ -374,7 +374,7 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
>> struct btrfs_root *root = fs_info->fs_root;
>> int ret;
>>
>> - if (len >= BTRFS_LABEL_SIZE) {
>> + if (len >= BTRFS_LABEL_SIZE || strchr(buf, '\n')) {
>> pr_err("BTRFS: unable to set label with more than %d bytes\n",
>> BTRFS_LABEL_SIZE - 1);
>
> so if I do:
>
> # echo "mylabel" > /sys/fs/btrfs/<fsid>/label
>
> I'll get:
>
> BTRFS: unable to set label with more than 255 bytes"
>
> which would be pretty confusing, IMHO, given the short
> label I tried to create.
>
> Just strip out the \n ...
>
> -Eric
>
>> return -EINVAL;
>>
>
> --
> 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
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] btrfs: label should not contain return char
2014-05-19 17:04 [PATCH 1/2] btrfs: label should not contain return char Anand Jain
2014-05-19 17:04 ` [PATCH 2/2] btrfs: usage error should not be logged into system log Anand Jain
2014-05-19 17:16 ` [PATCH 1/2] btrfs: label should not contain return char Eric Sandeen
@ 2014-05-19 17:19 ` Roman Mamedov
2014-05-20 6:42 ` Anand Jain
2014-05-20 6:36 ` [PATCH 1/2 v2] " Anand Jain
` (2 subsequent siblings)
5 siblings, 1 reply; 30+ messages in thread
From: Roman Mamedov @ 2014-05-19 17:19 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs
[-- Attachment #1: Type: text/plain, Size: 775 bytes --]
On Tue, 20 May 2014 01:04:30 +0800
Anand Jain <anand.jain@oracle.com> 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
Maybe instead consider checking for one trailing "\n", and silently remove it
if passed, so that both of the mentioned variants of 'echo' can be used?
All other sysfs files do not care if you pass an extra "\n" at the end, e.g.
echo cfq > /sys/block/sda/queue/scheduler
works fine, doesn't require you to use "echo -n cfq".
--
With respect,
Roman
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] btrfs: label should not contain return char
2014-05-19 17:19 ` Roman Mamedov
@ 2014-05-20 6:42 ` Anand Jain
0 siblings, 0 replies; 30+ messages in thread
From: Anand Jain @ 2014-05-20 6:42 UTC (permalink / raw)
To: Roman Mamedov; +Cc: linux-btrfs
On 20/05/14 01:19, Roman Mamedov wrote:
> On Tue, 20 May 2014 01:04:30 +0800
> Anand Jain <anand.jain@oracle.com> 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
>
> Maybe instead consider checking for one trailing "\n", and silently remove it
> if passed, so that both of the mentioned variants of 'echo' can be used?
>
> All other sysfs files do not care if you pass an extra "\n" at the end, e.g.
>
> echo cfq > /sys/block/sda/queue/scheduler
>
> works fine, doesn't require you to use "echo -n cfq".
>
Thanks Roman.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/2 v2] btrfs: label should not contain return char
2014-05-19 17:04 [PATCH 1/2] btrfs: label should not contain return char Anand Jain
` (2 preceding siblings ...)
2014-05-19 17:19 ` Roman Mamedov
@ 2014-05-20 6:36 ` Anand Jain
2014-05-20 16:32 ` Eric Sandeen
2014-05-20 16:33 ` David Sterba
2014-05-22 10:41 ` [PATCH 1/2 v3] " Anand Jain
2014-05-23 2:50 ` [PATCH 1/2 v4] " Anand Jain
5 siblings, 2 replies; 30+ messages in thread
From: Anand Jain @ 2014-05-20 6:36 UTC (permalink / raw)
To: linux-btrfs; +Cc: sandeen, rm
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);
+ 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;
--
1.7.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2 v2] btrfs: label should not contain return char
2014-05-20 6:36 ` [PATCH 1/2 v2] " Anand Jain
@ 2014-05-20 16:32 ` Eric Sandeen
2014-05-22 2:05 ` Anand Jain
2014-05-20 16:33 ` David Sterba
1 sibling, 1 reply; 30+ messages in thread
From: Eric Sandeen @ 2014-05-20 16:32 UTC (permalink / raw)
To: Anand Jain, linux-btrfs; +Cc: rm
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;
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2 v2] btrfs: label should not contain return char
2014-05-20 16:32 ` Eric Sandeen
@ 2014-05-22 2:05 ` Anand Jain
2014-05-22 2:14 ` Eric Sandeen
0 siblings, 1 reply; 30+ messages in thread
From: Anand Jain @ 2014-05-22 2:05 UTC (permalink / raw)
To: Eric Sandeen, linux-btrfs; +Cc: rm
>
> (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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2 v2] btrfs: label should not contain return char
2014-05-22 2:05 ` Anand Jain
@ 2014-05-22 2:14 ` Eric Sandeen
2014-05-22 4:14 ` Roman Mamedov
0 siblings, 1 reply; 30+ messages in thread
From: Eric Sandeen @ 2014-05-22 2:14 UTC (permalink / raw)
To: Anand Jain, linux-btrfs; +Cc: rm
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2 v2] btrfs: label should not contain return char
2014-05-22 2:14 ` Eric Sandeen
@ 2014-05-22 4:14 ` Roman Mamedov
2014-05-22 16:06 ` Eric Sandeen
0 siblings, 1 reply; 30+ messages in thread
From: Roman Mamedov @ 2014-05-22 4:14 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Anand Jain, linux-btrfs
[-- Attachment #1: Type: text/plain, Size: 857 bytes --]
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
--
With respect,
Roman
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2 v2] btrfs: label should not contain return char
2014-05-22 4:14 ` Roman Mamedov
@ 2014-05-22 16:06 ` Eric Sandeen
0 siblings, 0 replies; 30+ messages in thread
From: Eric Sandeen @ 2014-05-22 16:06 UTC (permalink / raw)
To: Roman Mamedov; +Cc: Anand Jain, linux-btrfs
-----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-----
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2 v2] btrfs: label should not contain return char
2014-05-20 6:36 ` [PATCH 1/2 v2] " Anand Jain
2014-05-20 16:32 ` Eric Sandeen
@ 2014-05-20 16:33 ` David Sterba
2014-05-20 16:41 ` Eric Sandeen
2014-05-22 10:47 ` Anand Jain
1 sibling, 2 replies; 30+ messages in thread
From: David Sterba @ 2014-05-20 16:33 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs, sandeen, rm
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2 v2] btrfs: label should not contain return char
2014-05-20 16:33 ` David Sterba
@ 2014-05-20 16:41 ` Eric Sandeen
2014-05-22 10:47 ` Anand Jain
1 sibling, 0 replies; 30+ messages in thread
From: Eric Sandeen @ 2014-05-20 16:41 UTC (permalink / raw)
To: dsterba, Anand Jain, linux-btrfs, rm
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2 v2] btrfs: label should not contain return char
2014-05-20 16:33 ` David Sterba
2014-05-20 16:41 ` Eric Sandeen
@ 2014-05-22 10:47 ` Anand Jain
1 sibling, 0 replies; 30+ messages in thread
From: Anand Jain @ 2014-05-22 10:47 UTC (permalink / raw)
To: dsterba, linux-btrfs, sandeen, rm
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/2 v3] btrfs: label should not contain return char
2014-05-19 17:04 [PATCH 1/2] btrfs: label should not contain return char Anand Jain
` (3 preceding siblings ...)
2014-05-20 6:36 ` [PATCH 1/2 v2] " Anand Jain
@ 2014-05-22 10:41 ` Anand Jain
2014-05-22 10:41 ` [PATCH 2/2 v3] btrfs: usage error should not be logged into system log Anand Jain
2014-05-22 11:41 ` [PATCH 1/2 v3] btrfs: label should not contain return char David Sterba
2014-05-23 2:50 ` [PATCH 1/2 v4] " Anand Jain
5 siblings, 2 replies; 30+ messages in thread
From: Anand Jain @ 2014-05-22 10:41 UTC (permalink / raw)
To: linux-btrfs; +Cc: dsterba, sandeen, rm
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>
---
v3: accepts review comments. Thanks David and Eric again
v2: accepts review comments. Thanks Eric and Roman
fs/btrfs/sysfs.c | 12 ++++++++++--
1 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index c5eb214..159df4f 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -373,8 +373,15 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
struct btrfs_trans_handle *trans;
struct btrfs_root *root = fs_info->fs_root;
int ret;
+ size_t p_len;
- if (len >= BTRFS_LABEL_SIZE) {
+ /*
+ * p_len is the len until the first occurrence of either
+ * '\n' or '\0'
+ */
+ p_len = strcspn(buf, "\n");
+
+ if (p_len >= BTRFS_LABEL_SIZE) {
pr_err("BTRFS: unable to set label with more than %d bytes\n",
BTRFS_LABEL_SIZE - 1);
return -EINVAL;
@@ -385,7 +392,8 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
return PTR_ERR(trans);
spin_lock(&root->fs_info->super_lock);
- strcpy(fs_info->super_copy->label, buf);
+ strncpy(fs_info->super_copy->label, buf, p_len);
+ memset(fs_info->super_copy->label + p_len, '\0', 1);
spin_unlock(&root->fs_info->super_lock);
ret = btrfs_commit_transaction(trans, root);
--
1.7.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/2 v3] btrfs: usage error should not be logged into system log
2014-05-22 10:41 ` [PATCH 1/2 v3] " Anand Jain
@ 2014-05-22 10:41 ` Anand Jain
2014-05-22 11:21 ` Koen Kooi
2014-05-22 11:41 ` [PATCH 1/2 v3] btrfs: label should not contain return char David Sterba
1 sibling, 1 reply; 30+ messages in thread
From: Anand Jain @ 2014-05-22 10:41 UTC (permalink / raw)
To: linux-btrfs; +Cc: dsterba, sandeen, rm
From: Anand Jain <Anand.Jain@oracle.com>
I have an opinion that system logs /var/log/messages are
valuable info to investigate the real system issues at
the data center. People handling data center issues
do spend a lot time and efforts analyzing messages
files. Having usage error logged into /var/log/messages
is something we should avoid.
Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
Reviewed-by: David Sterba <dsterba@suse.cz>
---
v3: rebase again
v2: rebase
fs/btrfs/sysfs.c | 5 +----
1 files changed, 1 insertions(+), 4 deletions(-)
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 159df4f..c81b499 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -381,11 +381,8 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
*/
p_len = strcspn(buf, "\n");
- if (p_len >= BTRFS_LABEL_SIZE) {
- pr_err("BTRFS: unable to set label with more than %d bytes\n",
- BTRFS_LABEL_SIZE - 1);
+ if (p_len >= BTRFS_LABEL_SIZE)
return -EINVAL;
- }
trans = btrfs_start_transaction(root, 0);
if (IS_ERR(trans))
--
1.7.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2 v3] btrfs: usage error should not be logged into system log
2014-05-22 10:41 ` [PATCH 2/2 v3] btrfs: usage error should not be logged into system log Anand Jain
@ 2014-05-22 11:21 ` Koen Kooi
2014-05-23 2:41 ` Anand Jain
0 siblings, 1 reply; 30+ messages in thread
From: Koen Kooi @ 2014-05-22 11:21 UTC (permalink / raw)
To: linux-btrfs
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Anand Jain schreef op 22-05-14 12:41:
> From: Anand Jain <Anand.Jain@oracle.com>
>
> I have an opinion that system logs /var/log/messages are valuable info to
> investigate the real system issues at the data center. People handling
> data center issues do spend a lot time and efforts analyzing messages
> files. Having usage error logged into /var/log/messages is something we
> should avoid.
Do you mean 'syslog' when you say '/var/log/messages'? There's no
/var/log/messages on my machines..
regards,
Koen
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Darwin)
Comment: GPGTools - http://gpgtools.org
iD8DBQFTfd2kMkyGM64RGpERAsd6AKCZxfhjjtYWUZLJwS0NnghuCb9lBQCfYye2
L3z3JmZqj9TTb+355MMB6w8=
=SfnW
-----END PGP SIGNATURE-----
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2 v3] btrfs: usage error should not be logged into system log
2014-05-22 11:21 ` Koen Kooi
@ 2014-05-23 2:41 ` Anand Jain
0 siblings, 0 replies; 30+ messages in thread
From: Anand Jain @ 2014-05-23 2:41 UTC (permalink / raw)
To: Koen Kooi, linux-btrfs
On 22/05/14 19:21, Koen Kooi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Anand Jain schreef op 22-05-14 12:41:
>> From: Anand Jain <Anand.Jain@oracle.com>
>>
>> I have an opinion that system logs /var/log/messages are valuable info to
>> investigate the real system issues at the data center. People handling
>> data center issues do spend a lot time and efforts analyzing messages
>> files. Having usage error logged into /var/log/messages is something we
>> should avoid.
>
> Do you mean 'syslog' when you say '/var/log/messages'? There's no
> /var/log/messages on my machines..
yes indeed, depending on whats configured.
sorry to confuse you. Will update the commit
Thanks, Anand
> regards,
>
> Koen
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2 v3] btrfs: label should not contain return char
2014-05-22 10:41 ` [PATCH 1/2 v3] " Anand Jain
2014-05-22 10:41 ` [PATCH 2/2 v3] btrfs: usage error should not be logged into system log Anand Jain
@ 2014-05-22 11:41 ` David Sterba
1 sibling, 0 replies; 30+ messages in thread
From: David Sterba @ 2014-05-22 11:41 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs, dsterba, sandeen, rm
On Thu, May 22, 2014 at 06:41:11PM +0800, Anand Jain wrote:
> @@ -385,7 +392,8 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
> return PTR_ERR(trans);
>
> spin_lock(&root->fs_info->super_lock);
> - strcpy(fs_info->super_copy->label, buf);
> + strncpy(fs_info->super_copy->label, buf, p_len);
> + memset(fs_info->super_copy->label + p_len, '\0', 1);
This looks strange, memset of length 1?
Anyway, I think the label buffer should be zeroed at the empty space, so
the idea is
memset(fs_info->super_copy->label, 0, BTRFS_LABEL_SIZE):
memcpy(fs_info->super_copy->label, buf, p_len);
Not super efficient, but works.
> spin_unlock(&root->fs_info->super_lock);
> ret = btrfs_commit_transaction(trans, root);
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/2 v4] btrfs: label should not contain return char
2014-05-19 17:04 [PATCH 1/2] btrfs: label should not contain return char Anand Jain
` (4 preceding siblings ...)
2014-05-22 10:41 ` [PATCH 1/2 v3] " Anand Jain
@ 2014-05-23 2:50 ` Anand Jain
2014-05-23 2:50 ` [PATCH 2/2 v4] btrfs: usage error should not be logged into system log Anand Jain
` (2 more replies)
5 siblings, 3 replies; 30+ messages in thread
From: Anand Jain @ 2014-05-23 2:50 UTC (permalink / raw)
To: linux-btrfs; +Cc: dsterba, sandeen, rm, koen
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>
---
v4: used memcpy and memset. Thanks David again
v3: accepts review comments. Thanks David and Eric again
v2: accepts review comments. Thanks Eric and Roman
fs/btrfs/sysfs.c | 12 ++++++++++--
1 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index c5eb214..e12e2be 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -373,8 +373,15 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
struct btrfs_trans_handle *trans;
struct btrfs_root *root = fs_info->fs_root;
int ret;
+ size_t p_len;
- if (len >= BTRFS_LABEL_SIZE) {
+ /*
+ * p_len is the len until the first occurrence of either
+ * '\n' or '\0'
+ */
+ p_len = strcspn(buf, "\n");
+
+ if (p_len >= BTRFS_LABEL_SIZE) {
pr_err("BTRFS: unable to set label with more than %d bytes\n",
BTRFS_LABEL_SIZE - 1);
return -EINVAL;
@@ -385,7 +392,8 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
return PTR_ERR(trans);
spin_lock(&root->fs_info->super_lock);
- strcpy(fs_info->super_copy->label, buf);
+ memset(fs_info->super_copy->label, 0, BTRFS_LABEL_SIZE);
+ memcpy(fs_info->super_copy->label, buf, p_len);
spin_unlock(&root->fs_info->super_lock);
ret = btrfs_commit_transaction(trans, root);
--
1.7.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/2 v4] btrfs: usage error should not be logged into system log
2014-05-23 2:50 ` [PATCH 1/2 v4] " Anand Jain
@ 2014-05-23 2:50 ` Anand Jain
2014-05-26 17:41 ` [PATCH 1/2 v4] btrfs: label should not contain return char David Sterba
2014-07-01 5:22 ` [PATCH v5] " Satoru Takeuchi
2 siblings, 0 replies; 30+ messages in thread
From: Anand Jain @ 2014-05-23 2:50 UTC (permalink / raw)
To: linux-btrfs; +Cc: dsterba, sandeen, rm, koen
From: Anand Jain <Anand.Jain@oracle.com>
I have an opinion that system logs under /var/log/ are
valuable info to investigate the real system issues at
the data center. People handling data center issues
do spend a lot time and efforts analyzing messages
files. Having usage error logged into system log is
something we should avoid.
Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
Reviewed-by: David Sterba <dsterba@suse.cz>
---
v4: commit reword. Thanks Koen
v3: rebase again
v2: rebase
fs/btrfs/sysfs.c | 5 +----
1 files changed, 1 insertions(+), 4 deletions(-)
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index e12e2be..522d023 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -381,11 +381,8 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
*/
p_len = strcspn(buf, "\n");
- if (p_len >= BTRFS_LABEL_SIZE) {
- pr_err("BTRFS: unable to set label with more than %d bytes\n",
- BTRFS_LABEL_SIZE - 1);
+ if (p_len >= BTRFS_LABEL_SIZE)
return -EINVAL;
- }
trans = btrfs_start_transaction(root, 0);
if (IS_ERR(trans))
--
1.7.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2 v4] btrfs: label should not contain return char
2014-05-23 2:50 ` [PATCH 1/2 v4] " Anand Jain
2014-05-23 2:50 ` [PATCH 2/2 v4] btrfs: usage error should not be logged into system log Anand Jain
@ 2014-05-26 17:41 ` David Sterba
2014-07-01 5:22 ` [PATCH v5] " Satoru Takeuchi
2 siblings, 0 replies; 30+ messages in thread
From: David Sterba @ 2014-05-26 17:41 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs, dsterba, sandeen, rm, koen
On Fri, May 23, 2014 at 10:50:01AM +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>
Reviewed-by: David Sterba <dsterba@suse.cz>
Thanks.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v5] btrfs: label should not contain return char
2014-05-23 2:50 ` [PATCH 1/2 v4] " Anand Jain
2014-05-23 2:50 ` [PATCH 2/2 v4] btrfs: usage error should not be logged into system log Anand Jain
2014-05-26 17:41 ` [PATCH 1/2 v4] btrfs: label should not contain return char David Sterba
@ 2014-07-01 5:22 ` Satoru Takeuchi
2014-07-01 6:46 ` Wang Shilong
2 siblings, 1 reply; 30+ messages in thread
From: Satoru Takeuchi @ 2014-07-01 5:22 UTC (permalink / raw)
To: Anand Jain, linux-btrfs; +Cc: dsterba, sandeen, rm, koen
Although Anand once sent the following two patches,
- [PATCH 1/2 v4] btrfs: label should not contain return char
- [PATCH 2/2 v4] btrfs: usage error should not be logged into system log
only the latter patch was merged to mason/for-linus and 3.16-rc3
as 402a0f4 (by accident?). It results in that the former patch can't
be cleanly applied to 3.16-rc3.
I fixed this problem, wrote a reproducer, and tested it.
Test Result:
3.16-rc3 w/o this patch: fail
3.16-rc3 w/ this patch: pass
Subject: [PATCH v5] btrfs: label should not contain return char
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
reproducer.sh:
===============================================================================
#!/bin/bash
TEST_DEV=/dev/vdb
TEST_DIR=/home/sat/mnt
umount /home/sat/mnt
mkfs.btrfs -f $TEST_DEV
UUID=$(btrfs fi show $TEST_DEV | head -1 | sed -e 's/.*uuid: \([-0-9a-z]*\)$/\1/')
mount $TEST_DEV $TEST_DIR
LABELFILE=/sys/fs/btrfs/$UUID/label
echo testlabel >$LABELFILE
LINES="$(cat $LABELFILE | wc -l | awk '{print $1}')"
RET=1
if [ $LINES -eq 1 ] ; then
echo '[PASS] Trailing \n is removed correctly.' >&2
RET=0
else
echo '[FAIL] Trailing \n still exists.' >&2
fi
exit $RET
===============================================================================
Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
Reviewed-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
Tested-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
---
v5: tweak to be able to apply on the top of 402a0f4.
add test program.
v4: used memcpy and memset. Thanks David again
v3: accepts review comments. Thanks David and Eric again
v2: accepts review comments. Thanks Eric and Roman
---
fs/btrfs/sysfs.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index df39458..dcae61a 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -374,8 +374,15 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
struct btrfs_trans_handle *trans;
struct btrfs_root *root = fs_info->fs_root;
int ret;
+ size_t p_len;
- if (len >= BTRFS_LABEL_SIZE)
+ /*
+ * p_len is the len until the first occurrence of either
+ * '\n' or '\0'
+ */
+ p_len = strcspn(buf, "\n");
+
+ if (p_len >= BTRFS_LABEL_SIZE)
return -EINVAL;
trans = btrfs_start_transaction(root, 0);
@@ -383,7 +390,8 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
return PTR_ERR(trans);
spin_lock(&root->fs_info->super_lock);
- strcpy(fs_info->super_copy->label, buf);
+ memset(fs_info->super_copy->label, 0, BTRFS_LABEL_SIZE);
+ memcpy(fs_info->super_copy->label, buf, p_len);
spin_unlock(&root->fs_info->super_lock);
ret = btrfs_commit_transaction(trans, root);
--
1.9.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v5] btrfs: label should not contain return char
2014-07-01 5:22 ` [PATCH v5] " Satoru Takeuchi
@ 2014-07-01 6:46 ` Wang Shilong
2014-07-01 8:00 ` [PATCH v6] " Satoru Takeuchi
0 siblings, 1 reply; 30+ messages in thread
From: Wang Shilong @ 2014-07-01 6:46 UTC (permalink / raw)
To: Satoru Takeuchi, Anand Jain, linux-btrfs; +Cc: dsterba, sandeen, rm, koen
Hi Satoru and all,
I think there maybe a leftover issue.
That is if we don't set label, in default it will output a blank line.
Steps to reproduce:
# mkfs.btrfs -f /dev/sdb
# mount /dev/sdb
# cat /sys/fs/btrfs/<uuid>/label -->an extra line will be outputed.
This is because in btrfs_label_show(), we did something like this directly:
return snprintf(buf, PAGE_SIZE, "%s\n", fs_info->super_copy->label);
Maybe we can have a check whether label is NULL before we output?
otherwise,the
extra blank line is outputed, IMO this is not so nice thing!
Thanks,
Wang
On 07/01/2014 01:22 PM, Satoru Takeuchi wrote:
> Although Anand once sent the following two patches,
>
> - [PATCH 1/2 v4] btrfs: label should not contain return char
> - [PATCH 2/2 v4] btrfs: usage error should not be logged into system log
>
> only the latter patch was merged to mason/for-linus and 3.16-rc3
> as 402a0f4 (by accident?). It results in that the former patch can't
> be cleanly applied to 3.16-rc3.
>
> I fixed this problem, wrote a reproducer, and tested it.
>
> Test Result:
> 3.16-rc3 w/o this patch: fail
> 3.16-rc3 w/ this patch: pass
>
>
>
> Subject: [PATCH v5] btrfs: label should not contain return char
>
> 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
>
> reproducer.sh:
> ===============================================================================
> #!/bin/bash
>
> TEST_DEV=/dev/vdb
> TEST_DIR=/home/sat/mnt
>
> umount /home/sat/mnt
>
> mkfs.btrfs -f $TEST_DEV
> UUID=$(btrfs fi show $TEST_DEV | head -1 | sed -e 's/.*uuid: \([-0-9a-z]*\)$/\1/')
> mount $TEST_DEV $TEST_DIR
> LABELFILE=/sys/fs/btrfs/$UUID/label
>
> echo testlabel >$LABELFILE
> LINES="$(cat $LABELFILE | wc -l | awk '{print $1}')"
>
> RET=1
> if [ $LINES -eq 1 ] ; then
> echo '[PASS] Trailing \n is removed correctly.' >&2
> RET=0
> else
> echo '[FAIL] Trailing \n still exists.' >&2
> fi
>
> exit $RET
> ===============================================================================
>
> Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
> Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
> Reviewed-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
> Tested-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
> ---
> v5: tweak to be able to apply on the top of 402a0f4.
> add test program.
> v4: used memcpy and memset. Thanks David again
> v3: accepts review comments. Thanks David and Eric again
> v2: accepts review comments. Thanks Eric and Roman
>
> ---
> fs/btrfs/sysfs.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index df39458..dcae61a 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -374,8 +374,15 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
> struct btrfs_trans_handle *trans;
> struct btrfs_root *root = fs_info->fs_root;
> int ret;
> + size_t p_len;
>
> - if (len >= BTRFS_LABEL_SIZE)
> + /*
> + * p_len is the len until the first occurrence of either
> + * '\n' or '\0'
> + */
> + p_len = strcspn(buf, "\n");
> +
> + if (p_len >= BTRFS_LABEL_SIZE)
> return -EINVAL;
>
> trans = btrfs_start_transaction(root, 0);
> @@ -383,7 +390,8 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
> return PTR_ERR(trans);
>
> spin_lock(&root->fs_info->super_lock);
> - strcpy(fs_info->super_copy->label, buf);
> + memset(fs_info->super_copy->label, 0, BTRFS_LABEL_SIZE);
> + memcpy(fs_info->super_copy->label, buf, p_len);
> spin_unlock(&root->fs_info->super_lock);
> ret = btrfs_commit_transaction(trans, root);
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v6] btrfs: label should not contain return char
2014-07-01 6:46 ` Wang Shilong
@ 2014-07-01 8:00 ` Satoru Takeuchi
2014-07-01 8:29 ` Wang Shilong
2014-07-01 15:05 ` David Sterba
0 siblings, 2 replies; 30+ messages in thread
From: Satoru Takeuchi @ 2014-07-01 8:00 UTC (permalink / raw)
To: Wang Shilong, Anand Jain, linux-btrfs; +Cc: dsterba, sandeen, rm, koen
Hi Wang,
(2014/07/01 15:46), Wang Shilong wrote:
> Hi Satoru and all,
>
> I think there maybe a leftover issue.
> That is if we don't set label, in default it will output a blank line.
>
> Steps to reproduce:
>
> # mkfs.btrfs -f /dev/sdb
> # mount /dev/sdb
> # cat /sys/fs/btrfs/<uuid>/label -->an extra line will be outputed.
>
> This is because in btrfs_label_show(), we did something like this directly:
>
> return snprintf(buf, PAGE_SIZE, "%s\n", fs_info->super_copy->label);
>
> Maybe we can have a check whether label is NULL before we output?
> otherwise,the
> extra blank line is outputed, IMO this is not so nice thing!
OK, how about it is? I also add a test for empty-label case.
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
reproducer.sh:
===============================================================================
#!/bin/bash
TEST_DEV=/dev/vdb
TEST_DIR=/home/sat/mnt
umount /home/sat/mnt
mkfs.btrfs -f $TEST_DEV
UUID=$(btrfs fi show $TEST_DEV | head -1 | sed -e 's/.*uuid: \([-0-9a-z]*\)$/\1/')
mount $TEST_DEV $TEST_DIR
LABELFILE=/sys/fs/btrfs/$UUID/label
echo "Test for empty label..." >&2
LINES="$(cat $LABELFILE | wc -l | awk '{print $1}')"
RET=0
if [ $LINES -eq 0 ] ; then
echo '[PASS] Trailing \n is removed correctly.' >&2
else
echo '[FAIL] Trailing \n still exists.' >&2
RET=1
fi
echo "Test for non-empty label..." >&2
echo testlabel >$LABELFILE
LINES="$(cat $LABELFILE | wc -l | awk '{print $1}')"
if [ $LINES -eq 1 ] ; then
echo '[PASS] Trailing \n is removed correctly.' >&2
else
echo '[FAIL] Trailing \n still exists.' >&2
RET=1
fi
exit $RET
===============================================================================
Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
Reviewed-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
Tested-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
---
v6: fix empty-label case
v5: tweak to be able to apply on the top of 402a0f4.
add test program.
v4: used memcpy and memset. Thanks David again
v3: accepts review comments. Thanks David and Eric again
v2: accepts review comments. Thanks Eric and Roman
---
fs/btrfs/sysfs.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index df39458..68d1f63 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -363,7 +363,8 @@ static ssize_t btrfs_label_show(struct kobject *kobj,
struct kobj_attribute *a, char *buf)
{
struct btrfs_fs_info *fs_info = to_fs_info(kobj);
- return snprintf(buf, PAGE_SIZE, "%s\n", fs_info->super_copy->label);
+ char *label = fs_info->super_copy->label;
+ return snprintf(buf, PAGE_SIZE, label[0] ? "%s\n" : "%s", label);
}
static ssize_t btrfs_label_store(struct kobject *kobj,
@@ -374,8 +375,15 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
struct btrfs_trans_handle *trans;
struct btrfs_root *root = fs_info->fs_root;
int ret;
+ size_t p_len;
- if (len >= BTRFS_LABEL_SIZE)
+ /*
+ * p_len is the len until the first occurrence of either
+ * '\n' or '\0'
+ */
+ p_len = strcspn(buf, "\n");
+
+ if (p_len >= BTRFS_LABEL_SIZE)
return -EINVAL;
trans = btrfs_start_transaction(root, 0);
@@ -383,7 +391,8 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
return PTR_ERR(trans);
spin_lock(&root->fs_info->super_lock);
- strcpy(fs_info->super_copy->label, buf);
+ memset(fs_info->super_copy->label, 0, BTRFS_LABEL_SIZE);
+ memcpy(fs_info->super_copy->label, buf, p_len);
spin_unlock(&root->fs_info->super_lock);
ret = btrfs_commit_transaction(trans, root);
--
1.9.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v6] btrfs: label should not contain return char
2014-07-01 8:00 ` [PATCH v6] " Satoru Takeuchi
@ 2014-07-01 8:29 ` Wang Shilong
2014-07-01 15:05 ` David Sterba
1 sibling, 0 replies; 30+ messages in thread
From: Wang Shilong @ 2014-07-01 8:29 UTC (permalink / raw)
To: Satoru Takeuchi, Anand Jain, linux-btrfs; +Cc: dsterba, sandeen, rm, koen
Hi Satoru,
On 07/01/2014 04:00 PM, Satoru Takeuchi wrote:
> Hi Wang,
>
> (2014/07/01 15:46), Wang Shilong wrote:
>> Hi Satoru and all,
>>
>> I think there maybe a leftover issue.
>> That is if we don't set label, in default it will output a blank line.
>>
>> Steps to reproduce:
>>
>> # mkfs.btrfs -f /dev/sdb
>> # mount /dev/sdb
>> # cat /sys/fs/btrfs/<uuid>/label -->an extra line will be outputed.
>>
>> This is because in btrfs_label_show(), we did something like this directly:
>>
>> return snprintf(buf, PAGE_SIZE, "%s\n", fs_info->super_copy->label);
>>
>> Maybe we can have a check whether label is NULL before we output?
>> otherwise,the
>> extra blank line is outputed, IMO this is not so nice thing!
> OK, how about it is? I also add a test for empty-label case.
>
>
>
> 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
>
> reproducer.sh:
> ===============================================================================
> #!/bin/bash
>
> TEST_DEV=/dev/vdb
> TEST_DIR=/home/sat/mnt
>
> umount /home/sat/mnt
>
> mkfs.btrfs -f $TEST_DEV
> UUID=$(btrfs fi show $TEST_DEV | head -1 | sed -e 's/.*uuid: \([-0-9a-z]*\)$/\1/')
> mount $TEST_DEV $TEST_DIR
> LABELFILE=/sys/fs/btrfs/$UUID/label
>
> echo "Test for empty label..." >&2
> LINES="$(cat $LABELFILE | wc -l | awk '{print $1}')"
> RET=0
>
> if [ $LINES -eq 0 ] ; then
> echo '[PASS] Trailing \n is removed correctly.' >&2
> else
> echo '[FAIL] Trailing \n still exists.' >&2
> RET=1
> fi
>
> echo "Test for non-empty label..." >&2
>
> echo testlabel >$LABELFILE
> LINES="$(cat $LABELFILE | wc -l | awk '{print $1}')"
>
> if [ $LINES -eq 1 ] ; then
> echo '[PASS] Trailing \n is removed correctly.' >&2
> else
> echo '[FAIL] Trailing \n still exists.' >&2
> RET=1
> fi
>
> exit $RET
> ===============================================================================
>
> Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
> Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
> Reviewed-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
> Tested-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
Thanks for pushing this patch, Now it Looks good to me.
Reviewed-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
> ---
> v6: fix empty-label case
> v5: tweak to be able to apply on the top of 402a0f4.
> add test program.
> v4: used memcpy and memset. Thanks David again
> v3: accepts review comments. Thanks David and Eric again
> v2: accepts review comments. Thanks Eric and Roman
> ---
> fs/btrfs/sysfs.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index df39458..68d1f63 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -363,7 +363,8 @@ static ssize_t btrfs_label_show(struct kobject *kobj,
> struct kobj_attribute *a, char *buf)
> {
> struct btrfs_fs_info *fs_info = to_fs_info(kobj);
> - return snprintf(buf, PAGE_SIZE, "%s\n", fs_info->super_copy->label);
> + char *label = fs_info->super_copy->label;
> + return snprintf(buf, PAGE_SIZE, label[0] ? "%s\n" : "%s", label);
> }
>
> static ssize_t btrfs_label_store(struct kobject *kobj,
> @@ -374,8 +375,15 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
> struct btrfs_trans_handle *trans;
> struct btrfs_root *root = fs_info->fs_root;
> int ret;
> + size_t p_len;
>
> - if (len >= BTRFS_LABEL_SIZE)
> + /*
> + * p_len is the len until the first occurrence of either
> + * '\n' or '\0'
> + */
> + p_len = strcspn(buf, "\n");
> +
> + if (p_len >= BTRFS_LABEL_SIZE)
> return -EINVAL;
>
> trans = btrfs_start_transaction(root, 0);
> @@ -383,7 +391,8 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
> return PTR_ERR(trans);
>
> spin_lock(&root->fs_info->super_lock);
> - strcpy(fs_info->super_copy->label, buf);
> + memset(fs_info->super_copy->label, 0, BTRFS_LABEL_SIZE);
> + memcpy(fs_info->super_copy->label, buf, p_len);
> spin_unlock(&root->fs_info->super_lock);
> ret = btrfs_commit_transaction(trans, root);
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v6] btrfs: label should not contain return char
2014-07-01 8:00 ` [PATCH v6] " Satoru Takeuchi
2014-07-01 8:29 ` Wang Shilong
@ 2014-07-01 15:05 ` David Sterba
1 sibling, 0 replies; 30+ messages in thread
From: David Sterba @ 2014-07-01 15:05 UTC (permalink / raw)
To: Satoru Takeuchi
Cc: Wang Shilong, Anand Jain, linux-btrfs, dsterba, sandeen, rm, koen
On Tue, Jul 01, 2014 at 05:00:07PM +0900, Satoru Takeuchi wrote:
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -363,7 +363,8 @@ static ssize_t btrfs_label_show(struct kobject *kobj,
> struct kobj_attribute *a, char *buf)
> {
> struct btrfs_fs_info *fs_info = to_fs_info(kobj);
> - return snprintf(buf, PAGE_SIZE, "%s\n", fs_info->super_copy->label);
> + char *label = fs_info->super_copy->label;
> + return snprintf(buf, PAGE_SIZE, label[0] ? "%s\n" : "%s", label);
I'd prefer "" instead of the "%s" branch, but does not matter that much,
Reviewed-by: David Sterba <dsterba@suse.cz>
^ permalink raw reply [flat|nested] 30+ messages in thread