* [PATCH] Btrfs-progs: use atoll() for mkfs.btrfs size option @ 2012-10-15 11:19 Stefan Behrens 2012-10-15 19:15 ` Goffredo Baroncelli 0 siblings, 1 reply; 5+ messages in thread From: Stefan Behrens @ 2012-10-15 11:19 UTC (permalink / raw) To: linux-btrfs 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Btrfs-progs: use atoll() for mkfs.btrfs size option 2012-10-15 11:19 [PATCH] Btrfs-progs: use atoll() for mkfs.btrfs size option Stefan Behrens @ 2012-10-15 19:15 ` Goffredo Baroncelli 2012-10-15 19:15 ` [PATCH] Move parse_size() in utils.c Goffredo Baroncelli 0 siblings, 1 reply; 5+ messages in thread From: Goffredo Baroncelli @ 2012-10-15 19:15 UTC (permalink / raw) To: Goffredo Baroncelli; +Cc: Goffredo Baroncelli, linux-btrfs, Stefan Behrens 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] Move parse_size() in utils.c 2012-10-15 19:15 ` Goffredo Baroncelli @ 2012-10-15 19:15 ` Goffredo Baroncelli 2012-10-15 20:36 ` Stefan Behrens 0 siblings, 1 reply; 5+ messages in thread From: Goffredo Baroncelli @ 2012-10-15 19:15 UTC (permalink / raw) To: Goffredo Baroncelli; +Cc: Goffredo Baroncelli, linux-btrfs, Stefan Behrens 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Move parse_size() in utils.c 2012-10-15 19:15 ` [PATCH] Move parse_size() in utils.c Goffredo Baroncelli @ 2012-10-15 20:36 ` Stefan Behrens 2012-10-15 21:07 ` Goffredo Baroncelli 0 siblings, 1 reply; 5+ messages in thread From: Stefan Behrens @ 2012-10-15 20:36 UTC (permalink / raw) To: Goffredo Baroncelli; +Cc: Goffredo Baroncelli, linux-btrfs 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*1024 Yes, 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Move parse_size() in utils.c 2012-10-15 20:36 ` Stefan Behrens @ 2012-10-15 21:07 ` Goffredo Baroncelli 0 siblings, 0 replies; 5+ messages in thread From: Goffredo Baroncelli @ 2012-10-15 21:07 UTC (permalink / raw) To: Stefan Behrens; +Cc: Goffredo Baroncelli, linux-btrfs 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-10-15 21:07 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-10-15 11:19 [PATCH] Btrfs-progs: use atoll() for mkfs.btrfs size option Stefan Behrens 2012-10-15 19:15 ` Goffredo Baroncelli 2012-10-15 19:15 ` [PATCH] Move parse_size() in utils.c Goffredo Baroncelli 2012-10-15 20:36 ` Stefan Behrens 2012-10-15 21:07 ` Goffredo Baroncelli
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).