Stefan Behrens
2012-Oct-15 11:19 UTC
[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
Goffredo Baroncelli
2012-Oct-15 19:15 UTC
Re: [PATCH] Btrfs-progs: use atoll() for mkfs.btrfs size option
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*1024Yes, 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