Libguestfs - Sep 2019 - [nbdkit PATCH 00/10] Avoid -Wshadow

While working on can_FOO caching, at one point I got confused by
whether 'readonly' meant the global set by -r or a local passed to
.open().  A quick attempt to compile with -Wshadow found several other
confusing points; this series gets us to the point that we can add
-Wshadow to builds with --enable-gcc-warnings.

Eric Blake (10):
  server: Avoid -Wshadow warnings
  guestfs: Avoid -Wshadow warnings
  ocaml: Avoid -Wshadow warnings
  perl: Ignore -Wshadow warnings
  ssh: Avoid -Wshadow warnings
  streaming: Avoid -Wshadow warnings
  tcl: Avoid -Wshadow warnings
  cache: Avoid -Wshadow warnings
  tests: Avoid -Wshadow warnings
  maint: Enable -Wshadow during compilation

 configure.ac                         |  2 +-
 server/internal.h                    |  2 +-
 common/bitmap/bitmap.h               |  6 +++---
 server/connections.c                 |  2 +-
 server/main.c                        | 23 +++++++++++------------
 server/protocol-handshake-newstyle.c |  7 +++----
 server/protocol.c                    |  4 ++--
 plugins/guestfs/guestfs-plugin.c     | 26 +++++++++++++-------------
 plugins/ocaml/ocaml.c                |  2 +-
 plugins/perl/perl.c                  |  3 +++
 plugins/ssh/ssh.c                    |  4 ++--
 plugins/streaming/streaming.c        | 14 ++++++--------
 plugins/tcl/tcl.c                    |  1 -
 tests/test-exit-with-parent.c        | 14 ++++++--------
 tests/test-layers.c                  | 20 ++++++++++----------
 tests/web-server.c                   |  6 +++---
 16 files changed, 66 insertions(+), 70 deletions(-)

-- 
2.21.0

None of these shadowing instances appear to misbehave, but it's
confusing to tell if code intentionally meant for a tighter scope to
override a broader one.

Signed-off-by: Eric Blake <eblake at redhat.com>
---
 server/internal.h                    |  2 +-
 server/connections.c                 |  2 +-
 server/main.c                        | 23 +++++++++++------------
 server/protocol-handshake-newstyle.c |  7 +++----
 server/protocol.c                    |  4 ++--
 5 files changed, 18 insertions(+), 20 deletions(-)

diff --git a/server/internal.h b/server/internal.h
index 72333599..fdafe22c 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -93,7 +93,7 @@ extern enum log_to log_to;
 extern bool newstyle;
 extern bool no_sr;
 extern const char *port;
-extern bool readonly;
+extern bool read_only;
 extern const char *run;
 extern bool listen_stdin;
 extern const char *selinux_label;
diff --git a/server/connections.c b/server/connections.c
index 8d5c81af..b5827648 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -149,7 +149,7 @@ _handle_single_connection (int sockin, int sockout)
     goto done;

   lock_request (conn);
-  r = backend_open (backend, conn, readonly);
+  r = backend_open (backend, conn, read_only);
   unlock_request (conn);
   if (r == -1)
     goto done;
diff --git a/server/main.c b/server/main.c
index 6999818c..22cf8d33 100644
--- a/server/main.c
+++ b/server/main.c
@@ -71,7 +71,7 @@ bool newstyle = true;           /* false = -o, true = -n */
 bool no_sr;                     /* --no-sr */
 char *pidfile;                  /* -P */
 const char *port;               /* -p */
-bool readonly;                  /* -r */
+bool read_only;                 /* -r */
 const char *run;                /* --run */
 bool listen_stdin;              /* -s */
 const char *selinux_label;      /* --selinux-label */
@@ -139,9 +139,9 @@ main (int argc, char *argv[])
   int c;
   bool help = false, version = false, dump_plugin = false;
   int tls_set_on_cli = false;
-  int short_name;
+  bool short_name;
   const char *filename;
