Libguestfs - Jan 2019 - Re: [PATCH nbdkit] server: Use bool for types which are really booleans.

For mainly historical reasons we tended to use int to store boolean
values.  However using bool is probably safer in some corner cases
(eg. ?v == true? can fail badly if v is an int, but works for bool).
bool was added in C99 so let's use it.
---
 server/internal.h    | 16 +++++------
 server/connections.c | 28 +++++++++---------
 server/crypto.c      |  4 +--
 server/main.c        | 67 ++++++++++++++++++++++----------------------
 server/plugins.c     |  2 +-
 5 files changed, 59 insertions(+), 58 deletions(-)

diff --git a/server/internal.h b/server/internal.h
index b13685e..76d3a6d 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -79,7 +79,7 @@ struct debug_flag {
   char *name;                   /* plugin or filter name */
   char *flag;                   /* flag name */
   int value;                    /* value of flag */
-  int used;                     /* if flag was successfully set */
+  bool used;                    /* if flag was successfully set */
 };
 
 enum log_to {
@@ -94,22 +94,22 @@ extern struct debug_flag *debug_flags;
 extern const char *exportname;
 extern const char *ipaddr;
 extern enum log_to log_to;
-extern int newstyle;
+extern bool newstyle;
 extern const char *port;
-extern int readonly;
+extern bool readonly;
 extern const char *selinux_label;
+extern int threads;
 extern int tls;
 extern const char *tls_certificates_dir;
 extern const char *tls_psk;
-extern int tls_verify_peer;
+extern bool tls_verify_peer;
 extern char *unixsocket;
-extern int verbose;
-extern int threads;
+extern bool verbose;
 
 extern volatile int quit;
 extern int quit_fd;
 
-extern int forked_into_background;
+extern bool forked_into_background;
 
 extern struct backend *backend;
 #define for_each_backend(b) for (b = backend; b != NULL; b = b->next)
@@ -140,7 +140,7 @@ extern void connection_set_close (struct connection *,
connection_close_function
 
 /* crypto.c */
 #define root_tls_certificates_dir sysconfdir "/pki/" PACKAGE_NAME
-extern void crypto_init (int tls_set_on_cli);
+extern void crypto_init (bool tls_set_on_cli);
 extern void crypto_free (void);
 extern int crypto_negotiate_tls (struct connection *conn, int sockin, int
sockout);
 
diff --git a/server/connections.c b/server/connections.c
index 0d1bd74..0a89315 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -78,13 +78,13 @@ struct connection {
   uint32_t cflags;
   uint64_t exportsize;
   uint16_t eflags;
-  int readonly;
-  int can_flush;
-  int is_rotational;
-  int can_trim;
-  int can_zero;
-  int can_fua;
-  int using_tls;
+  bool readonly;
+  bool can_flush;
+  bool is_rotational;
+  bool can_trim;
+  bool can_zero;
+  bool can_fua;
+  bool using_tls;
 
   int sockin, sockout;
   connection_recv_function recv;
@@ -419,7 +419,7 @@ compute_eflags (struct connection *conn, uint16_t *flags)
     return -1;
   if (readonly || !fl) {
     eflags |= NBD_FLAG_READ_ONLY;
-    conn->readonly = 1;
+    conn->readonly = true;
   }
   if (!conn->readonly) {
     fl = backend->can_zero (backend, conn);
@@ -427,7 +427,7 @@ compute_eflags (struct connection *conn, uint16_t *flags)
       return -1;
     if (fl) {
       eflags |= NBD_FLAG_SEND_WRITE_ZEROES;
-      conn->can_zero = 1;
+      conn->can_zero = true;
     }
 
     fl = backend->can_trim (backend, conn);
@@ -435,7 +435,7 @@ compute_eflags (struct connection *conn, uint16_t *flags)
       return -1;
     if (fl) {
       eflags |= NBD_FLAG_SEND_TRIM;
-      conn->can_trim = 1;
+      conn->can_trim = true;
     }
 
     fl = backend->can_fua (backend, conn);
@@ -443,7 +443,7 @@ compute_eflags (struct connection *conn, uint16_t *flags)
       return -1;
     if (fl) {
       eflags |= NBD_FLAG_SEND_FUA;
-      conn->can_fua = 1;
+      conn->can_fua = true;
     }
   }
 
@@ -452,7 +452,7 @@ compute_eflags (struct connection *conn, uint16_t *flags)
     return -1;
   if (fl) {
     eflags |= NBD_FLAG_SEND_FLUSH;
-    conn->can_flush = 1;
+    conn->can_flush = true;
   }
 
   fl = backend->is_rotational (backend, conn);
@@ -460,7 +460,7 @@ compute_eflags (struct connection *conn, uint16_t *flags)
     return -1;
   if (fl) {
     eflags |= NBD_FLAG_ROTATIONAL;
-    conn->is_rotational = 1;
+    conn->is_rotational = true;
   }
 
   *flags = eflags;
@@ -793,7 +793,7 @@ _negotiate_handshake_newstyle_options (struct connection
*conn)
         /* Upgrade the connection to TLS.  Also performs access control. */
         if (crypto_negotiate_tls (conn, conn->sockin, conn->sockout) ==
-1)
           return -1;
-        conn->using_tls = 1;
+        conn->using_tls = true;
         debug ("using TLS on this connection");
       }
       break;
diff --git a/server/crypto.c b/server/crypto.c
index 5b01684..4638a69 100644
--- a/server/crypto.c
+++ b/server/crypto.c
@@ -230,7 +230,7 @@ start_psk (void)
  * and loading the server certificate.
  */
 void
-crypto_init (int tls_set_on_cli)
+crypto_init (bool tls_set_on_cli)
 {
   int err, r;
   const char *what;
@@ -521,7 +521,7 @@ crypto_negotiate_tls (struct connection *conn, int sockin,
int sockout)
  */
 
 void
-crypto_init (int tls_set_on_cli)
+crypto_init (bool tls_set_on_cli)
 {
   if (tls > 0) {
     fprintf (stderr,
diff --git a/server/main.c b/server/main.c
index e8a15f7..5e538bf 100644
--- a/server/main.c
+++ b/server/main.c
@@ -35,6 +35,7 @@
 
 #include <stdio.h>
 #include <stdlib.h>
+#include <stdbool.h>
 #include <string.h>
 #include <fcntl.h>
 #include <unistd.h>
@@ -75,26 +76,26 @@ static unsigned int get_socket_activation (void);
 static int is_config_key (const char *key, size_t len);
 
 struct debug_flag *debug_flags; /* -D */
-int exit_with_parent;           /* --exit-with-parent */
+bool exit_with_parent;          /* --exit-with-parent */
 const char *exportname;         /* -e */
-int foreground;                 /* -f */
+bool foreground;                /* -f */
 const char *ipaddr;             /* -i */
 enum log_to log_to = LOG_TO_DEFAULT; /* --log */
-int newstyle = 1;               /* 0 = -o, 1 = -n */
+bool newstyle = true;           /* false = -o, true = -n */
 char *pidfile;                  /* -P */
 const char *port;               /* -p */
-int readonly;                   /* -r */
+bool readonly;                  /* -r */
 char *run;                      /* --run */
-int listen_stdin;               /* -s */
+bool listen_stdin;              /* -s */
 const char *selinux_label;      /* --selinux-label */
 int threads;                    /* -t */
 int tls;                        /* --tls : 0=off 1=on 2=require */
 const char *tls_certificates_dir; /* --tls-certificates */
 const char *tls_psk;            /* --tls-psk */
-int tls_verify_peer;            /* --tls-verify-peer */
+bool tls_verify_peer;           /* --tls-verify-peer */
 char *unixsocket;               /* -U */
 const char *user, *group;       /* -u & -g */
-int verbose;                    /* -v */
+bool verbose;                   /* -v */
 unsigned int socket_activation  /* $LISTEN_FDS and $LISTEN_PID set */;
 
 /* Detection of request to exit via signal.  Most places in the code
@@ -106,7 +107,7 @@ int quit_fd;
 static int write_quit_fd;
 
 /* True if we forked into the background (used to control log messages). */
-int forked_into_background;
+bool forked_into_background;
 
 /* The currently loaded plugin. */
 struct backend *backend;
@@ -160,8 +161,8 @@ int
 main (int argc, char *argv[])
 {
   int c;
-  int help = 0, version = 0, dump_plugin = 0;
-  int tls_set_on_cli = 0;
+  bool help = false, version = false, dump_plugin = false;
+  int tls_set_on_cli = false;
   int short_name;
   const char *filename;
   char *p;
@@ -229,7 +230,7 @@ main (int argc, char *argv[])
         flag->flag = strndup (p, q-p-1);
         if (!flag->flag) goto debug_flag_perror;
         if (sscanf (q, "%d", &flag->value) != 1) goto
bad_debug_flag;
-        flag->used = 0;
+        flag->used = false;
 
         /* Add flag to the linked list. */
         flag->next = debug_flags;
@@ -242,13 +243,13 @@ main (int argc, char *argv[])
       exit (EXIT_SUCCESS);
 
     case DUMP_PLUGIN_OPTION:
-      dump_plugin = 1;
+      dump_plugin = true;
       break;
 
     case EXIT_WITH_PARENT_OPTION:
 #ifdef HAVE_EXIT_WITH_PARENT
-      exit_with_parent = 1;
-      foreground = 1;
+      exit_with_parent = true;
+      foreground = true;
       break;
 #else
       fprintf (stderr,
@@ -300,7 +301,7 @@ main (int argc, char *argv[])
         exit (EXIT_FAILURE);
       }
       run = optarg;
-      foreground = 1;
+      foreground = true;
       break;
 
     case SELINUX_LABEL_OPTION:
@@ -315,7 +316,7 @@ main (int argc, char *argv[])
       exit (EXIT_SUCCESS);
 
     case TLS_OPTION:
-      tls_set_on_cli = 1;
+      tls_set_on_cli = true;
       if (strcasecmp (optarg, "require") == 0 ||
           strcasecmp (optarg, "required") == 0 ||
           strcasecmp (optarg, "force") == 0)
@@ -336,16 +337,16 @@ main (int argc, char *argv[])
       break;
 
     case TLS_VERIFY_PEER_OPTION:
-      tls_verify_peer = 1;
+      tls_verify_peer = true;
       break;
 
     case 'e':
       exportname = optarg;
-      newstyle = 1;
+      newstyle = true;
       break;
 
     case 'f':
-      foreground = 1;
+      foreground = true;
       break;
 
     case 'g':
@@ -362,11 +363,11 @@ main (int argc, char *argv[])
       break;
 
     case 'n':
-      newstyle = 1;
+      newstyle = true;
       break;
 
     case 'o':
-      newstyle = 0;
+      newstyle = false;
       break;
 
     case 'P':
@@ -385,7 +386,7 @@ main (int argc, char *argv[])
       break;
 
     case 'r':
-      readonly = 1;
+      readonly = true;
       break;
 
     case 's':
@@ -394,7 +395,7 @@ main (int argc, char *argv[])
                  program_name);
         exit (EXIT_FAILURE);
       }
-      listen_stdin = 1;
+      listen_stdin = true;
       break;
 
     case 't':
@@ -431,15 +432,15 @@ main (int argc, char *argv[])
       break;
 
     case 'v':
-      verbose = 1;
+      verbose = true;
       break;
 
     case 'V':
-      version = 1;
+      version = true;
       break;
 
     case HELP_OPTION:
-      help = 1;
+      help = true;
       break;
 
     default:
@@ -477,7 +478,7 @@ main (int argc, char *argv[])
   }
 
   /* Oldstyle protocol + exportname not allowed. */
-  if (newstyle == 0 && exportname != NULL) {
+  if (!newstyle && exportname != NULL) {
     fprintf (stderr,
              "%s: cannot use oldstyle protocol (-o) and exportname
(-e)\n",
              program_name);
@@ -489,7 +490,7 @@ main (int argc, char *argv[])
     exportname = "";
 
   /* --tls=require and oldstyle won't work. */
-  if (tls == 2 && newstyle == 0) {
+  if (tls == 2 && !newstyle) {
     fprintf (stderr,
              "%s: cannot use oldstyle protocol (-o) and require
TLS\n",
              program_name);
@@ -727,7 +728,7 @@ open_plugin_so (size_t i, const char *name, int short_name)
 {
   struct backend *ret;
   char *filename = (char *) name;
-  int free_filename = 0;
+  bool free_filename = false;
   void *dl;
   struct nbdkit_plugin *(*plugin_init) (void);
   char *error;
@@ -739,7 +740,7 @@ open_plugin_so (size_t i, const char *name, int short_name)
       perror ("asprintf");
       exit (EXIT_FAILURE);
     }
-    free_filename = 1;
+    free_filename = true;
   }
 
   dl = dlopen (filename, RTLD_NOW|RTLD_GLOBAL);
@@ -780,7 +781,7 @@ open_filter_so (struct backend *next, size_t i,
 {
   struct backend *ret;
   char *filename = (char *) name;
-  int free_filename = 0;
+  bool free_filename = false;
   void *dl;
   struct nbdkit_filter *(*filter_init) (void);
   char *error;
@@ -792,7 +793,7 @@ open_filter_so (struct backend *next, size_t i,
       perror ("asprintf");
       exit (EXIT_FAILURE);
     }
-    free_filename = 1;
+    free_filename = true;
   }
 
   dl = dlopen (filename, RTLD_NOW|RTLD_GLOBAL);
@@ -1023,7 +1024,7 @@ fork_into_background (void)
   if (!verbose)
     dup2 (1, 2);
 
-  forked_into_background = 1;
+  forked_into_background = true;
   debug ("forked into background (new pid = %d)", getpid ());
 }
 
diff --git a/server/plugins.c b/server/plugins.c
index 52dd3a5..701c18e 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -811,7 +811,7 @@ set_debug_flags (void *dl, const char *name)
       *sym = flag->value;
 
       /* Mark this flag as used. */
-      flag->used = 1;
+      flag->used = true;
     }
   }
 }
-- 
2.19.2

On 1/1/19 12:21 PM, Richard W.M. Jones wrote:
> For mainly historical reasons we tended to use int to store boolean
> values.  However using bool is probably safer in some corner cases
> (eg. ‘v == true’ can fail badly if v is an int, but works for bool).
The problems only occur when v is an int and set to something other than
0 or 1.  But yes, in general using the bool type is worth doing.
> bool was added in C99 so let's use it.
> ---
>  server/internal.h    | 16 +++++------
>  server/connections.c | 28 +++++++++---------
>  server/crypto.c      |  4 +--
>  server/main.c        | 67 ++++++++++++++++++++++----------------------
>  server/plugins.c     |  2 +-
>  5 files changed, 59 insertions(+), 58 deletions(-)
> +++ b/server/connections.c
> @@ -78,13 +78,13 @@ struct connection {
>    uint32_t cflags;
>    uint64_t exportsize;
>    uint16_t eflags;
> -  int readonly;
> -  int can_flush;
> -  int is_rotational;
> -  int can_trim;
> -  int can_zero;
> -  int can_fua;
> -  int using_tls;
> +  bool readonly;
> +  bool can_flush;
> +  bool is_rotational;
> +  bool can_trim;
> +  bool can_zero;
> +  bool can_fua;
> +  bool using_tls;
Some of these were 'int' because they have tri-state returns from the
client (-1 for error, or 0/1 for success).  I suppose that making them
bool means that you only store into it after checking for errors, but it
does mean that we have to audit a bit more carefully that we aren't
accidentally turning -1 into true.

But I didn't spot any obvious problems, so I think the patch is good to go.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Wed, Jan 02, 2019 at 08:54:15AM -0600, Eric Blake
wrote:
> On 1/1/19 12:21 PM, Richard W.M. Jones wrote:
> > For mainly historical reasons we tended to use int to store boolean
> > values.  However using bool is probably safer in some corner cases
> > (eg. ‘v == true’ can fail badly if v is an int, but works for bool).
> 
> The problems only occur when v is an int and set to something other than
> 0 or 1.  But yes, in general using the bool type is worth doing.
> 
> > bool was added in C99 so let's use it.
> > ---
> >  server/internal.h    | 16 +++++------
> >  server/connections.c | 28 +++++++++---------
> >  server/crypto.c      |  4 +--
> >  server/main.c        | 67
++++++++++++++++++++++----------------------
> >  server/plugins.c     |  2 +-
> >  5 files changed, 59 insertions(+), 58 deletions(-)
> 
> 
> > +++ b/server/connections.c
> > @@ -78,13 +78,13 @@ struct connection {
> >    uint32_t cflags;
> >    uint64_t exportsize;
> >    uint16_t eflags;
> > -  int readonly;
> > -  int can_flush;
> > -  int is_rotational;
> > -  int can_trim;
> > -  int can_zero;
> > -  int can_fua;
> > -  int using_tls;
> > +  bool readonly;
> > +  bool can_flush;
> > +  bool is_rotational;
> > +  bool can_trim;
> > +  bool can_zero;
> > +  bool can_fua;
> > +  bool using_tls;
> 
> Some of these were 'int' because they have tri-state returns from
the
> client (-1 for error, or 0/1 for success).  I suppose that making them
> bool means that you only store into it after checking for errors, but it
> does mean that we have to audit a bit more carefully that we aren't
> accidentally turning -1 into true.
Yes in the public API, but as far as I can see in this struct they are
all really booleans, ie. all of the code is like:

  fl = backend->can_flush (backend, conn);
  if (fl == -1)
    return -1;
  if (fl) {
    eflags |= NBD_FLAG_SEND_FLUSH;
    conn->can_flush = 1;
  }

  ...

  if (!conn->flush ...
> But I didn't spot any obvious problems, so I think the patch is good to
go.
So yes it's my belief also that this change is safe.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top