Re: iked(8): boolify

[Wataru Ashihara]

Hi, thank you for sharing your thoughts, Tobias.

On 2020/04/03 21:31
Tobias Heider  wrote:

> On Fri, Apr 03, 2020 at 12:52:24AM +0900, Wataru Ashihara wrote:
> > It would save our time of thinking and reading the source (i.e.
> > eliminate the process of "what if the variable 'mobike' was 2 or more?
> > ...aha it's just a bool").
> > 
> > This is still work in progress. I would continue if you maintainers are
> > positive on this proposal.
> 
> Hi,
> 
> i think using bool is not generally a bad idea but I am somewhat sceptical
> that the benefits of this really outweight the dangers of introducing new
> bugs in otherwise working code.
> 
> Another concern of mine is that I would prefer our codebase to be coherent
> in style (which currently means not using ). Having a coherent
> style is pretty important because it allows developers to work in any
> part of our code base without changing their habits.
> 
> We have also had discussions about the use of stdbool.h before. Here is a
> link a recent one with some more opinions on the topic:
> https://marc.info/?l=openbsd-tech=157427044406050=2

It makes sense to me. Unfortunately clang and gcc can't raise errors
about implicit conversion between bool and int. I didn't consider much
about introducing new bugs because I've been helped by clang-tidy, which
warns about it[1], but not everyone uses it... (at least at this time)

[1]: 
http://clang.llvm.org/extra/clang-tidy/checks/readability-implicit-bool-conversion.html

I give up this patch, but I still believe that the self-explanatory type
of bool must be informative especially for newbies, so now I hope this
style would be proposed as a part of style(9) if compilers implement
-Wbool-blah-blah or OpenBSD starts its CI with clang-tidy...

> 
> - Tobias
> 
> 

Regards,
Wataru

Re: iked(8): boolify

[Tobias Heider]

On Fri, Apr 03, 2020 at 12:52:24AM +0900, Wataru Ashihara wrote:
> It would save our time of thinking and reading the source (i.e.
> eliminate the process of "what if the variable 'mobike' was 2 or more?
> ...aha it's just a bool").
> 
> This is still work in progress. I would continue if you maintainers are
> positive on this proposal.

Hi,

i think using bool is not generally a bad idea but I am somewhat sceptical
that the benefits of this really outweight the dangers of introducing new
bugs in otherwise working code.

Another concern of mine is that I would prefer our codebase to be coherent
in style (which currently means not using ). Having a coherent
style is pretty important because it allows developers to work in any
part of our code base without changing their habits.

We have also had discussions about the use of stdbool.h before. Here is a
link a recent one with some more opinions on the topic:
https://marc.info/?l=openbsd-tech=157427044406050=2

- Tobias

iked(8): boolify

[Wataru Ashihara]

It would save our time of thinking and reading the source (i.e.
eliminate the process of "what if the variable 'mobike' was 2 or more?
...aha it's just a bool").

This is still work in progress. I would continue if you maintainers are
positive on this proposal.


Index: sbin/iked/config.c
===
RCS file: /cvs/src/sbin/iked/config.c,v
retrieving revision 1.55
diff -u -r1.55 config.c
--- sbin/iked/config.c  24 Mar 2020 13:32:36 -  1.55
+++ sbin/iked/config.c  2 Apr 2020 15:45:44 -
@@ -22,6 +22,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 #include 
@@ -39,7 +40,7 @@
 #include "ikev2.h"
 
 struct iked_sa *
-config_new_sa(struct iked *env, int initiator)
+config_new_sa(struct iked *env, bool initiator)
 {
struct iked_sa  *sa;
 
@@ -451,7 +452,7 @@
  */
 
 int
-config_setcoupled(struct iked *env, unsigned int couple)
+config_setcoupled(struct iked *env, bool couple)
 {
unsigned int type;
 
@@ -465,11 +466,11 @@
 config_getcoupled(struct iked *env, unsigned int type)
 {
return (pfkey_couple(env->sc_pfkey, >sc_sas,
-   type == IMSG_CTL_COUPLE ? 1 : 0));
+   type == IMSG_CTL_COUPLE));
 }
 
 int
-config_setmode(struct iked *env, unsigned int passive)
+config_setmode(struct iked *env, bool passive)
 {
unsigned int type;
 
@@ -482,17 +483,17 @@
 int
 config_getmode(struct iked *env, unsigned int type)
 {
-   uint8_t  old;
+   bool old;
unsigned char   *mode[] = { "active", "passive" };
 
-   old = env->sc_passive ? 1 : 0;
-   env->sc_passive = type == IMSG_CTL_PASSIVE ? 1 : 0;
+   old = env->sc_passive;
+   env->sc_passive = (type == IMSG_CTL_PASSIVE);
 
if (old == env->sc_passive)
return (0);
 
log_debug("%s: mode %s -> %s", __func__,
-   mode[old], mode[env->sc_passive]);
+   mode[old ? 1 : 0], mode[env->sc_passive ? 1 : 0]);
 
return (0);
 }
@@ -848,22 +849,22 @@
 int
 config_setmobike(struct iked *env)
 {
-   unsigned int boolval;
+   bool val;
 
-   boolval = env->sc_mobike;
+   val = env->sc_mobike;
proc_compose(>sc_ps, PROC_IKEV2, IMSG_CTL_MOBIKE,
-   , sizeof(boolval));
+   , sizeof(val));
return (0);
 }
 
 int
 config_getmobike(struct iked *env, struct imsg *imsg)
 {
-   unsigned int boolval;
+   bool mobike;
 
-   IMSG_SIZE_CHECK(imsg, );
-   memcpy(, imsg->data, sizeof(boolval));
-   env->sc_mobike = boolval;
+   IMSG_SIZE_CHECK(imsg, );
+   memcpy(, imsg->data, sizeof(mobike));
+   env->sc_mobike = mobike;
log_debug("%s: %smobike", __func__, env->sc_mobike ? "" : "no ");
return (0);
 }
@@ -871,22 +872,22 @@
 int
 config_setfragmentation(struct iked *env)
 {
-   unsigned int boolval;
+   bool fragmentation;
 
-   boolval = env->sc_frag;
+   fragmentation = env->sc_frag;
proc_compose(>sc_ps, PROC_IKEV2, IMSG_CTL_FRAGMENTATION,
-   , sizeof(boolval));
+   , sizeof(fragmentation));
return (0);
 }
 
 int
 config_getfragmentation(struct iked *env, struct imsg *imsg)
 {
-   unsigned int boolval;
+   bool fragmentation;
 
-   IMSG_SIZE_CHECK(imsg, );
-   memcpy(, imsg->data, sizeof(boolval));
-   env->sc_frag = boolval;
+   IMSG_SIZE_CHECK(imsg, );
+   memcpy(, imsg->data, sizeof(fragmentation));
+   env->sc_frag = fragmentation;
log_debug("%s: %sfragmentation", __func__, env->sc_frag ? "" : "no ");
return (0);
 }
Index: sbin/iked/crypto.c
===
RCS file: /cvs/src/sbin/iked/crypto.c,v
retrieving revision 1.23
diff -u -r1.23 crypto.c
--- sbin/iked/crypto.c  14 Feb 2020 13:02:31 -  1.23
+++ sbin/iked/crypto.c  2 Apr 2020 15:45:44 -
@@ -21,6 +21,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 #include 
@@ -504,7 +505,7 @@
if (prf == NULL || prf->hash_priv == NULL)
fatalx("dsa_new: invalid PRF");
dsa.dsa_priv = prf->hash_priv;
-   dsa.dsa_hmac = 1;
+   dsa.dsa_hmac = true;
break;
case IKEV2_AUTH_DSS_SIG:
dsa.dsa_priv = EVP_dss1();
Index: sbin/iked/iked.c
===
RCS file: /cvs/src/sbin/iked/iked.c,v
retrieving revision 1.41
diff -u -r1.41 iked.c
--- sbin/iked/iked.c16 Jan 2020 20:05:00 -  1.41
+++ sbin/iked/iked.c2 Apr 2020 15:45:44 -
@@ -22,6 +22,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 #include 
@@ -261,10 +262,10 @@
config_setmobike(env);
config_setfragmentation(env);
config_setnattport(env);
-   config_setcoupled(env, env->sc_decoupled ? 0 : 1);
+   config_setcoupled(env, !env->sc_decoupled);
config_setocsp(env);
/* Must