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> -- 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 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
--
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
Goffredo Baroncelli
2012-Oct-22 20:17 UTC
[PATCH 2/5] parse_size(): replace atoll() with strtoull()
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 -- 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
Goffredo Baroncelli
2012-Oct-22 20:17 UTC
[PATCH 3/5] parse_size(): check for invalid suffix
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
--
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>
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
--
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
Goffredo Baroncelli
2012-Oct-22 20:17 UTC
[PATCH 5/5] Update the man page with the new prefixes.
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 -- 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