[PATCH][BTRFS-PROGS][V2] Update to parse_size()

[PATCH][BTRFS-PROGS][V2] Update to parse_size()

[Goffredo Baroncelli]

Hi all,

the following patches attempt to address some issues to the function
parse_size():
- this function is defined both in mkfs.c and cmds-filesystem.c; I 
moved it in utils.c (which is already used in both mkfs.btrfs and
btrfs) in order to avoid code duplication.
- it used the function atoll(); I replaceed atoll() with strtoull() 
because we are dealing with u64
- no check on suffixes was performed. If the user put 'MB' as suffix he got
bytes instead megabytes. The patches check the suffix is valid
- add new suffixes (t,p,e for terabytes, petabytes, exabytes)
- update the man page of the command mkfs.btrfs and 
"btrfs filesystem defragment", both use parse_size()

Several peoples (see cc's) suggested these improvements with different 
patches, I collected them togheter.

Please reviewed them, test them. Comments are welcome.

The patches are available also to 

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


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

Changelog:
V1: avoid to change the parse_size argument; 
    better check of a wrong suffix;
    force strtoull to use a decimal base

[PATCH 1/5] Move parse_size() to utils.[hc]

[Goffredo Baroncelli]

From: Goffredo Baroncelli <kreijack@inwind.it>

Move the function from cmds-filesystem.c and mkfs.c to utils.c
---
 cmds-filesystem.c |   26 --------------------------
 mkfs.c            |   31 -------------------------------
 utils.c           |   26 ++++++++++++++++++++++++++
 utils.h           |    2 ++
 4 files changed, 28 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..705be7b 100644
--- a/utils.c
+++ b/utils.c
@@ -1220,3 +1220,29 @@ scan_again:
 	return 0;
 }
 
+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;
+}
+
diff --git a/utils.h b/utils.h
index 3a0368b..714fd7a 100644
--- a/utils.h
+++ b/utils.h
@@ -46,4 +46,6 @@ 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

[PATCH 2/5] parse_size(): replace atoll() with strtoull()

[Goffredo Baroncelli]

From: Goffredo Baroncelli <kreijack@inwind.it>

Replace the function atoll with strtoull()
---
 utils.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utils.c b/utils.c
index 705be7b..732c782 100644
--- a/utils.c
+++ b/utils.c
@@ -1243,6 +1243,6 @@ u64 parse_size(char *s)
 		}
 		s[len - 1] = '\0';
 	}
-	return atoll(s) * mult;
+	return strtoull(s, NULL, 0) * mult;
 }
 
-- 
1.7.10.4

[PATCH 3/5] parse_size(): check for invalid suffix

[Goffredo Baroncelli]

From: Goffredo Baroncelli <kreijack@inwind.it>

Check that the suffix for the parse_size() input is of only
one character.
---
 utils.c |   25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/utils.c b/utils.c
index 732c782..e61cdea 100644
--- a/utils.c
+++ b/utils.c
@@ -31,6 +31,7 @@
 #include <fcntl.h>
 #include <unistd.h>
 #include <mntent.h>
+#include <ctype.h>
 #include <linux/loop.h>
 #include <linux/major.h>
 #include <linux/kdev_t.h>
