Btrfs devel - Oct 2012 - [PATCH] Btrfs-progs: use atoll() for mkfs.btrfs size option

On 32 bit systems, a numerical parameter of 2147483648 or above to
the mkfs.btrfs -b option does not work. The parameter is stored in
an u64 but is read using atol() and thus not read correctly.
This patch changes it to use atoll().
Specifying a multiplier (k, m or g) like "100g" in the mkfs.btrfs
parameter list would also be a working workaround.

Signed-off-by: Stefan Behrens <sbehrens@giantdisaster.de>
---
The patch is based on the master branch of
git://git.kernel.org/pub/scm/linux/kernel/git/mason/btrfs-progs.git

 mkfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mkfs.c b/mkfs.c
index 47f0c9c..e62f0e4 100644
--- a/mkfs.c
+++ b/mkfs.c
@@ -80,7 +80,7 @@ static u64 parse_size(char *s)
 		}
 		s[len - 1] = ''\0'';
 	}
-	ret = atol(s) * mult;
+	ret = atoll(s) * mult;
 	free(s);
 	return ret;
 }
-- 
1.7.12.3

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

Hi Stefan,

when I checked your patch I found further problems to the function
parse_size():
- this function is declared two time: both in mkfs.c and in cmd-filesystem.c;
this is a good chance to move it in the utils.c file.
- your suggestion was to use atoll, which is signed. strtoull (which is
unsigned) would be a better choice.
- this function doesn''t check for overflow [minor]
- if a number like 123MB is passed, it is evaluated as 123 instead of
123*1024*1024 [bug]

This suggested me to make the following patch.
Hoping to not offende anybody :-)

BR
G.Baroncelli

Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>

You can pull the patch from 
   http://cassiopea.homelinux.net/git/btrfs-progs-unstable.git
branch
   parse_size


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

From: Goffredo Baroncelli <kreijack@inwind.it>

Move parse_size() in utils.c, because it is used both from
cmds-filesystem.c and mkfs.c.
Moreover added check about overflow, support to further units (
t=tera, e=exa, z=zetta, y=yotta).
Correct a bug which happens if a value like 123MB is passed: before
the function returned 123 instead of 123*1024*1024
---
 cmds-filesystem.c |   26 -----------------------
 mkfs.c            |   31 ---------------------------
 utils.c           |   61 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 utils.h           |    1 +
 4 files changed, 62 insertions(+), 57 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 9c43d35..507239a 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -311,32 +311,6 @@ static int cmd_sync(int argc, char **argv)
 	return 0;
 }
 
