Libguestfs - Sep 2019 - [PATCH nbdkit] server: Remove tricksy initialization of struct b_conn_handle.

b_conn_handle fields exportsize and can_* have a special meaning when
they are -1.  It means that the value of the field has not been
computed and cached yet.

The struct was being initialized using memset (_, -1, _) which does
indeed have the effect of setting all the fields to -1, although it's
somewhat non-obvious to say the least.

This commit replaces it with ordinary field initialization, also
setting h->handle to NULL.  By keeping the initialization function
next to the struct definition, hopefully they will be updated in
tandem in future.

GCC 9.2.1 actually optimizes this back into the memset equivalent
using inline AVX instructions, so good job there!

Simple refactoring, should have no effect on how the code works.

See commit d60d0f4248610fc1d116dc9f249526d20913c9a3.
---
 server/connections.c |  3 +--
 server/internal.h    | 17 +++++++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/server/connections.c b/server/connections.c
index 819f7b8..ec28815 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -269,9 +269,8 @@ new_connection (int sockin, int sockout, int nworkers)
     goto error;
   }
   conn->nr_handles = backend->i + 1;
-  memset (conn->handles, -1, conn->nr_handles * sizeof
*conn->handles);
   for_each_backend (b)
-    conn->handles[b->i].handle = NULL;
+    reset_b_conn_handle (&conn->handles[b->i]);
 
   conn->status = 1;
   conn->nworkers = nworkers;
diff --git a/server/internal.h b/server/internal.h
index c31bb34..604dd89 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -168,6 +168,23 @@ struct b_conn_handle {
   int can_cache;
 };
 
+static inline void
+reset_b_conn_handle (struct b_conn_handle *h)
+{
+  h->handle = NULL;
+  h->exportsize = -1;
+  h->can_write = -1;
+  h->can_flush = -1;
+  h->is_rotational = -1;
+  h->can_trim = -1;
+  h->can_zero = -1;
+  h->can_fast_zero = -1;
+  h->can_fua = -1;
+  h->can_multi_conn = -1;
+  h->can_extents = -1;
+  h->can_cache = -1;
+}
+
 struct connection {
   pthread_mutex_t request_lock;
   pthread_mutex_t read_lock;
-- 
2.23.0

On 9/19/19 7:56 AM, Richard W.M. Jones wrote:
> b_conn_handle fields exportsize and can_* have a special meaning when
> they are -1.  It means that the value of the field has not been
> computed and cached yet.
> 
> The struct was being initialized using memset (_, -1, _) which does
> indeed have the effect of setting all the fields to -1, although it's
> somewhat non-obvious to say the least.
> 
> This commit replaces it with ordinary field initialization, also
> setting h->handle to NULL.  By keeping the initialization function
> next to the struct definition, hopefully they will be updated in
> tandem in future.
> 
> GCC 9.2.1 actually optimizes this back into the memset equivalent
> using inline AVX instructions, so good job there!
> 
> Simple refactoring, should have no effect on how the code works.
> 
> See commit d60d0f4248610fc1d116dc9f249526d20913c9a3.
ACK. And affects 3 and 4 of my pending spec cleanup patches, but yours
should go in first (I'll rebase mine, because I'm working on refactoring
those for less code duplication anyway).

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