@@ -1222,12 +1223,18 @@ scan_again:
 
 u64 parse_size(char *s)
 {
-	int len = strlen(s);
+	int i;
 	char c;
 	u64 mult = 1;
 
-	if (!isdigit(s[len - 1])) {
-		c = tolower(s[len - 1]);
+	for( i=0 ; s[i] && isdigit(s[i]) ; i++ ) ;
+	if( !i ){
+		fprintf(stderr, "ERROR: size value is empty\n");
+		exit(50);
+	}
+
+	if ( s[i] ) {
+		c = tolower(s[i]);
 		switch (c) {
 		case 'g':
 			mult *= 1024;
@@ -1238,11 +1245,17 @@ u64 parse_size(char *s)
 		case 'b':
 			break;
 		default:
-			fprintf(stderr, "Unknown size descriptor %c\n", c);
+			fprintf(stderr, "ERROR: Unknown size descriptor "
+				"'%c'\n", c);
 			exit(1);
 		}
-		s[len - 1] = '\0';
 	}
-	return strtoull(s, NULL, 0) * mult;
+	if( s[i] && s[i+1]) {
+		fprintf(stderr, "ERROR: Illegal suffix contains "
+			"character '%c' in wrong position\n",
+			s[i+1]);
+		exit(51);
+	}
+	return strtoull(s, NULL, 10) * mult;
 }
 
-- 
1.7.10.4

[PATCH 4/5] parse_size(): add new suffixes

[Goffredo Baroncelli]

From: Goffredo Baroncelli <kreijack@inwind.it>

Add new suffixes in parse_size() function. New suffixes are: T as
terabyte, P as petabyte, E as exabyte. Note these units are
multiply of 2 .
---
 utils.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/utils.c b/utils.c
index e61cdea..4b16fc9 100644
--- a/utils.c
+++ b/utils.c
@@ -1236,6 +1236,12 @@ u64 parse_size(char *s)
 	if ( s[i] ) {
 		c = tolower(s[i]);
 		switch (c) {
+		case 'e':
+			mult *= 1024;
+		case 'p':
+			mult *= 1024;
+		case 't':
+			mult *= 1024;
 		case 'g':
 			mult *= 1024;
 		case 'm':
-- 
1.7.10.4

[PATCH 5/5] Update the man page with the new prefixes.

[Goffredo Baroncelli]

From: Goffredo Baroncelli <kreijack@inwind.it>

---
 man/btrfs.8.in      |    3 +++
 man/mkfs.btrfs.8.in |    3 +++
 2 files changed, 6 insertions(+)

diff --git a/man/btrfs.8.in b/man/btrfs.8.in
index 9222580..33bd78d 100644
--- a/man/btrfs.8.in
+++ b/man/btrfs.8.in
@@ -184,6 +184,9 @@ defragment operations.
 
 \fB-t size\fP defragment only files at least \fIsize\fR bytes big
 
+For \fBstart\fP, \fBlen\fP, \fBsize\fP it is possible to append a prefix
+like \fBk\fP for 1 KBytes, \fBm\fP for 1 MBytes...
+
 NOTE: defragmenting with kernels up to 2.6.37 will unlink COW-ed copies of data,
 don't use it if you use snapshots, have de-duplicated your data or made
 copies with \fBcp --reflink\fP.
diff --git a/man/mkfs.btrfs.8.in b/man/mkfs.btrfs.8.in
index 72025ed..6f37cd8 100644
--- a/man/mkfs.btrfs.8.in
+++ b/man/mkfs.btrfs.8.in
@@ -69,6 +69,9 @@ Do not perform whole device TRIM operation by default.
 .TP
 \fB\-V\fR, \fB\-\-version\fR
 Print the \fBmkfs.btrfs\fP version and exit.
+.SH UNIT
+As default the unit is the byte, however it is possible to append a suffix
+to the arguments like \fBk\fP for KBytes, \fBm\fP for MBytes...
 .SH AVAILABILITY
 .B mkfs.btrfs
 is part of btrfs-progs. Btrfs is currently under heavy development,
-- 
1.7.10.4

Re: [PATCH 5/5] Update the man page with the new prefixes.

[David Sterba]

Hi,

one minor typo

On Tue, Oct 23, 2012 at 07:51:55PM +0200, Goffredo Baroncelli wrote:
> --- a/man/btrfs.8.in
> +++ b/man/btrfs.8.in
> @@ -184,6 +184,9 @@ defragment operations.
>  
>  \fB-t size\fP defragment only files at least \fIsize\fR bytes big
>  
> +For \fBstart\fP, \fBlen\fP, \fBsize\fP it is possible to append a prefix
                                                                     ^^^^^^
suffix

> +like \fBk\fP for 1 KBytes, \fBm\fP for 1 MBytes...
> +

david

Re: [PATCH 5/5] Update the man page with the new prefixes.

[Goffredo Baroncelli]

On 2012-10-25 18:13, David Sterba wrote:
> Hi,
> 
> one minor typo
> 
> On Tue, Oct 23, 2012 at 07:51:55PM +0200, Goffredo Baroncelli wrote:
>> --- a/man/btrfs.8.in
>> +++ b/man/btrfs.8.in
>> @@ -184,6 +184,9 @@ defragment operations.
>>  
>>  \fB-t size\fP defragment only files at least \fIsize\fR bytes big
>>  
>> +For \fBstart\fP, \fBlen\fP, \fBsize\fP it is possible to append a prefix
>                                                                      ^^^^^^
> suffix

Thanks, this week I will update the patches.

> 
>> +like \fBk\fP for 1 KBytes, \fBm\fP for 1 MBytes...
>> +
> 
> david
> .
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH][BTRFS-PROGS][V2] Update to parse_size()

[Goffredo Baroncelli]

Sorry the version is V1 and not V2

On Tue, Oct 23, 2012 at 7:51 PM, Goffredo Baroncelli <kreijack@gmail.com> wrote:
> Hi all,
>
> the following patches attempt to address some issues to the function
> parse_size():
> - this function is defined both in mkfs.c and cmds-filesystem.c; I
> moved it in utils.c (which is already used in both mkfs.btrfs and
> btrfs) in order to avoid code duplication.
> - it used the function atoll(); I replaceed atoll() with strtoull()
> because we are dealing with u64
> - no check on suffixes was performed. If the user put 'MB' as suffix he got
> bytes instead megabytes. The patches check the suffix is valid
> - add new suffixes (t,p,e for terabytes, petabytes, exabytes)
> - update the man page of the command mkfs.btrfs and
> "btrfs filesystem defragment", both use parse_size()
>
> Several peoples (see cc's) suggested these improvements with different
> patches, I collected them togheter.
>
> Please reviewed them, test them. Comments are welcome.
>
> The patches are available also to
>
> You can pull the patch from
>         http://cassiopea.homelinux.net/git/btrfs-progs-unstable.git
> branch
>         parse_size
>
>
> Signed-off-by: Goffredo Baroncelli <kreij...@inwind.it>
>
> Changelog:
> V1: avoid to change the parse_size argument;
>     better check of a wrong suffix;
>     force strtoull to use a decimal base
>

Re: [PATCH][BTRFS-PROGS][V2] Update to parse_size()

[David Sterba]

On Tue, Oct 23, 2012 at 08:48:08PM +0200, Goffredo Baroncelli wrote:
> Sorry the version is V1 and not V2

> > Changelog:
> > V1: avoid to change the parse_size argument;
> >     better check of a wrong suffix;
> >     force strtoull to use a decimal base

[I looked at patches at your git repo, not sure if this was the V2, but
the changelog seems to apply]

Code basically looks ok, the individual patches do not have your
Signed-off-by lines, please add them for the final version. There are a
few code style issues, like

+       for( i=0 ; s[i] && isdigit(s[i]) ; i++ ) ;
+       if( !i ){

ie. the spaces around keywords etc. Progs share lots of code from kernel
and thus this is the prevalent stylep, please keep it like that (unless
the maintainer is fine with your style of course :)

You can drop the patch "Replace the function atoll with strtoull()" and
fold the change into "Check that the suffix for the parse_size() input
is of only one character."

> On Tue, Oct 23, 2012 at 7:51 PM, Goffredo Baroncelli <kreijack@gmail.com> wrote:
> > the following patches attempt to address some issues to the function
> > parse_size():
> > - this function is defined both in mkfs.c and cmds-filesystem.c; I
> > moved it in utils.c (which is already used in both mkfs.btrfs and
> > btrfs) in order to avoid code duplication.
> > - it used the function atoll(); I replaceed atoll() with strtoull()
> > because we are dealing with u64
> > - no check on suffixes was performed. If the user put 'MB' as suffix he got
> > bytes instead megabytes. The patches check the suffix is valid
> > - add new suffixes (t,p,e for terabytes, petabytes, exabytes)
> > - update the man page of the command mkfs.btrfs and
> > "btrfs filesystem defragment", both use parse_size()

Nice set of updates, thanks.

david

[PATCH][BTRFS-PROGS] Update to parse_size()

[Goffredo Baroncelli]

Hi all,

the following patches attempt to address some issues to the function
parse_size():
- this function is defined both in mkfs.c and cmds-filesystem.c; I 
moved it in utils.c (which is already used in both mkfs.btrfs and
btrfs) in order to avoid code duplication.
- it used the function atoll(); I replaceed atoll() with strtoull() 
because we are dealing with u64
- no check on suffixes was performed. If the user put 'MB' as suffix he got
bytes instead megabytes. The patches check the suffix is valid
- add new suffixes (t,p,e for terabytes, petabytes, exabytes)
- update the man page of the command mkfs.btrfs and 
"btrfs filesystem defragment", both use parse_size()

Several peoples (see cc's) suggested these improvements with different 
patches, I collected them togheter.

Please reviewed them, test them. Comments are welcome.

The patches are available also to 

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


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

[PATCH 1/5] Move parse_size() to utils.[hc]

[Goffredo Baroncelli]

From: Goffredo Baroncelli <kreijack@inwind.it>

Move the function from cmds-filesystem.c and mkfs.c to utils.c
---
 cmds-filesystem.c |   26 --------------------------
 mkfs.c            |   31 -------------------------------
 utils.c           |   26 ++++++++++++++++++++++++++
 utils.h           |    2 ++
 4 files changed, 28 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..705be7b 100644
--- a/utils.c
+++ b/utils.c
@@ -1220,3 +1220,29 @@ scan_again:
 	return 0;
 }
 
+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;
+}
+
diff --git a/utils.h b/utils.h
index 3a0368b..714fd7a 100644
--- a/utils.h
+++ b/utils.h
@@ -46,4 +46,6 @@ 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