[PATCH v4 6/8] sched: Replace strncmp with str_has_prefix

[PATCH v4 6/8] sched: Replace strncmp with str_has_prefix

[Chuhong Yuan]

strncmp(str, const, len) is error-prone because len
is easy to have typo.
The example is the hard-coded len has counting error
or sizeof(const) forgets - 1.
So we prefer using newly introduced str_has_prefix()
to substitute such strncmp to make code better.

Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
---
Changes in v4:
  - Eliminate assignments in if conditions.

 kernel/sched/debug.c     |  6 ++++--
 kernel/sched/isolation.c | 11 +++++++----
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
index f7e4579e746c..a03900523e5d 100644
--- a/kernel/sched/debug.c
+++ b/kernel/sched/debug.c
@@ -102,10 +102,12 @@ static int sched_feat_set(char *cmp)
 {
 	int i;
 	int neg = 0;
+	size_t len;
 
-	if (strncmp(cmp, "NO_", 3) == 0) {
+	len = str_has_prefix(cmp, "NO_");
+	if (len) {
 		neg = 1;
-		cmp += 3;
+		cmp += len;
 	}
 
 	i = match_string(sched_feat_names, __SCHED_FEAT_NR, cmp);
diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
index ccb28085b114..ea2ead4b1906 100644
--- a/kernel/sched/isolation.c
+++ b/kernel/sched/isolation.c
@@ -141,16 +141,19 @@ __setup("nohz_full=", housekeeping_nohz_full_setup);
 static int __init housekeeping_isolcpus_setup(char *str)
 {
 	unsigned int flags = 0;
+	size_t len;
 
 	while (isalpha(*str)) {
-		if (!strncmp(str, "nohz,", 5)) {
-			str += 5;
+		len = str_has_prefix(str, "nohz,");
+		if (len) {
+			str += len;
 			flags |= HK_FLAG_TICK;
 			continue;
 		}
 
-		if (!strncmp(str, "domain,", 7)) {
-			str += 7;
+		len = str_has_prefix(str, "domain,");
+		if (len) {
+			str += len;
 			flags |= HK_FLAG_DOMAIN;
 			continue;
 		}
-- 
2.20.1

Re: [PATCH v4 6/8] sched: Replace strncmp with str_has_prefix

[Valentin Schneider]

On 09/08/2019 08:10, Chuhong Yuan wrote:
> strncmp(str, const, len) is error-prone because len
> is easy to have typo.
> The example is the hard-coded len has counting error
> or sizeof(const) forgets - 1.
> So we prefer using newly introduced str_has_prefix()
> to substitute such strncmp to make code better.
> 
> Signed-off-by: Chuhong Yuan <hslester96@gmail.com>

I tried to have a look at the series as a whole but it's not properly
threaded (or at least doesn't appear as such on lore), which makes it
unnecessarily annoying to review.

Please make sure to use git-send-email, which should properly thread all
patches (IOW make them in-reply-to the cover letter).


Other than that, I stared at it and it seems fine. It's not that helpful
here since I doubt any of these prefixes will change in the near feature,
but hey, why not.

Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>

> ---
> Changes in v4:
>   - Eliminate assignments in if conditions.
> 
>  kernel/sched/debug.c     |  6 ++++--
>  kernel/sched/isolation.c | 11 +++++++----
>  2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
> index f7e4579e746c..a03900523e5d 100644
> --- a/kernel/sched/debug.c
> +++ b/kernel/sched/debug.c
> @@ -102,10 +102,12 @@ static int sched_feat_set(char *cmp)
>  {
>  	int i;
>  	int neg = 0;
> +	size_t len;
>  
> -	if (strncmp(cmp, "NO_", 3) == 0) {
> +	len = str_has_prefix(cmp, "NO_");
> +	if (len) {
>  		neg = 1;
> -		cmp += 3;
> +		cmp += len;
>  	}
>  
>  	i = match_string(sched_feat_names, __SCHED_FEAT_NR, cmp);
> diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
> index ccb28085b114..ea2ead4b1906 100644
> --- a/kernel/sched/isolation.c
> +++ b/kernel/sched/isolation.c
> @@ -141,16 +141,19 @@ __setup("nohz_full=", housekeeping_nohz_full_setup);
>  static int __init housekeeping_isolcpus_setup(char *str)
>  {
>  	unsigned int flags = 0;
> +	size_t len;
>  
>  	while (isalpha(*str)) {
> -		if (!strncmp(str, "nohz,", 5)) {
> -			str += 5;
> +		len = str_has_prefix(str, "nohz,");
> +		if (len) {
> +			str += len;
>  			flags |= HK_FLAG_TICK;
>  			continue;
>  		}
>  
> -		if (!strncmp(str, "domain,", 7)) {
> -			str += 7;
> +		len = str_has_prefix(str, "domain,");
> +		if (len) {
> +			str += len;
>  			flags |= HK_FLAG_DOMAIN;
>  			continue;
>  		}
> 

Re: [PATCH v4 6/8] sched: Replace strncmp with str_has_prefix

[Chuhong Yuan]

On Fri, Aug 9, 2019 at 7:31 PM Valentin Schneider
<valentin.schneider@arm.com> wrote:
>
> On 09/08/2019 08:10, Chuhong Yuan wrote:
> > strncmp(str, const, len) is error-prone because len
> > is easy to have typo.
> > The example is the hard-coded len has counting error
> > or sizeof(const) forgets - 1.
> > So we prefer using newly introduced str_has_prefix()
> > to substitute such strncmp to make code better.
> >
> > Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
>
> I tried to have a look at the series as a whole but it's not properly
> threaded (or at least doesn't appear as such on lore), which makes it
> unnecessarily annoying to review.
>
> Please make sure to use git-send-email, which should properly thread all
> patches (IOW make them in-reply-to the cover letter).
>

I have used git-send-email to send the series with a cover letter.
The cover letter is here:
https://lkml.org/lkml/2019/8/9/108

Indeed I find the series are not in the same thread, I am not sure
what is wrong with them.

>
> Other than that, I stared at it and it seems fine. It's not that helpful
> here since I doubt any of these prefixes will change in the near feature,
> but hey, why not.
>
> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>
> > ---
> > Changes in v4:
> >   - Eliminate assignments in if conditions.
> >
> >  kernel/sched/debug.c     |  6 ++++--
> >  kernel/sched/isolation.c | 11 +++++++----
> >  2 files changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
> > index f7e4579e746c..a03900523e5d 100644
> > --- a/kernel/sched/debug.c
> > +++ b/kernel/sched/debug.c
> > @@ -102,10 +102,12 @@ static int sched_feat_set(char *cmp)
> >  {
> >       int i;
> >       int neg = 0;
> > +     size_t len;
> >
> > -     if (strncmp(cmp, "NO_", 3) == 0) {
> > +     len = str_has_prefix(cmp, "NO_");
> > +     if (len) {
> >               neg = 1;
> > -             cmp += 3;
> > +             cmp += len;
> >       }
> >
> >       i = match_string(sched_feat_names, __SCHED_FEAT_NR, cmp);
> > diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
> > index ccb28085b114..ea2ead4b1906 100644
> > --- a/kernel/sched/isolation.c
> > +++ b/kernel/sched/isolation.c
> > @@ -141,16 +141,19 @@ __setup("nohz_full=", housekeeping_nohz_full_setup);
> >  static int __init housekeeping_isolcpus_setup(char *str)
> >  {
> >       unsigned int flags = 0;
> > +     size_t len;
> >
> >       while (isalpha(*str)) {
> > -             if (!strncmp(str, "nohz,", 5)) {
> > -                     str += 5;
> > +             len = str_has_prefix(str, "nohz,");
> > +             if (len) {
> > +                     str += len;
> >                       flags |= HK_FLAG_TICK;
> >                       continue;
> >               }
> >
> > -             if (!strncmp(str, "domain,", 7)) {
> > -                     str += 7;
> > +             len = str_has_prefix(str, "domain,");
> > +             if (len) {
> > +                     str += len;
> >                       flags |= HK_FLAG_DOMAIN;
> >                       continue;
> >               }
> >

Re: [PATCH v4 6/8] sched: Replace strncmp with str_has_prefix

[Peter Zijlstra]

On Fri, Aug 09, 2019 at 03:10:51PM +0800, Chuhong Yuan wrote:
> strncmp(str, const, len) is error-prone because len
> is easy to have typo.

I'm thinking that is exactly the easy case for compilers/semantic
checkers to verify. Now granted, GCC doesn't seem to do that by itself,
but still.

I'd buy your argument if the prefix is variable, because in that case
you can do prefix matching cheaper than strlen+strncmp, but as is, not
really.