-static u64 parse_size(char *s)
-{
-	int len = strlen(s);
-	char c;
-	u64 mult = 1;
-
-	if (!isdigit(s[len - 1])) {
-		c = tolower(s[len - 1]);
-		switch (c) {
-		case ''g'':
-			mult *= 1024;
-		case ''m'':
-			mult *= 1024;
-		case ''k'':
-			mult *= 1024;
-		case ''b'':
-			break;
-		default:
-			fprintf(stderr, "Unknown size descriptor %c\n", c);
-			exit(1);
-		}
-		s[len - 1] = ''\0'';
-	}
-	return atoll(s) * mult;
-}
-
 static int parse_compress_type(char *s)
 {
 	if (strcmp(optarg, "zlib") == 0)
diff --git a/mkfs.c b/mkfs.c
index 47f0c9c..ca850d9 100644
--- a/mkfs.c
+++ b/mkfs.c
@@ -54,37 +54,6 @@ struct directory_name_entry {
 	struct list_head list;
 };
 
-static u64 parse_size(char *s)
-{
-	int len = strlen(s);
-	char c;
-	u64 mult = 1;
-	u64 ret;
-
-	s = strdup(s);
-
-	if (len && !isdigit(s[len - 1])) {
-		c = tolower(s[len - 1]);
-		switch (c) {
-		case ''g'':
-			mult *= 1024;
-		case ''m'':
-			mult *= 1024;
-		case ''k'':
-			mult *= 1024;
-		case ''b'':
-			break;
-		default:
-			fprintf(stderr, "Unknown size descriptor %c\n", c);
-			exit(1);
-		}
-		s[len - 1] = ''\0'';
-	}
-	ret = atol(s) * mult;
-	free(s);
-	return ret;
-}
-
 static int make_root_dir(struct btrfs_root *root, int mixed)
 {
 	struct btrfs_trans_handle *trans;
diff --git a/utils.c b/utils.c
index 205e667..b1bd669 100644
--- a/utils.c
+++ b/utils.c
@@ -1220,3 +1220,64 @@ scan_again:
 	return 0;
 }
 
+/* 
+ *  see http://gurmeet.net/puzzles/fast-bit-counting-routines/ for
+ *  further algorithms, I used the only one which I understood :-)
+ */
+static int bitcount(u64 v)
+{
+	int	n;
+	for(n=0; v ; n++, v >>= 1) ;
+
+	return n;
+}
+
+u64 parse_size(char *s)
+{
+	int shift = 0;
+	u64 ret;
+	int n, i;
+
+	s = strdup(s);
+
+	for( i = 0 ; s[i] && isdigit(s[i]) ; i++ ) ; 
+	switch (tolower(s[i])) {
+		/* note: the yobibyte and the zebibyte don''t fit
+		   in a u64, they need an u128 !!! */
+		case ''y'':	/* yobibyte */
+			shift += 10;
+		case ''z'':	/* zebibyte */
+			shift += 10;
+		case ''e'':	/* exbibyte */
+			shift += 10;
+		case ''p'':	/* pebibyte */
+			shift += 10;
+		case ''t'':	/* tetibyte */
+			shift += 10;
+		case ''g'':	/* gibibyte */
+			shift += 10;
+		case ''m'':	/* mebibyte */
+			shift += 10;
+		case ''k'':	/* kibibyte */
+			shift += 10;
+		case 0:
+			break;
+		default:
+			fprintf(stderr, "ERROR: Unknown size descriptor %c\n", 
+				s[i]);
+			exit(1);
+	}
+	ret = strtoull(s, 0, 0);
+	n = bitcount(ret);
+
+	if( ( n + shift ) > ( sizeof(u64) * CHAR_BIT )) {
+		fprintf(stderr, "ERROR: Overflow, the value ''%s'' is
too big\n",
+			s);
+		fprintf(stderr, "ERROR: Abort\n");
+		exit(1);
+	}
+
+	return ret << shift;
+}
+
+
diff --git a/utils.h b/utils.h
index 3a0368b..180b3f9 100644
--- a/utils.h
+++ b/utils.h
@@ -46,4 +46,5 @@ int check_label(char *input);
 int get_mountpt(char *dev, char *mntpt, size_t size);
 
 int btrfs_scan_block_devices(int run_ioctl);
+u64 parse_size(char *s);
 #endif
-- 
1.7.10.4

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

Hi Goffredo

On 10/15/2012 21:15, Goffredo Baroncelli wrote:
> From: Goffredo Baroncelli <kreijack@inwind.it>
>
> Move parse_size() in utils.c, because it is used both from
> cmds-filesystem.c and mkfs.c.
Makes sense.
(Jan also sent such a patch on 1 Feb 2011 
<http://permalink.gmane.org/gmane.comp.file-systems.btrfs/9016>, as part 
of the patch for speed profiles and dedicated log devices. But the whole 
patch series was not integrated.)
> Moreover added check about overflow, support to further units (
> t=tera, e=exa, z=zetta, y=yotta).
Don''t make it too complicated, please. Btrfs cannot handle more than 
2^64 bytes.
> Correct a bug which happens if a value like 123MB is passed: before
> the function returned 123 instead of 123*1024*1024
Yes, that should be fixed. ''B'' is taken as the size multiplier
''byte''
(times one) and ''M'' is silently ignored.
> ---
>   cmds-filesystem.c |   26 -----------------------
>   mkfs.c            |   31 ---------------------------
>   utils.c           |   61
+++++++++++++++++++++++++++++++++++++++++++++++++++++
>   utils.h           |    1 +
>   4 files changed, 62 insertions(+), 57 deletions(-)
>
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index 9c43d35..507239a 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -311,32 +311,6 @@ static int cmd_sync(int argc, char **argv)
>   	return 0;
>   }
>
> -static u64 parse_size(char *s)
> -{
> -	int len = strlen(s);
> -	char c;
> -	u64 mult = 1;
> -
> -	if (!isdigit(s[len - 1])) {
> -		c = tolower(s[len - 1]);
> -		switch (c) {
> -		case ''g'':
> -			mult *= 1024;
> -		case ''m'':
> -			mult *= 1024;
> -		case ''k'':
> -			mult *= 1024;
> -		case ''b'':
> -			break;
> -		default:
> -			fprintf(stderr, "Unknown size descriptor %c\n", c);
> -			exit(1);
> -		}
> -		s[len - 1] = ''\0'';
> -	}
> -	return atoll(s) * mult;
> -}
> -
>   static int parse_compress_type(char *s)
>   {
>   	if (strcmp(optarg, "zlib") == 0)
> diff --git a/mkfs.c b/mkfs.c
> index 47f0c9c..ca850d9 100644
> --- a/mkfs.c
> +++ b/mkfs.c
> @@ -54,37 +54,6 @@ struct directory_name_entry {
>   	struct list_head list;
>   };
>
> -static u64 parse_size(char *s)
> -{
> -	int len = strlen(s);
> -	char c;
> -	u64 mult = 1;
> -	u64 ret;
> -
> -	s = strdup(s);
> -
> -	if (len && !isdigit(s[len - 1])) {
> -		c = tolower(s[len - 1]);
> -		switch (c) {
> -		case ''g'':
> -			mult *= 1024;
> -		case ''m'':
> -			mult *= 1024;
> -		case ''k'':
> -			mult *= 1024;
> -		case ''b'':
> -			break;
> -		default:
> -			fprintf(stderr, "Unknown size descriptor %c\n", c);
> -			exit(1);
> -		}
> -		s[len - 1] = ''\0'';
> -	}
> -	ret = atol(s) * mult;
> -	free(s);
> -	return ret;
> -}
> -
>   static int make_root_dir(struct btrfs_root *root, int mixed)
>   {
>   	struct btrfs_trans_handle *trans;
> diff --git a/utils.c b/utils.c
> index 205e667..b1bd669 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -1220,3 +1220,64 @@ scan_again:
>   	return 0;
>   }
>
> +/*
> + *  see http://gurmeet.net/puzzles/fast-bit-counting-routines/ for
> + *  further algorithms, I used the only one which I understood :-)
> + */
> +static int bitcount(u64 v)
> +{
> +	int	n;
> +	for(n=0; v ; n++, v >>= 1) ;
> +
> +	return n;
> +}
IMO, something like bitcount() is not needed, much too complicated for 
such a minor issue.
> +
> +u64 parse_size(char *s)
> +{
> +	int shift = 0;
> +	u64 ret;
> +	int n, i;
> +
> +	s = strdup(s);
Either don''t call strdup() or free() the memory at the end.
> +
> +	for( i = 0 ; s[i] && isdigit(s[i]) ; i++ ) ;
> +	switch (tolower(s[i])) {
> +		/* note: the yobibyte and the zebibyte don''t fit
> +		   in a u64, they need an u128 !!! */
If the comment is correct, please remove yobi, zebi and the comment :)
> +		case ''y'':	/* yobibyte */
> +			shift += 10;
> +		case ''z'':	/* zebibyte */
> +			shift += 10;
> +		case ''e'':	/* exbibyte */
> +			shift += 10;
> +		case ''p'':	/* pebibyte */
> +			shift += 10;
> +		case ''t'':	/* tetibyte */
> +			shift += 10;
> +		case ''g'':	/* gibibyte */
> +			shift += 10;
> +		case ''m'':	/* mebibyte */
> +			shift += 10;
> +		case ''k'':	/* kibibyte */
> +			shift += 10;
> +		case 0:
This should be "case ''b''" at the end in order to
maintain backward
compatibility.
> +			break;
> +		default:
> +			fprintf(stderr, "ERROR: Unknown size descriptor %c\n",
> +				s[i]);
> +			exit(1);
> +	}
> +	ret = strtoull(s, 0, 0);
> +	n = bitcount(ret);
> +
> +	if( ( n + shift ) > ( sizeof(u64) * CHAR_BIT )) {
> +		fprintf(stderr, "ERROR: Overflow, the value ''%s''
is too big\n",
> +			s);
> +		fprintf(stderr, "ERROR: Abort\n");
> +		exit(1);
> +	}
IMO, bitcount() and the check afterwards should be removed.
> +
> +	return ret << shift;
> +}
> +
> +
> diff --git a/utils.h b/utils.h
> index 3a0368b..180b3f9 100644
> --- a/utils.h
> +++ b/utils.h
> @@ -46,4 +46,5 @@ int check_label(char *input);
>   int get_mountpt(char *dev, char *mntpt, size_t size);
>
>   int btrfs_scan_block_devices(int run_ioctl);
> +u64 parse_size(char *s);
>   #endif
>
--
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