-  char *p;
+  char *p, *q;
   static struct filter_filename {
     struct filter_filename *next;
     const char *filename;
@@ -182,7 +182,6 @@ main (int argc, char *argv[])
     switch (c) {
     case 'D':
       {
-        const char *p, *q;
         struct debug_flag *flag;

         /* Debug Flag must be "NAME.FLAG=N".
@@ -377,7 +376,7 @@ main (int argc, char *argv[])
       break;

     case 'r':
-      readonly = true;
+      read_only = true;
       break;

     case 's':
@@ -526,7 +525,6 @@ main (int argc, char *argv[])
    * If so we simply execute it with the current command line.
    */
   if (short_name) {
-    size_t i;
     struct stat statbuf;
     CLEANUP_FREE char *script;

@@ -560,8 +558,9 @@ main (int argc, char *argv[])
   i = 1;
   while (filter_filenames) {
     struct filter_filename *t = filter_filenames;
-    const char *filename = t->filename;
-    int short_name = is_short_name (filename);
+
+    filename = t->filename;
+    short_name = is_short_name (filename);

     backend = open_filter_so (backend, i++, filename, short_name);

@@ -687,7 +686,7 @@ static char *
 make_random_fifo (void)
 {
   char template[] = "/tmp/nbdkitXXXXXX";
-  char *unixsocket;
+  char *sock;

   if (mkdtemp (template) == NULL) {
     perror ("mkdtemp");
@@ -705,13 +704,13 @@ make_random_fifo (void)
     return NULL;
   }

-  unixsocket = strdup (random_fifo);
-  if (unixsocket == NULL) {
+  sock = strdup (random_fifo);
+  if (sock == NULL) {
     perror ("strdup");
     return NULL;
   }

-  return unixsocket;
+  return sock;
 }

 static struct backend *
diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index 23579e5d..9ddc3198 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -80,8 +80,7 @@ send_newstyle_option_reply (struct connection *conn,

 static int
 send_newstyle_option_reply_exportname (struct connection *conn,
-                                       uint32_t option, uint32_t reply,
-                                       const char *exportname)
+                                       uint32_t option, uint32_t reply)
 {
   struct fixed_new_option_reply fixed_new_option_reply;
   size_t name_len = strlen (exportname);
@@ -320,8 +319,8 @@ negotiate_handshake_newstyle_options (struct connection
*conn)
       /* Send back the exportname. */
       debug ("newstyle negotiation: %s: advertising export
'%s'",
              name_of_nbd_opt (option), exportname);
-      if (send_newstyle_option_reply_exportname (conn, option, NBD_REP_SERVER,
-                                                 exportname) == -1)
+      if (send_newstyle_option_reply_exportname (conn, option,
+                                                 NBD_REP_SERVER) == -1)
         return -1;

       if (send_newstyle_option_reply (conn, option, NBD_REP_ACK) == -1)
diff --git a/server/protocol.c b/server/protocol.c
index 948b48ac..29cdcad3 100644
--- a/server/protocol.c
+++ b/server/protocol.c
@@ -263,12 +263,12 @@ handle_request (struct connection *conn,

   case NBD_CMD_CACHE:
     if (conn->emulate_cache) {
-      static char buf[MAX_REQUEST_SIZE]; /* data sink, never read */
+      static char zerobuf[MAX_REQUEST_SIZE]; /* data sink, never read */
       uint32_t limit;

       while (count) {
         limit = MIN (count, sizeof buf);
-        if (backend_pread (backend, conn, buf, limit, offset, flags,
+        if (backend_pread (backend, conn, zerobuf, limit, offset, flags,
                            &err) == -1)
           return err;
         count -= limit;
-- 
2.21.0

Shadowing the name of a global list in order to perform creative
reverse-order recursion is confusing.  Pick distinct names.

Signed-off-by: Eric Blake <eblake at redhat.com>
---
 plugins/guestfs/guestfs-plugin.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/plugins/guestfs/guestfs-plugin.c b/plugins/guestfs/guestfs-plugin.c
index b0061e26..c8af987c 100644
--- a/plugins/guestfs/guestfs-plugin.c
+++ b/plugins/guestfs/guestfs-plugin.c
@@ -60,7 +60,7 @@ struct drive {
   const char *value;
   const char *format;
 };
-struct drive *drives = NULL;
+struct drive *all_drives = NULL;

 /* mount options.  (NB: list stored in reverse order)  */
 struct mount {
@@ -69,7 +69,7 @@ struct mount {
   const char *dev;
   const char *mp;
 };
-struct mount *mounts = NULL;
+struct mount *all_mounts = NULL;

 static int
 plugin_guestfs_config (const char *key, const char *value)
@@ -109,8 +109,8 @@ plugin_guestfs_config (const char *key, const char *value)
     d->type = drv_disk;
     d->value = value;
     d->format = format;
-    d->next = drives;
-    drives = d;
+    d->next = all_drives;
+    all_drives = d;
   }
   else if (strcmp (key, "domain") == 0) {
     struct drive *d;
@@ -122,8 +122,8 @@ plugin_guestfs_config (const char *key, const char *value)
     }
     d->type = drv_domain;
     d->value = value;
-    d->next = drives;
-    drives = d;
+    d->next = all_drives;
+    all_drives = d;
   }
   else if (strcmp (key, "mount") == 0) {
     struct mount *m;
@@ -150,8 +150,8 @@ plugin_guestfs_config (const char *key, const char *value)
       m->dev = value;
       m->mp = "/";
     }
-    m->next = mounts;
-    mounts = m;
+    m->next = all_mounts;
+    all_mounts = m;
   }
   else {
     nbdkit_error ("unknown parameter '%s'", key);
@@ -169,7 +169,7 @@ plugin_guestfs_config_complete (void)
     return -1;
   }

-  if (drives == NULL) {
+  if (all_drives == NULL) {
     nbdkit_error ("at least one 'disk' or 'domain'
parameter is required");
     return -1;
   }
@@ -193,12 +193,12 @@ plugin_guestfs_unload (void)
   struct drive *d, *d_next;
   struct mount *m, *m_next;

-  for (d = drives; d != NULL; d = d_next) {
+  for (d = all_drives; d != NULL; d = d_next) {
     d_next = d->next;
     free (d);
   }

-  for (m = mounts; m != NULL; m = m_next) {
+  for (m = all_mounts; m != NULL; m = m_next) {
     m_next = m->next;
     free (m);
   }
@@ -257,7 +257,7 @@ plugin_guestfs_open (int readonly)
   if (set_up_logging (h->g) == -1)
     goto err2;

-  if (add_disks (h->g, readonly, drives) == -1)
+  if (add_disks (h->g, readonly, all_drives) == -1)
     goto err2;

   if (guestfs_launch (h->g) == -1) {
@@ -265,7 +265,7 @@ plugin_guestfs_open (int readonly)
     goto err2;
   }

-  if (mount_filesystems (h->g, readonly, mounts) == -1)
+  if (mount_filesystems (h->g, readonly, all_mounts) == -1)
     goto err2;

   /* Exported thing. */
-- 
2.21.0

Reusing 'offset' for a different purpose in the second half of the
function is safe enough without needing a shadow or a separate
variable name.

Signed-off-by: Eric Blake <eblake at redhat.com>
---
 plugins/ocaml/ocaml.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/plugins/ocaml/ocaml.c b/plugins/ocaml/ocaml.c
index c472281f..f2b99276 100644
--- a/plugins/ocaml/ocaml.c
+++ b/plugins/ocaml/ocaml.c
@@ -622,7 +622,7 @@ extents_wrapper (void *h, uint32_t count, uint64_t offset,
uint32_t flags,

   /* Convert extents list into calls to nbdkit_add_extent. */
   while (rv != Val_int (0)) {
-    uint64_t offset, length;
+    uint64_t length;
     uint32_t type = 0;

     v = Field (rv, 0);          /* extent struct */
-- 
2.21.0

perl.h requires that the user declare a static PerlInterpreter * named
'my_perl', but also shadows this declaration in functions declared
with the XS() macro.  As there's no way to avoid this issue, turn off
this warning here so we can fix -Wshadow warnings elsewhere.

Signed-off-by: Eric Blake <eblake at redhat.com>
---
 plugins/perl/perl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/plugins/perl/perl.c b/plugins/perl/perl.c
index 59b26788..b0886bb6 100644
--- a/plugins/perl/perl.c
+++ b/plugins/perl/perl.c
@@ -50,6 +50,9 @@

 #include "cleanup.h"

+/* Use of perl.h insists on shadowing my_perl during XS(). */
+#pragma GCC diagnostic ignored "-Wshadow"
+
 static PerlInterpreter *my_perl;
 static const char *script;

-- 
2.21.0

This one was harmless, but easy enough to avoid.

Signed-off-by: Eric Blake <eblake at redhat.com>
---
 plugins/ssh/ssh.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/plugins/ssh/ssh.c b/plugins/ssh/ssh.c
index d3fa2931..ee42ee1f 100644
--- a/plugins/ssh/ssh.c
+++ b/plugins/ssh/ssh.c
@@ -281,11 +281,11 @@ authenticate_pubkey (ssh_session session)
 }

 static int
-authenticate_password (ssh_session session, const char *password)
+authenticate_password (ssh_session session, const char *pass)
 {
   int rc;

-  rc = ssh_userauth_password (session, NULL, password);
+  rc = ssh_userauth_password (session, NULL, pass);
   if (rc == SSH_AUTH_ERROR)
     nbdkit_debug ("password authentication failed: %s",
                   ssh_get_error (session));
-- 
2.21.0

Renaming variables is easy enough here. While at it, use a static
buffer rather than wasting time on memset().

Signed-off-by: Eric Blake <eblake at redhat.com>
---
 plugins/streaming/streaming.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/plugins/streaming/streaming.c b/plugins/streaming/streaming.c
index 4ab73b8b..b2359540 100644
--- a/plugins/streaming/streaming.c
+++ b/plugins/streaming/streaming.c
@@ -185,21 +185,19 @@ streaming_pwrite (void *handle, const void *buf,

   /* Need to write some zeroes. */
   if (offset > highestwrite) {
-    int64_t size = offset - highestwrite;
-    char buf[4096];
+    int64_t remaining = offset - highestwrite;
+    static char zerobuf[4096];

-    memset (buf, 0, sizeof buf);
-
-    while (size > 0) {
-      n = size > sizeof buf ? sizeof buf : size;
-      r = write (fd, buf, n);
+    while (remaining > 0) {
+      n = remaining > sizeof zerobuf ? sizeof zerobuf : remaining;
+      r = write (fd, zerobuf, n);
       if (r == -1) {
         nbdkit_error ("write: %m");
         errorstate = 1;
         return -1;
       }
       highestwrite += r;
-      size -= r;
+      remaining -= r;
     }
   }

-- 
2.21.0

This one was harmless, but easy to avoid.

Signed-off-by: Eric Blake <eblake at redhat.com>
---
 plugins/tcl/tcl.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/plugins/tcl/tcl.c b/plugins/tcl/tcl.c
index 042ac4a1..4b3c50a7 100644
--- a/plugins/tcl/tcl.c
+++ b/plugins/tcl/tcl.c
@@ -156,7 +156,6 @@ tcl_config (const char *key, const char *value)
     }
   }
   else if (proc_defined ("config")) {
-    int r;
     Tcl_Obj *cmd;

     cmd = Tcl_NewObj ();
-- 
2.21.0

The cache filter declares 'blksize' as a global; the common bitmap.h
header shadows this.  It's easy enough to rename to avoid the warning.

Signed-off-by: Eric Blake <eblake at redhat.com>
---
 common/bitmap/bitmap.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/common/bitmap/bitmap.h b/common/bitmap/bitmap.h
index d80cdfe6..c18ae93a 100644
--- a/common/bitmap/bitmap.h
+++ b/common/bitmap/bitmap.h
@@ -65,10 +65,10 @@ struct bitmap {
 };

 static inline void __attribute__((__nonnull__ (1)))
-bitmap_init (struct bitmap *bm, unsigned blksize, unsigned bpb)
+bitmap_init (struct bitmap *bm, unsigned blocksize, unsigned bpb)
 {
-  assert (is_power_of_2 (blksize));
-  bm->blksize = blksize;
+  assert (is_power_of_2 (blocksize));
+  bm->blksize = blocksize;

   /* bpb can be 1, 2, 4 or 8 only. */
   bm->bpb = bpb;
-- 
2.21.0

In test-layers, declaring a variable 'log' technically conflicts with
POSIX's rule that <math.h> reserves log(), even if we don't use
math.h.  Picking a different name shuts up the compiler.

In test-exit-with-parent, the use of 'pid' for the grandchild process
collides with the (unused) global 'pid' from test.h; but when you
realize that the use of 'nbdpid' in the child process does not collide
with its use in the monitor process, we don't need an extra
declaration.

In web-server, the only caller of xpread passed global 'fd' to a
shadowed name; just rely on the global instead.

Signed-off-by: Eric Blake <eblake at redhat.com>
---
 tests/test-exit-with-parent.c | 14 ++++++--------
 tests/test-layers.c           | 20 ++++++++++----------
 tests/web-server.c            |  6 +++---
 3 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/tests/test-exit-with-parent.c b/tests/test-exit-with-parent.c
index 2bb5d8f5..9c98db44 100644
--- a/tests/test-exit-with-parent.c
+++ b/tests/test-exit-with-parent.c
@@ -92,10 +92,8 @@ run_test (void)
    */
   cpid = fork ();
   if (cpid == 0) {              /* child process */
-    pid_t pid;
-
-    pid = fork ();
-    if (pid == 0) {             /* exec nbdkit process */
+    nbdpid = fork ();
+    if (nbdpid == 0) {             /* exec nbdkit process */
       const char *argv[] = {
         "nbdkit", "-U", "-", "-P",
pidpath, "-f", "--exit-with-parent",
         "example1",
@@ -109,20 +107,20 @@ run_test (void)

     /* Wait for the pidfile to turn up, which indicates that nbdkit has
      * started up successfully and is ready to serve requests.  However
-     * if 'pid' exits in this time it indicates a failure to start up.
+     * if 'nbdpid' exits in this time it indicates a failure to start
up.
      * Also there is a timeout in case nbdkit hangs.
      */
     for (i = 0; i < NBDKIT_START_TIMEOUT; ++i) {
-      if (waitpid (pid, NULL, WNOHANG) == pid)
+      if (waitpid (nbdpid, NULL, WNOHANG) == nbdpid)
         goto early_exit;

-      if (kill (pid, 0) == -1) {
+      if (kill (nbdpid, 0) == -1) {
         if (errno == ESRCH) {
         early_exit:
           fprintf (stderr,
                    "%s FAILED: nbdkit exited before starting to serve
files\n",
                    program_name);
-          pid = 0;
+          nbdpid = 0;
           exit (EXIT_FAILURE);
         }
         perror ("kill");
diff --git a/tests/test-layers.c b/tests/test-layers.c
index a7184d72..4d0167eb 100644
--- a/tests/test-layers.c
+++ b/tests/test-layers.c
@@ -615,7 +615,7 @@ main (int argc, char *argv[])
 }

 /* The log from nbdkit is captured in a separate thread. */
-static char *log = NULL;
+static char *log_buf = NULL;
 static size_t log_len = 0;
 static pthread_mutex_t log_lock = PTHREAD_MUTEX_INITIALIZER;

@@ -631,15 +631,15 @@ start_log_capture (void *arg)
       ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&log_lock);
       if (allocated <= log_len) {
         allocated += 4096;
-        log = realloc (log, allocated);
-        if (log == NULL) {
+        log_buf = realloc (log_buf, allocated);
+        if (log_buf == NULL) {
           perror ("log: realloc");
           exit (EXIT_FAILURE);
         }
       }
     }

-    r = read (fd, &log[log_len], allocated-log_len);
+    r = read (fd, &log_buf[log_len], allocated-log_len);
     if (r == -1) {
       perror ("log: read");
       exit (EXIT_FAILURE);
@@ -648,7 +648,7 @@ start_log_capture (void *arg)
       break;

     /* Dump the log as we receive it to stderr, for debugging. */
-    if (write (2, &log[log_len], r) == -1)
+    if (write (2, &log_buf[log_len], r) == -1)
       perror ("log: write");

     ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&log_lock);
@@ -679,7 +679,7 @@ static void
 log_verify_seen (const char *msg)
 {
   ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&log_lock);
-  if (memmem (log, log_len, msg, strlen (msg)) == NULL)
+  if (memmem (log_buf, log_len, msg, strlen (msg)) == NULL)
     no_message_error (msg);
 }

@@ -703,13 +703,13 @@ log_verify_seen_in_order (const char *msg, ...)

   ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&log_lock);

-  prev = memmem (log, log_len, msg, strlen (msg));
+  prev = memmem (log_buf, log_len, msg, strlen (msg));
   if (prev == NULL) no_message_error (msg);
   prev_msg = msg;

   va_start (args, msg);
   while ((curr_msg = va_arg (args, char *)) != NULL) {
-    curr = memmem (log, log_len, curr_msg, strlen (curr_msg));
+    curr = memmem (log_buf, log_len, curr_msg, strlen (curr_msg));
     if (curr == NULL) no_message_error (curr_msg);
     if (prev > curr) messages_out_of_order (prev_msg, curr_msg);
     prev_msg = curr_msg;
@@ -722,7 +722,7 @@ static void
 log_free (void)
 {
   ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&log_lock);
-  free (log);
-  log = NULL;
+  free (log_buf);
+  log_buf = NULL;
   log_len = 0;
 }
diff --git a/tests/web-server.c b/tests/web-server.c
index 83f90881..f27ee70d 100644
--- a/tests/web-server.c
+++ b/tests/web-server.c
@@ -66,7 +66,7 @@ static void *start_web_server (void *arg);
 static void handle_requests (int s);
 static void handle_request (int s, bool headers_only);
 static void xwrite (int s, const char *buf, size_t len);
-static void xpread (int fd, char *buf, size_t count, off_t offset);
+static void xpread (char *buf, size_t count, off_t offset);

 static void
 cleanup (void)
@@ -281,7 +281,7 @@ handle_request (int s, bool headers_only)
     exit (EXIT_FAILURE);
   }

-  xpread (fd, data, length, offset);
+  xpread (data, length, offset);
   xwrite (s, data, length);

   free (data);
@@ -304,7 +304,7 @@ xwrite (int s, const char *buf, size_t len)
 }

 static void
-xpread (int fd, char *buf, size_t count, off_t offset)
+xpread (char *buf, size_t count, off_t offset)
 {
   ssize_t r;

-- 
2.21.0

Now that previous patches have cleaned up shadowing warnings, it's
worth preventing any further relapses.

Signed-off-by: Eric Blake <eblake at redhat.com>
---
 configure.ac | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 4c92e2b1..5842c6f9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -90,7 +90,7 @@ AC_ARG_ENABLE([gcc-warnings],
       [gcc_warnings=no]
 )
 if test "x$gcc_warnings" = "xyes"; then
-    WARNINGS_CFLAGS="-Wall -Werror"
+    WARNINGS_CFLAGS="-Wall -Wshadow -Werror"
     AC_SUBST([WARNINGS_CFLAGS])
 fi

-- 
2.21.0

ACK series, thanks.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW