[PATCH 1/2] btrfs: label should not contain return char

[PATCH 1/2] btrfs: label should not contain return char

[Anand Jain]

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>
---
 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);
 		return -EINVAL;
-- 
1.8.5.3

[PATCH 2/2] btrfs: usage error should not be logged into system log

[Anand Jain]

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

Re: [PATCH 1/2] btrfs: label should not contain return char

[Eric Sandeen]

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;
> 

Re: [PATCH 1/2] btrfs: label should not contain return char

[Roman Mamedov]

[-- 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 --]

[PATCH 1/2 v2] btrfs: label should not contain return char

[Anand Jain]

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

[PATCH 2/2 v2] btrfs: usage error should not be logged into system log

[Anand Jain]

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

Re: [PATCH 1/2] btrfs: label should not contain return char

[Anand Jain]

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
>

Re: [PATCH 1/2] btrfs: label should not contain return char

[Anand Jain]

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.

Re: [PATCH 1/2 v2] btrfs: label should not contain return char

[Eric Sandeen]

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;
>  
> 

Re: [PATCH 1/2 v2] btrfs: label should not contain return char

[David Sterba]

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

Re: [PATCH 2/2 v2] btrfs: usage error should not be logged into system log

[David Sterba]

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.

Re: [PATCH 1/2 v2] btrfs: label should not contain return char

[Eric Sandeen]

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

Re: [PATCH 1/2 v2] btrfs: label should not contain return char

[Anand Jain]

>
> (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

Re: [PATCH 1/2 v2] btrfs: label should not contain return char

[Eric Sandeen]

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

Re: [PATCH 1/2 v2] btrfs: label should not contain return char

[Roman Mamedov]

[-- 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 --]

[PATCH 1/2 v3] btrfs: label should not contain return char

[Anand Jain]

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

[PATCH 2/2 v3] btrfs: usage error should not be logged into system log

[Anand Jain]

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

Re: [PATCH 1/2 v2] btrfs: label should not contain return char

[Anand Jain]

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.

Re: [PATCH 2/2 v3] btrfs: usage error should not be logged into system log

[Koen Kooi]

-----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-----

Re: [PATCH 1/2 v3] btrfs: label should not contain return char

[David Sterba]

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);

Re: [PATCH 1/2 v2] btrfs: label should not contain return char

[Eric Sandeen]

-----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-----

Re: [PATCH 2/2 v3] btrfs: usage error should not be logged into system log

[Anand Jain]

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

[PATCH 1/2 v4] btrfs: label should not contain return char

[Anand Jain]

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

[PATCH 2/2 v4] btrfs: usage error should not be logged into system log

[Anand Jain]

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

Re: [PATCH 1/2 v4] btrfs: label should not contain return char

[David Sterba]

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.

[PATCH v5] btrfs: label should not contain return char

[Satoru Takeuchi]

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

Re: [PATCH v5] btrfs: label should not contain return char

[Wang Shilong]

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);
>  

[PATCH v6] btrfs: label should not contain return char

[Satoru Takeuchi]

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

Re: [PATCH v6] btrfs: label should not contain return char

[Wang Shilong]

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);
>  

Re: [PATCH v6] btrfs: label should not contain return char

[David Sterba]

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>