Libguestfs - Sep 2019 - [PATCH nbdkit] server: Remove useless thread local sockaddr.

When accepting a connection on a TCP or Unix domain socket we recorded
the peer address in both the thread_data struct and thread-local
storage.  But for no reason because it was never used anywhere.  Since
we were only allocating a ‘struct sockaddr’ (rather than a ‘struct
sockaddr_storage’) it's likely that some peer addresses would have
been truncated.

Remove all this code, it had no effect.

Plugins that want to get the peer address can use nbdkit_peer_name()
which was added in commit 03a2cc3d766e and doesn't suffer from the
above truncation problem.

(I considered an alternative where we use the saved address to answer
nbdkit_peer_name but since that call will in general be used very
rarely it doesn't make sense to do the extra work for all callers.)
---
 server/internal.h    |  4 ----
 server/sockets.c     | 12 ++----------
 server/threadlocal.c | 19 -------------------
 3 files changed, 2 insertions(+), 33 deletions(-)

diff --git a/server/internal.h b/server/internal.h
index 1f72b01..c31bb34 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -431,10 +431,6 @@ extern void threadlocal_set_name (const char *name)
 extern const char *threadlocal_get_name (void);
 extern void threadlocal_set_instance_num (size_t instance_num);
 extern size_t threadlocal_get_instance_num (void);
-extern void threadlocal_set_sockaddr (const struct sockaddr *addr,
-                                      socklen_t addrlen)
-  __attribute__((__nonnull__ (1)));
-/*extern void threadlocal_get_sockaddr ();*/
 extern void threadlocal_set_error (int err);
 extern int threadlocal_get_error (void);
 extern void *threadlocal_buffer (size_t size);
diff --git a/server/sockets.c b/server/sockets.c
index dfaa3ea..3514c69 100644
--- a/server/sockets.c
+++ b/server/sockets.c
@@ -260,8 +260,6 @@ free_listening_sockets (int *socks, size_t nr_socks)
 struct thread_data {
   int sock;
   size_t instance_num;
-  struct sockaddr addr;
-  socklen_t addrlen;
 };
 
 static void *
@@ -274,7 +272,6 @@ start_thread (void *datav)
   /* Set thread-local data. */
   threadlocal_new_server_thread ();
   threadlocal_set_instance_num (data->instance_num);
-  threadlocal_set_sockaddr (&data->addr, data->addrlen);
 
   handle_single_connection (data->sock, data->sock);
 
@@ -299,12 +296,9 @@ accept_connection (int listen_sock)
   }
 
   thread_data->instance_num = instance_num++;
-  thread_data->addrlen = sizeof thread_data->addr;
  again:
 #ifdef HAVE_ACCEPT4
-  thread_data->sock = accept4 (listen_sock,
-                               &thread_data->addr,
&thread_data->addrlen,
-                               SOCK_CLOEXEC);
+  thread_data->sock = accept4 (listen_sock, NULL, NULL, SOCK_CLOEXEC);
 #else
   /* If we were fully parallel, then this function could be accepting
    * connections in one thread while another thread could be in a
@@ -316,9 +310,7 @@ accept_connection (int listen_sock)
   assert (backend->thread_model (backend) <          
NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS);
   lock_request (NULL);
-  thread_data->sock = set_cloexec (accept (listen_sock,
-                                           &thread_data->addr,
-                                           &thread_data->addrlen));
+  thread_data->sock = set_cloexec (accept (listen_sock, NULL, NULL));
   unlock_request (NULL);
 #endif
   if (thread_data->sock == -1) {
diff --git a/server/threadlocal.c b/server/threadlocal.c
index a796fce..71cc2d2 100644
--- a/server/threadlocal.c
+++ b/server/threadlocal.c
@@ -55,8 +55,6 @@
 struct threadlocal {
   char *name;                   /* Can be NULL. */
   size_t instance_num;          /* Can be 0. */
-  struct sockaddr *addr;
-  socklen_t addrlen;
   int err;
   void *buffer;
   size_t buffer_size;
@@ -71,7 +69,6 @@ free_threadlocal (void *threadlocalv)
   struct threadlocal *threadlocal = threadlocalv;
 
   free (threadlocal->name);
-  free (threadlocal->addr);
   free (threadlocal->buffer);
   free (threadlocal);
 }
@@ -133,22 +130,6 @@ threadlocal_set_instance_num (size_t instance_num)
     threadlocal->instance_num = instance_num;
 }
 
-void
-threadlocal_set_sockaddr (const struct sockaddr *addr, socklen_t addrlen)
-{
-  struct threadlocal *threadlocal = pthread_getspecific (threadlocal_key);
-
-  if (threadlocal) {
-    free (threadlocal->addr);
-    threadlocal->addr = calloc (1, addrlen);
-    if (threadlocal->addr == NULL) {
-      perror ("calloc");
-      exit (EXIT_FAILURE);
-    }
-    memcpy (threadlocal->addr, addr, addrlen);
-  }
-}
-
 const char *
 threadlocal_get_name (void)
 {
-- 
2.23.0

On 9/18/19 4:08 AM, Richard W.M. Jones wrote:
> When accepting a connection on a TCP or Unix domain socket we recorded
> the peer address in both the thread_data struct and thread-local
> storage.  But for no reason because it was never used anywhere.  Since
> we were only allocating a ‘struct sockaddr’ (rather than a ‘struct
> sockaddr_storage’) it's likely that some peer addresses would have
> been truncated.
> 
> Remove all this code, it had no effect.
Indeed.  Looks good.
> 
> Plugins that want to get the peer address can use nbdkit_peer_name()
> which was added in commit 03a2cc3d766e and doesn't suffer from the
> above truncation problem.
> 
> (I considered an alternative where we use the saved address to answer
> nbdkit_peer_name but since that call will in general be used very
> rarely it doesn't make sense to do the extra work for all callers.)
> ---
>  server/internal.h    |  4 ----
>  server/sockets.c     | 12 ++----------
>  server/threadlocal.c | 19 -------------------
>  3 files changed, 2 insertions(+), 33 deletions(-)
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org