Re: [PATCH 17/17 V2] btrfs-progs: replace strtok_r with strsep
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
> >> ---
> >>
> >> 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
> >
> > 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 17/17 V2] btrfs-progs: replace strtok_r with strsep
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
>> ---
>>
>> 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
>
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 17/17 V2] btrfs-progs: replace strtok_r with strsep
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
> ---
>
> 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
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 17/17 V2] btrfs-progs: replace strtok_r with strsep
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
---
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 , ","))) {
if ((value = strchr(this_char, '=')) != NULL)
*value++ = 0;
if (!strcmp(this_char, "profiles")) {
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
