Re: setpriority(2): booleans are not scalars

[Theo de Raadt]

David Riley  wrote:

> On Sep 25, 2020, at 3:13 PM, Todd C. Miller  wrote:
> > 
> > On Fri, 25 Sep 2020 13:58:01 -0500, Scott Cheloha wrote:
> > 
> >> `found' serves as a boolean here.  I'd prefer to simple and set it to
> >> 1 instead of incrementing it when we find what we're looking for.
> > 
> > Makes sense to me, the use of var++ instead of var=1 is old-school
> > style ;-)
> 
> There are still some architectures where an increment vs. an immediate load 
> is a shorter instruction, but I strongly suspect the performance difference 
> (where there even is one) is not worth the unclarity here.

Which architecture is faster to load+increment+store, than store a constant?
I'd like to know.

But let's look deeper.  How about we fix OpenBSD to support 2GB or 4GB
of processes.  What happens next?

Alternatively on some architectures it is faster to increment a char than
an int, how about we change var to 'char var = 0'?  Still feeling good about
the change?

Premature optimization can be hazardous.  The ++ code as written carries
meanings and intents, which are incorrect.

Re: setpriority(2): booleans are not scalars

[David Riley]

On Sep 25, 2020, at 3:13 PM, Todd C. Miller  wrote:
> 
> On Fri, 25 Sep 2020 13:58:01 -0500, Scott Cheloha wrote:
> 
>> `found' serves as a boolean here.  I'd prefer to simple and set it to
>> 1 instead of incrementing it when we find what we're looking for.
> 
> Makes sense to me, the use of var++ instead of var=1 is old-school
> style ;-)

There are still some architectures where an increment vs. an immediate load is 
a shorter instruction, but I strongly suspect the performance difference (where 
there even is one) is not worth the unclarity here.


- Dave

Re: setpriority(2): booleans are not scalars

[Theo de Raadt]

We can't have 2 billion processes to reach a wrap.

But I agree, it seems quite wrong, and seems better to just observe
match rather than count.


Scott Cheloha  wrote:

> `found' serves as a boolean here.  I'd prefer to simple and set it to
> 1 instead of incrementing it when we find what we're looking for.
> 
> ok?
> 
> Index: kern_resource.c
> ===
> RCS file: /cvs/src/sys/kern/kern_resource.c,v
> retrieving revision 1.68
> diff -u -p -r1.68 kern_resource.c
> --- kern_resource.c   15 Jul 2019 20:44:48 -  1.68
> +++ kern_resource.c   25 Sep 2020 18:54:34 -
> @@ -157,7 +157,7 @@ sys_setpriority(struct proc *curp, void 
>   if (pr == NULL)
>   break;
>   error = donice(curp, pr, SCARG(uap, prio));
> - found++;
> + found = 1;
>   break;
>  
>   case PRIO_PGRP: {
> @@ -169,7 +169,7 @@ sys_setpriority(struct proc *curp, void 
>   break;
>   LIST_FOREACH(pr, >pg_members, ps_pglist) {
>   error = donice(curp, pr, SCARG(uap, prio));
> - found++;
> + found = 1;
>   }
>   break;
>   }
> @@ -180,14 +180,14 @@ sys_setpriority(struct proc *curp, void 
>   LIST_FOREACH(pr, , ps_list)
>   if (pr->ps_ucred->cr_uid == SCARG(uap, who)) {
>   error = donice(curp, pr, SCARG(uap, prio));
> - found++;
> + found = 1;
>   }
>   break;
>  
>   default:
>   return (EINVAL);
>   }
> - if (found == 0)
> + if (!found)
>   return (ESRCH);
>   return (error);
>  }
> 

Re: setpriority(2): booleans are not scalars

[Todd C . Miller]

On Fri, 25 Sep 2020 13:58:01 -0500, Scott Cheloha wrote:

> `found' serves as a boolean here.  I'd prefer to simple and set it to
> 1 instead of incrementing it when we find what we're looking for.

Makes sense to me, the use of var++ instead of var=1 is old-school
style ;-)

 - todd

setpriority(2): booleans are not scalars

[Scott Cheloha]

`found' serves as a boolean here.  I'd prefer to simple and set it to
1 instead of incrementing it when we find what we're looking for.

ok?

Index: kern_resource.c
===
RCS file: /cvs/src/sys/kern/kern_resource.c,v
retrieving revision 1.68
diff -u -p -r1.68 kern_resource.c
--- kern_resource.c 15 Jul 2019 20:44:48 -  1.68
+++ kern_resource.c 25 Sep 2020 18:54:34 -
@@ -157,7 +157,7 @@ sys_setpriority(struct proc *curp, void 
if (pr == NULL)
break;
error = donice(curp, pr, SCARG(uap, prio));
-   found++;
+   found = 1;
break;
 
case PRIO_PGRP: {
@@ -169,7 +169,7 @@ sys_setpriority(struct proc *curp, void 
break;
LIST_FOREACH(pr, >pg_members, ps_pglist) {
error = donice(curp, pr, SCARG(uap, prio));
-   found++;
+   found = 1;
}
break;
}
@@ -180,14 +180,14 @@ sys_setpriority(struct proc *curp, void 
LIST_FOREACH(pr, , ps_list)
if (pr->ps_ucred->cr_uid == SCARG(uap, who)) {
error = donice(curp, pr, SCARG(uap, prio));
-   found++;
+   found = 1;
}
break;
 
default:
return (EINVAL);
}
-   if (found == 0)
+   if (!found)
return (ESRCH);
return (error);
 }