Hi Stefan,

On 2012-10-15 22:36, Stefan Behrens wrote:
> Hi Goffredo
>
> On 10/15/2012 21:15, Goffredo Baroncelli wrote:
>> From: Goffredo Baroncelli <kreijack@inwind.it>
>>
>> Move parse_size() in utils.c, because it is used both from
>> cmds-filesystem.c and mkfs.c.
>
> Makes sense.
> (Jan also sent such a patch on 1 Feb 2011 
> <http://permalink.gmane.org/gmane.comp.file-systems.btrfs/9016>, as 
> part of the patch for speed profiles and dedicated log devices. But 
> the whole patch series was not integrated.)
>
>> Moreover added check about overflow, support to further units (
>> t=tera, e=exa, z=zetta, y=yotta).
>
> Don''t make it too complicated, please. Btrfs cannot handle more
than
> 2^64 bytes.
yeaa I didn''t resisted :-)
>
>> Correct a bug which happens if a value like 123MB is passed: before
>> the function returned 123 instead of 123*1024*1024
>
> Yes, that should be fixed. ''B'' is taken as the size
multiplier ''byte''
> (times one) and ''M'' is silently ignored. 
> [....]
>>
>> +/*
>> + *  see http://gurmeet.net/puzzles/fast-bit-counting-routines/ for
>> + *  further algorithms, I used the only one which I understood :-)
>> + */
>> +static int bitcount(u64 v)
>> +{
>> +    int    n;
>> +    for(n=0; v ; n++, v >>= 1) ;
>> +
>> +    return n;
>> +}
>
> IMO, something like bitcount() is not needed, much too complicated for 
> such a minor issue.
I fear that if somebody pass something like 16E, it got un-predicible 
results. Let me to wait a day to see if anyone has a different opinion 
on it. If nobody tell otherwise I will remove this check.
>
>> +
>> +u64 parse_size(char *s)
>> +{
>> +    int shift = 0;
>> +    u64 ret;
>> +    int n, i;
>> +
>> +    s = strdup(s);
>
> Either don''t call strdup() or free() the memory at the end.
This was a my BUG, I removed the needing of strdup(), but I don''t
remove
the strdup() itself :-)
Thanks to catch it.
>
>> +
>> +    for( i = 0 ; s[i] && isdigit(s[i]) ; i++ ) ;
>> +    switch (tolower(s[i])) {
>> +        /* note: the yobibyte and the zebibyte don''t fit
>> +           in a u64, they need an u128 !!! */
>
> If the comment is correct, please remove yobi, zebi and the comment :)
ok... (but zfs supports zettabyte filesystem.... )
>
>> +        case ''y'':    /* yobibyte */
>> +            shift += 10;
>> +        case ''z'':    /* zebibyte */
>> +            shift += 10;
>> +        case ''e'':    /* exbibyte */
>> +            shift += 10;
>> +        case ''p'':    /* pebibyte */
>> +            shift += 10;
>> +        case ''t'':    /* tetibyte */
>> +            shift += 10;
>> +        case ''g'':    /* gibibyte */
>> +            shift += 10;
>> +        case ''m'':    /* mebibyte */
>> +            shift += 10;
>> +        case ''k'':    /* kibibyte */
>> +            shift += 10;
>> +        case 0:
>
> This should be "case ''b''" at the end in order
to maintain backward
> compatibility.
Ok, this make sense
>
>> +            break;
>> +        default:
>> +            fprintf(stderr, "ERROR: Unknown size descriptor
%c\n",
>> +                s[i]);
>> +            exit(1);
>> +    }
>> +    ret = strtoull(s, 0, 0);
>> +    n = bitcount(ret);
>> +
>> +    if( ( n + shift ) > ( sizeof(u64) * CHAR_BIT )) {
>> +        fprintf(stderr, "ERROR: Overflow, the value
''%s'' is too big\n",
>> +            s);
>> +        fprintf(stderr, "ERROR: Abort\n");
>> +        exit(1);
>> +    }
>
> IMO, bitcount() and the check afterwards should be removed.
>
>> +
>> +    return ret << shift;
>> +}
>> +
>> +
>> diff --git a/utils.h b/utils.h
>> index 3a0368b..180b3f9 100644
>> --- a/utils.h
>> +++ b/utils.h
>> @@ -46,4 +46,5 @@ int check_label(char *input);
>>   int get_mountpt(char *dev, char *mntpt, size_t size);
>>
>>   int btrfs_scan_block_devices(int run_ioctl);
>> +u64 parse_size(char *s);
>>   #endif
>>
>
>
I updated the patch on my repo (see first email), if someone want to 
make some tests. As explained above I will wait a day, then I will 
re-issue the patch.

BR
G.Baroncelli

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