| Message ID | 512D190E.2050002@redhat.com (mailing list archive) |
|---|---|
| State | Under Review, archived |
| Headers | show |
On Tue, Feb 26, 2013 at 02:20:30PM -0600, Eric Sandeen wrote: > The coverity runs had a false positive complaining that save_ptr > is uninitialized in the call to strtok_r. > > We could initialize it, but Zach points out that just using > strsep is a lot simpler if there's only one delimiter, > so just switch to that. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > V2: Remove accidentally-added debug printfs, thanks Geoffredo! > > diff --git a/cmds-balance.c b/cmds-balance.c > index b671e1d..cfbb8eb 100644 > --- a/cmds-balance.c > +++ b/cmds-balance.c > @@ -67,11 +67,8 @@ static int parse_one_profile(const char *profile, u64 *flags) > static int parse_profiles(char *profiles, u64 *flags) > { > char *this_char; > - char *save_ptr; > > - for (this_char = strtok_r(profiles, "|", &save_ptr); > - this_char != NULL; > - this_char = strtok_r(NULL, "|", &save_ptr)) { > + while ((this_char = strsep(&profiles, "|"))) { > if (parse_one_profile(this_char, flags)) > return 1; > } > @@ -136,14 +133,11 @@ static int parse_filters(char *filters, struct btrfs_balance_args *args) > { > char *this_char; > char *value; > - char *save_ptr; > > if (!filters) > return 0; > > - for (this_char = strtok_r(filters, ",", &save_ptr); > - this_char != NULL; > - this_char = strtok_r(NULL, ",", &save_ptr)) { > + while ((this_char = strsep(&filters , ","))) { ^^^ whitespace One of the differences between strtok() and strsep() is that the former allows multiple delimiters between two tokens. With strsep(), this btrfs balance -dfoo1=bar1,,,foo2=bar2 <mnt> fails with error, whereas with strtok() it passes. I don't have a strong opinion here (this has been loosely modeled on the way mount(8) handles -o options), but might it be better to just initialize save_ptr? (And yes, I know that strsep() is better ;)) Thanks, Ilya -- 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/26/13 2:40 PM, Ilya Dryomov wrote: > On Tue, Feb 26, 2013 at 02:20:30PM -0600, Eric Sandeen wrote: >> The coverity runs had a false positive complaining that save_ptr >> is uninitialized in the call to strtok_r. >> >> We could initialize it, but Zach points out that just using >> strsep is a lot simpler if there's only one delimiter, >> so just switch to that. >> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> --- >> >> V2: Remove accidentally-added debug printfs, thanks Geoffredo! >> >> diff --git a/cmds-balance.c b/cmds-balance.c >> index b671e1d..cfbb8eb 100644 >> --- a/cmds-balance.c >> +++ b/cmds-balance.c >> @@ -67,11 +67,8 @@ static int parse_one_profile(const char *profile, u64 *flags) >> static int parse_profiles(char *profiles, u64 *flags) >> { >> char *this_char; >> - char *save_ptr; >> >> - for (this_char = strtok_r(profiles, "|", &save_ptr); >> - this_char != NULL; >> - this_char = strtok_r(NULL, "|", &save_ptr)) { >> + while ((this_char = strsep(&profiles, "|"))) { >> if (parse_one_profile(this_char, flags)) >> return 1; >> } >> @@ -136,14 +133,11 @@ static int parse_filters(char *filters, struct btrfs_balance_args *args) >> { >> char *this_char; >> char *value; >> - char *save_ptr; >> >> if (!filters) >> return 0; >> >> - for (this_char = strtok_r(filters, ",", &save_ptr); >> - this_char != NULL; >> - this_char = strtok_r(NULL, ",", &save_ptr)) { >> + while ((this_char = strsep(&filters , ","))) { > > ^^^ whitespace > > > One of the differences between strtok() and strsep() is that the former > allows multiple delimiters between two tokens. With strsep(), this > > btrfs balance -dfoo1=bar1,,,foo2=bar2 <mnt> > > fails with error, whereas with strtok() it passes. I don't have a > strong opinion here (this has been loosely modeled on the way mount(8) > handles -o options), but might it be better to just initialize save_ptr? > (And yes, I know that strsep() is better ;)) I don't really care much either way, TBH. Initializing it seems a little bit magic, but with a comment as to why, it'd be fine. If you did it this way intentionally to allow the above format, and changing it would break expectations, then I'll happily just initialize save_ptr. (And, I realize that lots of these changes are pedantic and seemingly pointless, but we've gotten the static checker errors down from over 100 to under 30 and dropping; the more noise we remove the more likely we are to pay attention to the output and catch actual errors. At least that's my feeling; if people think this is getting to be pointless churn, I'm ok with that, too). Thanks for the review, -Eric p.s. Zach made me do it. ;) > Thanks, > > Ilya > -- 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, Feb 26, 2013 at 02:46:30PM -0600, Eric Sandeen wrote: > On 2/26/13 2:40 PM, Ilya Dryomov wrote: > > On Tue, Feb 26, 2013 at 02:20:30PM -0600, Eric Sandeen wrote: > >> The coverity runs had a false positive complaining that save_ptr > >> is uninitialized in the call to strtok_r. > >> > >> We could initialize it, but Zach points out that just using > >> strsep is a lot simpler if there's only one delimiter, > >> so just switch to that. > >> > >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> > >> --- > >> > >> V2: Remove accidentally-added debug printfs, thanks Geoffredo! > >> > >> diff --git a/cmds-balance.c b/cmds-balance.c > >> index b671e1d..cfbb8eb 100644 > >> --- a/cmds-balance.c > >> +++ b/cmds-balance.c > >> @@ -67,11 +67,8 @@ static int parse_one_profile(const char *profile, u64 *flags) > >> static int parse_profiles(char *profiles, u64 *flags) > >> { > >> char *this_char; > >> - char *save_ptr; > >> > >> - for (this_char = strtok_r(profiles, "|", &save_ptr); > >> - this_char != NULL; > >> - this_char = strtok_r(NULL, "|", &save_ptr)) { > >> + while ((this_char = strsep(&profiles, "|"))) { > >> if (parse_one_profile(this_char, flags)) > >> return 1; > >> } > >> @@ -136,14 +133,11 @@ static int parse_filters(char *filters, struct btrfs_balance_args *args) > >> { > >> char *this_char; > >> char *value; > >> - char *save_ptr; > >> > >> if (!filters) > >> return 0; > >> > >> - for (this_char = strtok_r(filters, ",", &save_ptr); > >> - this_char != NULL; > >> - this_char = strtok_r(NULL, ",", &save_ptr)) { > >> + while ((this_char = strsep(&filters , ","))) { > > > > ^^^ whitespace > > > > > > One of the differences between strtok() and strsep() is that the former > > allows multiple delimiters between two tokens. With strsep(), this > > > > btrfs balance -dfoo1=bar1,,,foo2=bar2 <mnt> > > > > fails with error, whereas with strtok() it passes. I don't have a > > strong opinion here (this has been loosely modeled on the way mount(8) > > handles -o options), but might it be better to just initialize save_ptr? > > (And yes, I know that strsep() is better ;)) > > I don't really care much either way, TBH. Initializing it seems a little > bit magic, but with a comment as to why, it'd be fine. If you did it this > way intentionally to allow the above format, and changing it would break > expectations, then I'll happily just initialize save_ptr. Yeah, although I doubt anybody would notice, it's probably better to keep the old behaviour. > > (And, I realize that lots of these changes are pedantic and seemingly > pointless, but we've gotten the static checker errors down from over 100 > to under 30 and dropping; the more noise we remove the more likely we are > to pay attention to the output and catch actual errors. At least that's > my feeling; if people think this is getting to be pointless churn, I'm > ok with that, too). Not at all. Btrfs-progs had rarely seen any cleanups, with you and Zach patching it up and David's integration work it has gotten a whole new life ;) BTW, huge thanks for the kernel-style build output, I am forever grateful.. Thanks, Ilya -- 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
diff --git a/cmds-balance.c b/cmds-balance.c index b671e1d..cfbb8eb 100644 --- a/cmds-balance.c +++ b/cmds-balance.c @@ -67,11 +67,8 @@ static int parse_one_profile(const char *profile, u64 *flags) static int parse_profiles(char *profiles, u64 *flags) { char *this_char; - char *save_ptr; - for (this_char = strtok_r(profiles, "|", &save_ptr); - this_char != NULL; - this_char = strtok_r(NULL, "|", &save_ptr)) { + while ((this_char = strsep(&profiles, "|"))) { if (parse_one_profile(this_char, flags)) return 1; } @@ -136,14 +133,11 @@ static int parse_filters(char *filters, struct btrfs_balance_args *args) { char *this_char; char *value; - char *save_ptr; if (!filters) return 0; - for (this_char = strtok_r(filters, ",", &save_ptr); - this_char != NULL; - this_char = strtok_r(NULL, ",", &save_ptr)) { + while ((this_char = strsep(&filters , ","))) { if ((value = strchr(this_char, '=')) != NULL) *value++ = 0; if (!strcmp(this_char, "profiles")) {
The coverity runs had a false positive complaining that save_ptr is uninitialized in the call to strtok_r. We could initialize it, but Zach points out that just using strsep is a lot simpler if there's only one delimiter, so just switch to that. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- V2: Remove accidentally-added debug printfs, thanks Geoffredo! -- 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