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

[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

[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 |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/utils.c b/utils.c
index 732c782..de75309 100644
--- a/utils.c
+++ b/utils.c
@@ -1226,6 +1226,11 @@ u64 parse_size(char *s)
 	char c;
 	u64 mult = 1;
 
+	if( len <= 0 ){
+		fprintf(stderr, "ERROR: size value is empty\n");
+		exit(50);
+	}
+
 	if (!isdigit(s[len - 1])) {
 		c = tolower(s[len - 1]);
 		switch (c) {
@@ -1242,6 +1247,13 @@ u64 parse_size(char *s)
 			exit(1);
 		}
 		s[len - 1] = '\0';
+		len--;
+	}
+	if( len > 0 && !isdigit(s[len - 1])) {
+		fprintf(stderr, "ERROR: Illegal size value contains "
+			"non-digit character %c in wrong position\n",
+			s[len-1]);
+		exit(51);
 	}
 	return strtoull(s, NULL, 0) * 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 de75309..a5fabdc 100644
--- a/utils.c
+++ b/utils.c
@@ -1234,6 +1234,12 @@ u64 parse_size(char *s)
 	if (!isdigit(s[len - 1])) {
 		c = tolower(s[len - 1]);
 		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

[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