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 -- 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-23 17:51 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-23 17:51 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 | 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 -- 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 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 -- 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-23 17:51 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
Goffredo Baroncelli
2012-Oct-23 18:48 UTC
Re: [PATCH][BTRFS-PROGS][V2] Update to parse_size()
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 >-- 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
David Sterba
2012-Oct-25 16:13 UTC
Re: [PATCH 5/5] Update the man page with the new prefixes.
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 -- 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
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 -- 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-25 19:22 UTC
Re: [PATCH 5/5] Update the man page with the new prefixes.
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 > ^^^^^^ > suffixThanks, 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 -- 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