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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

This one was harmless, but easy enough to avoid. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- 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

This one was harmless, but easy to avoid. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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