| Message ID | 1361832890-40921-2-git-send-email-sandeen@redhat.com (mailing list archive) |
|---|---|
| State | Under Review, archived |
| Headers | show |
> case 'e': > mult *= 1024; > + /* Fallthrough */ These comments still annoy me :). And really, that code kind of annoys me too. That's a lot of duplicated code for a mapping of characters to powers of 1024. How about.. u64 pow_u64(u64 x, unsigned y) { u64 ret = 1; while (y--) ret *= x; return ret; } u64 get_mult(char unit) { static char *units = "bkmgtpe"; char *found = index(units, unit); if (found) return pow_u64(1024, found - units); return 0; } Seems like a lot less noise for the same functionality. - z -- 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 2/25/13 5:26 PM, Zach Brown wrote: >> case 'e': >> mult *= 1024; >> + /* Fallthrough */ > > These comments still annoy me :). it shuts up coverity & other static checkers which are worried about a missing break... > And really, that code kind of annoys > me too. That's a lot of duplicated code for a mapping of characters to > powers of 1024. > > How about.. > > u64 pow_u64(u64 x, unsigned y) > { > u64 ret = 1; > > while (y--) > ret *= x; > > return ret; > } > > u64 get_mult(char unit) > { > static char *units = "bkmgtpe"; > char *found = index(units, unit); > > if (found) > return pow_u64(1024, found - units); > return 0; > } > > Seems like a lot less noise for the same functionality. If you want to submit that patch as a further cleanup I'd review it ;) But TBH (going out on a limb crossing Zach here ;) my feeble brain is easily confused by your clever code. I'd rather just do the simple thing simply... -Eric > - z > -- 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
> > These comments still annoy me :). > > it shuts up coverity & other static checkers which are worried about > a missing break... Yeah, I know. And it's annoying! So my little brain fart there was an attempt to use a construct that simply couldn't be confused with mistakes that'd require comments to clear up. > If you want to submit that patch as a further cleanup I'd review it ;) Sure! > But TBH (going out on a limb crossing Zach here ;) my feeble brain > is easily confused by your clever code. I'd rather just do the > simple thing simply... Hrmph. pow() is hardly rocket science :). - z -- 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 02/25/2013 11:54 PM, Eric Sandeen wrote: > cmds-qgroup.c contained a parse_limit() function which > duplicates much of the functionality of parse_size. > The only unique behavior is to handle "none"; then we > can just pass it off to parse_size(). > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > cmds-qgroup.c | 44 ++++++-------------------------------------- > utils.c | 8 +++++++- > utils.h | 2 +- > 3 files changed, 14 insertions(+), 40 deletions(-) > > diff --git a/cmds-qgroup.c b/cmds-qgroup.c > index 26f0ab0..ce013c8 100644 > --- a/cmds-qgroup.c > +++ b/cmds-qgroup.c > @@ -198,43 +198,13 @@ done: > return ret; > } > > -static int parse_limit(const char *p, unsigned long long *s) > +static u64 parse_limit(const char *p) > { > - char *endptr; > - unsigned long long size; > - > - if (strcasecmp(p, "none") == 0) { > - *s = 0; > - return 1; > - } > - size = strtoull(p, &endptr, 10); > - switch (*endptr) { > - case 'T': > - case 't': > - size *= 1024; > - case 'G': > - case 'g': > - size *= 1024; > - case 'M': > - case 'm': > - size *= 1024; > - case 'K': > - case 'k': > - size *= 1024; > - ++endptr; > - break; > - case 0: > - break; > - default: > - return 0; > - } > - > - if (*endptr) > + if (strcasecmp(p, "none") == 0) > return 0; > > - *s = size; > - > - return 1; > + /* parse_size() will exit() on any error */ > + return parse_size(p); I don't think that this is a good thing to do: the parse_limit behaviour is the good one: return an error to the caller instead of exit()-ing. We should convert the parse_size() to return an error, no to convert parse_limit to exit(). Of course this is out of the goals of this set of patches. > } > > static const char * const cmd_qgroup_assign_usage[] = { > @@ -364,10 +334,8 @@ static int cmd_qgroup_limit(int argc, char **argv) > if (check_argc_min(argc - optind, 2)) > usage(cmd_qgroup_limit_usage); > > - if (!parse_limit(argv[optind], &size)) { > - fprintf(stderr, "Invalid size argument given\n"); > - return 1; > - } > + /* parse_limit will exit on any error */ > + size = parse_limit(argv[optind]); > > memset(&args, 0, sizeof(args)); > if (size) { > diff --git a/utils.c b/utils.c > index d660507..bc6d5fe 100644 > --- a/utils.c > +++ b/utils.c > @@ -1251,7 +1251,7 @@ scan_again: > return 0; > } > > -u64 parse_size(char *s) > +u64 parse_size(const char *s) > { > int i; > char c; > @@ -1268,16 +1268,22 @@ u64 parse_size(char *s) > switch (c) { > case 'e': > mult *= 1024; > + /* Fallthrough */ > case 'p': > mult *= 1024; > + /* Fallthrough */ > case 't': > mult *= 1024; > + /* Fallthrough */ > case 'g': > mult *= 1024; > + /* Fallthrough */ > case 'm': > mult *= 1024; > + /* Fallthrough */ > case 'k': > mult *= 1024; > + /* Fallthrough */ > case 'b': > break; > default: > diff --git a/utils.h b/utils.h > index 60a0fea..dcdf475 100644 > --- a/utils.h > +++ b/utils.h > @@ -47,7 +47,7 @@ char *pretty_sizes(u64 size); > 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); > +u64 parse_size(const char *s); > int open_file_or_dir(const char *fname); > int get_device_info(int fd, u64 devid, > struct btrfs_ioctl_dev_info_args *di_args);
On 2/26/13 12:50 PM, Goffredo Baroncelli wrote: > On 02/25/2013 11:54 PM, Eric Sandeen wrote: >> cmds-qgroup.c contained a parse_limit() function which >> duplicates much of the functionality of parse_size. >> The only unique behavior is to handle "none"; then we >> can just pass it off to parse_size(). >> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> --- >> cmds-qgroup.c | 44 ++++++-------------------------------------- >> utils.c | 8 +++++++- >> utils.h | 2 +- >> 3 files changed, 14 insertions(+), 40 deletions(-) >> >> diff --git a/cmds-qgroup.c b/cmds-qgroup.c >> index 26f0ab0..ce013c8 100644 >> --- a/cmds-qgroup.c >> +++ b/cmds-qgroup.c >> @@ -198,43 +198,13 @@ done: >> return ret; >> } >> >> -static int parse_limit(const char *p, unsigned long long *s) >> +static u64 parse_limit(const char *p) ... >> + /* parse_size() will exit() on any error */ >> + return parse_size(p); > > I don't think that this is a good thing to do: the parse_limit behaviour > is the good one: return an error to the caller instead of exit()-ing. > > We should convert the parse_size() to return an error, no to convert > parse_limit to exit(). Of course this is out of the goals of this set of > patches. Hm, fair point. I could either fix it before this patch, and add a 0.5/17, or add it to my next set of patches. What do you think? Thanks, -Eric -- 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 02/26/2013 09:17 PM, Eric Sandeen wrote: > On 2/26/13 12:50 PM, Goffredo Baroncelli wrote: >> On 02/25/2013 11:54 PM, Eric Sandeen wrote: >>> cmds-qgroup.c contained a parse_limit() function which >>> duplicates much of the functionality of parse_size. >>> The only unique behavior is to handle "none"; then we >>> can just pass it off to parse_size(). >>> >>> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >>> --- >>> cmds-qgroup.c | 44 ++++++-------------------------------------- >>> utils.c | 8 +++++++- >>> utils.h | 2 +- >>> 3 files changed, 14 insertions(+), 40 deletions(-) >>> >>> diff --git a/cmds-qgroup.c b/cmds-qgroup.c >>> index 26f0ab0..ce013c8 100644 >>> --- a/cmds-qgroup.c >>> +++ b/cmds-qgroup.c >>> @@ -198,43 +198,13 @@ done: >>> return ret; >>> } >>> >>> -static int parse_limit(const char *p, unsigned long long *s) >>> +static u64 parse_limit(const char *p) > > ... > >>> + /* parse_size() will exit() on any error */ >>> + return parse_size(p); >> >> I don't think that this is a good thing to do: the parse_limit behaviour >> is the good one: return an error to the caller instead of exit()-ing. >> >> We should convert the parse_size() to return an error, no to convert >> parse_limit to exit(). Of course this is out of the goals of this set of >> patches. > > Hm, fair point. I could either fix it before this patch, and add > a 0.5/17, or add it to my next set of patches. What do you think? I think that there is a general problem about parse_size/parse_limit/str[x]ll functions... We should address all these issues with another set of patches. Your set of patches is big enough, and now we should stabilise them in order to be accepted. If we agree that it is not good thing to allow parse_limit to call exit(), I will leave parse_limit() as is. If you are brave enough ( :) ) add the /* Fallthrough */ comment and add the peta and exa cases. In the future we could move parse_limit in utils.c then implement parse_size in terms of parse_limits and finally replace all the occurrences of strto[x]ll... > > Thanks, > -Eric > >
diff --git a/cmds-qgroup.c b/cmds-qgroup.c index 26f0ab0..ce013c8 100644 --- a/cmds-qgroup.c +++ b/cmds-qgroup.c @@ -198,43 +198,13 @@ done: return ret; } -static int parse_limit(const char *p, unsigned long long *s) +static u64 parse_limit(const char *p) { - char *endptr; - unsigned long long size; - - if (strcasecmp(p, "none") == 0) { - *s = 0; - return 1; - } - size = strtoull(p, &endptr, 10); - switch (*endptr) { - case 'T': - case 't': - size *= 1024; - case 'G': - case 'g': - size *= 1024; - case 'M': - case 'm': - size *= 1024; - case 'K': - case 'k': - size *= 1024; - ++endptr; - break; - case 0: - break; - default: - return 0; - } - - if (*endptr) + if (strcasecmp(p, "none") == 0) return 0; - *s = size; - - return 1; + /* parse_size() will exit() on any error */ + return parse_size(p); } static const char * const cmd_qgroup_assign_usage[] = { @@ -364,10 +334,8 @@ static int cmd_qgroup_limit(int argc, char **argv) if (check_argc_min(argc - optind, 2)) usage(cmd_qgroup_limit_usage); - if (!parse_limit(argv[optind], &size)) { - fprintf(stderr, "Invalid size argument given\n"); - return 1; - } + /* parse_limit will exit on any error */ + size = parse_limit(argv[optind]); memset(&args, 0, sizeof(args)); if (size) { diff --git a/utils.c b/utils.c index d660507..bc6d5fe 100644 --- a/utils.c +++ b/utils.c @@ -1251,7 +1251,7 @@ scan_again: return 0; } -u64 parse_size(char *s) +u64 parse_size(const char *s) { int i; char c; @@ -1268,16 +1268,22 @@ u64 parse_size(char *s) switch (c) { case 'e': mult *= 1024; + /* Fallthrough */ case 'p': mult *= 1024; + /* Fallthrough */ case 't': mult *= 1024; + /* Fallthrough */ case 'g': mult *= 1024; + /* Fallthrough */ case 'm': mult *= 1024; + /* Fallthrough */ case 'k': mult *= 1024; + /* Fallthrough */ case 'b': break; default: diff --git a/utils.h b/utils.h index 60a0fea..dcdf475 100644 --- a/utils.h +++ b/utils.h @@ -47,7 +47,7 @@ char *pretty_sizes(u64 size); 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); +u64 parse_size(const char *s); int open_file_or_dir(const char *fname); int get_device_info(int fd, u64 devid, struct btrfs_ioctl_dev_info_args *di_args);
cmds-qgroup.c contained a parse_limit() function which duplicates much of the functionality of parse_size. The only unique behavior is to handle "none"; then we can just pass it off to parse_size(). Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- cmds-qgroup.c | 44 ++++++-------------------------------------- utils.c | 8 +++++++- utils.h | 2 +- 3 files changed, 14 insertions(+), 40 deletions(-)