Libguestfs - Feb 2023 - [libnbd PATCH v3 05/29] vector: (mostly) factor out DEFINE_VECTOR_EMPTY

The "name##_iter" function is used 11 times in libnbd; in all those
cases,
"name" is "string_vector", and the "f" callback is
"free":

  string_vector_iter (..., (void *) free);

Casting "free" to (void*) is ugly. (Well-defined by POSIX, but still.)

Furthermore, in all 11 cases, the freeing of the vector's strings is
immediately followed by the release of the vector's array-of-pointers too.
(This additional step is not expressed consistently across libnbd: it's
sometimes spelled as free(vec.ptr), sometimes as
string_vector_reset(&vec).)

Remove the generic "name##_iter" function definition, and introduce
"name##_empty", which performs both steps at the same time. Convert
the
call sites. (Note that the converted code performs more cleanup steps in
some cases than strictly necessary, but the extra work is harmless, and
arguably beneficial for clarity / consistency.)

Expose the "name##_empty" function definition with a new, separate
macro:
DEFINE_VECTOR_EMPTY(). The existent DEFINE_VECTOR_TYPE() macro permits
such element types that are not pointers, or are pointers to const- and/or
volatile-qualified objects. Whereas "name##_empty" requires that the
elements be pointers to dynamically allocated, non-const, non-volatile
objects.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 common/utils/string-vector.h                 |  1 +
 common/utils/vector.h                        | 27 +++++++++++++-------
 generator/states-connect-socket-activation.c |  9 +++----
 lib/handle.c                                 | 12 +++------
 lib/utils.c                                  |  6 ++---
 common/utils/test-vector.c                   |  3 +--
 info/show.c                                  |  3 +--
 7 files changed, 30 insertions(+), 31 deletions(-)

diff --git a/common/utils/string-vector.h b/common/utils/string-vector.h
index aa33fd48ceb5..78d0b4f9bf10 100644
--- a/common/utils/string-vector.h
+++ b/common/utils/string-vector.h
@@ -38,5 +38,6 @@
 #include "vector.h"
 
 DEFINE_VECTOR_TYPE (string_vector, char *);
+DEFINE_VECTOR_EMPTY (string_vector);
 
 #endif /* STRING_VECTOR_H */
diff --git a/common/utils/vector.h b/common/utils/vector.h
index fb2482c853ff..14bf5b0ddbc0 100644
--- a/common/utils/vector.h
+++ b/common/utils/vector.h
@@ -150,15 +150,6 @@
     v->len = v->cap = 0;                                                \
   }                                                                     \
                                                                         \
-  /* Iterate over the vector, calling f() on each element. */           \
-  static inline void __attribute__ ((__unused__))                       \
-  name##_iter (name *v, void (*f) (type elem))                          \
-  {                                                                     \
-    size_t i;                                                           \
-    for (i = 0; i < v->len; ++i)                                        \
-      f (v->ptr[i]);                                                    \
-  }                                                                     \
-                                                                        \
   /* Sort the elements of the vector. */                                \
   static inline void __attribute__ ((__unused__))                       \
   name##_sort (name *v,                                                 \
@@ -180,6 +171,24 @@
 
 #define empty_vector { .ptr = NULL, .len = 0, .cap = 0 }
 
+/* This macro should only be used if:
+ * - the vector contains pointers, and
+ * - the pointed-to objects are:
+ *   - neither const- nor volatile-qualified, and
+ *   - allocated with malloc() or equivalent.
+ */
+#define DEFINE_VECTOR_EMPTY(name)                                      \
+  /* Call free() on each element of the vector, then reset the vector. \
+   */                                                                  \
+  static inline void __attribute__ ((__unused__))                      \
+  name##_empty (name *v)                                               \
+  {                                                                    \
+    size_t i;                                                          \
+    for (i = 0; i < v->len; ++i)                                       \
+      free (v->ptr[i]);                                                \
+    name##_reset (v);                                                  \
+  }
+
 struct generic_vector {
   void *ptr;
   size_t len;
diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index 7138e7638e30..3b621b8be44f 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -91,8 +91,7 @@ prepare_socket_activation_environment (string_vector *env)
 
  err:
   set_error (errno, "malloc");
-  string_vector_iter (env, (void *) free);
-  free (env->ptr);
+  string_vector_empty (env);
   return -1;
 }
 
@@ -166,8 +165,7 @@  CONNECT_SA.START:
     SET_NEXT_STATE (%.DEAD);
     set_error (errno, "fork");
     close (s);
-    string_vector_iter (&env, (void *) free);
-    free (env.ptr);
+    string_vector_empty (&env);
     return 0;
   }
   if (pid == 0) {         /* child - run command */
@@ -210,8 +208,7 @@  CONNECT_SA.START:
 
   /* Parent. */
   close (s);
-  string_vector_iter (&env, (void *) free);
-  free (env.ptr);
+  string_vector_empty (&env);
   h->pid = pid;
 
   h->connaddrlen = sizeof addr;
diff --git a/lib/handle.c b/lib/handle.c
index dfd8c2e5cdb9..fb92f16e6c91 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -128,8 +128,7 @@ nbd_close (struct nbd_handle *h)
   /* Free user callbacks first. */
   nbd_unlocked_clear_debug_callback (h);
 
-  string_vector_iter (&h->querylist, (void *) free);
-  free (h->querylist.ptr);
+  string_vector_empty (&h->querylist);
   free (h->bs_entries);
   nbd_internal_reset_size_and_flags (h);
   for (i = 0; i < h->meta_contexts.len; ++i)
@@ -139,8 +138,7 @@ nbd_close (struct nbd_handle *h)
   free_cmd_list (h->cmds_to_issue);
   free_cmd_list (h->cmds_in_flight);
   free_cmd_list (h->cmds_done);
-  string_vector_iter (&h->argv, (void *) free);
-  free (h->argv.ptr);
+  string_vector_empty (&h->argv);
   if (h->sact_sockpath) {
     if (h->pid > 0)
       kill (h->pid, SIGTERM);
@@ -164,8 +162,7 @@ nbd_close (struct nbd_handle *h)
   free (h->tls_certificates);
   free (h->tls_username);
   free (h->tls_psk_file);
-  string_vector_iter (&h->request_meta_contexts, (void *) free);
-  free (h->request_meta_contexts.ptr);
+  string_vector_empty (&h->request_meta_contexts);
   free (h->hname);
   pthread_mutex_destroy (&h->lock);
   free (h);
@@ -379,8 +376,7 @@ nbd_unlocked_get_meta_context (struct nbd_handle *h, size_t
i)
 int
 nbd_unlocked_clear_meta_contexts (struct nbd_handle *h)
 {
-  string_vector_iter (&h->request_meta_contexts, (void *) free);
-  string_vector_reset (&h->request_meta_contexts);
+  string_vector_empty (&h->request_meta_contexts);
   return 0;
 }
 
diff --git a/lib/utils.c b/lib/utils.c
index c229ebfc6106..bba4b3846e77 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -93,8 +93,7 @@ nbd_internal_set_argv (struct nbd_handle *h, char **argv)
     return -1;
   }
 
-  string_vector_iter (&h->argv, (void *) free);
-  string_vector_reset (&h->argv);
+  string_vector_empty (&h->argv);
 
   if (nbd_internal_copy_string_list (&h->argv, argv) == -1) {
     set_error (errno, "realloc");
@@ -110,8 +109,7 @@ nbd_internal_set_argv (struct nbd_handle *h, char **argv)
 int
 nbd_internal_set_querylist (struct nbd_handle *h, char **queries)
 {
-  string_vector_iter (&h->querylist, (void *) free);
-  string_vector_reset (&h->querylist);
+  string_vector_empty (&h->querylist);
 
   if (queries) {
     if (nbd_internal_copy_string_list (&h->querylist, queries) == -1) {
diff --git a/common/utils/test-vector.c b/common/utils/test-vector.c
index 55c8ebeb8a1e..05ac5cec3dba 100644
--- a/common/utils/test-vector.c
+++ b/common/utils/test-vector.c
@@ -151,8 +151,7 @@ test_string_vector (void)
     printf ("%s\n", v.ptr[i]);
   assert (i == 10);
 
-  string_vector_iter (&v, (void*)free);
-  string_vector_reset (&v);
+  string_vector_empty (&v);
 }
 
 static void
diff --git a/info/show.c b/info/show.c
index 4bf596715cf9..e6c9b9bf1243 100644
--- a/info/show.c
+++ b/info/show.c
@@ -275,8 +275,7 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
       fprintf (fp, "\t},\n");
   }
 
-  string_vector_iter (&contexts, (void *) free);
-  free (contexts.ptr);
+  string_vector_empty (&contexts);
   free (content);
   free (export_name);
   free (export_desc);

On Wed, Feb 15, 2023 at 03:11:34PM +0100, Laszlo Ersek
wrote:
> The "name##_iter" function is used 11 times in libnbd; in all
those cases,
> "name" is "string_vector", and the "f"
callback is "free":
> 
>   string_vector_iter (..., (void *) free);
> 
> Casting "free" to (void*) is ugly. (Well-defined by POSIX, but
still.)
> 
> Furthermore, in all 11 cases, the freeing of the vector's strings is
> immediately followed by the release of the vector's array-of-pointers
too.
> (This additional step is not expressed consistently across libnbd: it's
> sometimes spelled as free(vec.ptr), sometimes as
> string_vector_reset(&vec).)
> 
> Remove the generic "name##_iter" function definition, and
introduce
> "name##_empty", which performs both steps at the same time.
Convert the
> call sites. (Note that the converted code performs more cleanup steps in
> some cases than strictly necessary, but the extra work is harmless, and
> arguably beneficial for clarity / consistency.)
> 
> Expose the "name##_empty" function definition with a new,
separate macro:
> DEFINE_VECTOR_EMPTY(). The existent DEFINE_VECTOR_TYPE() macro permits
> such element types that are not pointers, or are pointers to const- and/or
> volatile-qualified objects. Whereas "name##_empty" requires that
the
> elements be pointers to dynamically allocated, non-const, non-volatile
> objects.
> 
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> ---
>  common/utils/string-vector.h                 |  1 +
>  common/utils/vector.h                        | 27 +++++++++++++-------
>  generator/states-connect-socket-activation.c |  9 +++----
>  lib/handle.c                                 | 12 +++------
>  lib/utils.c                                  |  6 ++---
>  common/utils/test-vector.c                   |  3 +--
>  info/show.c                                  |  3 +--
>  7 files changed, 30 insertions(+), 31 deletions(-)
> 
> diff --git a/common/utils/string-vector.h b/common/utils/string-vector.h
> index aa33fd48ceb5..78d0b4f9bf10 100644
> --- a/common/utils/string-vector.h
> +++ b/common/utils/string-vector.h
> @@ -38,5 +38,6 @@
>  #include "vector.h"
>  
>  DEFINE_VECTOR_TYPE (string_vector, char *);
> +DEFINE_VECTOR_EMPTY (string_vector);
>  
>  #endif /* STRING_VECTOR_H */
> diff --git a/common/utils/vector.h b/common/utils/vector.h
> index fb2482c853ff..14bf5b0ddbc0 100644
> --- a/common/utils/vector.h
> +++ b/common/utils/vector.h
> @@ -150,15 +150,6 @@
>      v->len = v->cap = 0;                                            
\
>    }                                                                     \
>                                                                          \
> -  /* Iterate over the vector, calling f() on each element. */           \
> -  static inline void __attribute__ ((__unused__))                       \
> -  name##_iter (name *v, void (*f) (type elem))                          \
> -  {                                                                     \
> -    size_t i;                                                           \
> -    for (i = 0; i < v->len; ++i)                                    
\
> -      f (v->ptr[i]);                                                   
\
> -  }                                                                     \
> -                                                                        \
What's the reason for removing iter?  It's used by nbdkit in ways that
don't involve free().  I'd prefer if we leave this definition in.
>    /* Sort the elements of the vector. */                                \
>    static inline void __attribute__ ((__unused__))                       \
>    name##_sort (name *v,                                                 \
> @@ -180,6 +171,24 @@
>  
>  #define empty_vector { .ptr = NULL, .len = 0, .cap = 0 }
>  
> +/* This macro should only be used if:
> + * - the vector contains pointers, and
> + * - the pointed-to objects are:
> + *   - neither const- nor volatile-qualified, and
> + *   - allocated with malloc() or equivalent.
> + */
> +#define DEFINE_VECTOR_EMPTY(name)                                      \
I'm going to be that guy ...

Can we call it ADD_VECTOR_EMPTY_METHOD or similar/better?

Otherwise it seems like DEFINE_VECTOR_EMPTY is an alternative to
DEFINE_VECTOR_TYPE (which I thought was the case initially), rather
than something you have to call in addition to DEFINE_VECTOR_TYPE.

The rest seems fine.

Rich.
> +  /* Call free() on each element of the vector, then reset the vector. \
> +   */                                                                  \
> +  static inline void __attribute__ ((__unused__))                      \
> +  name##_empty (name *v)                                               \
> +  {                                                                    \
> +    size_t i;                                                          \
> +    for (i = 0; i < v->len; ++i)                                    
\
> +      free (v->ptr[i]);                                               
\
> +    name##_reset (v);                                                  \
> +  }
> +
>  struct generic_vector {
>    void *ptr;
>    size_t len;
> diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
> index 7138e7638e30..3b621b8be44f 100644
> --- a/generator/states-connect-socket-activation.c
> +++ b/generator/states-connect-socket-activation.c
> @@ -91,8 +91,7 @@ prepare_socket_activation_environment (string_vector
*env)
>  
>   err:
>    set_error (errno, "malloc");
> -  string_vector_iter (env, (void *) free);
> -  free (env->ptr);
> +  string_vector_empty (env);
>    return -1;
>  }
>  
> @@ -166,8 +165,7 @@  CONNECT_SA.START:
>      SET_NEXT_STATE (%.DEAD);
>      set_error (errno, "fork");
>      close (s);
> -    string_vector_iter (&env, (void *) free);
> -    free (env.ptr);
> +    string_vector_empty (&env);
>      return 0;
>    }
>    if (pid == 0) {         /* child - run command */
> @@ -210,8 +208,7 @@  CONNECT_SA.START:
>  
>    /* Parent. */
>    close (s);
> -  string_vector_iter (&env, (void *) free);
> -  free (env.ptr);
> +  string_vector_empty (&env);
>    h->pid = pid;
>  
>    h->connaddrlen = sizeof addr;
> diff --git a/lib/handle.c b/lib/handle.c
> index dfd8c2e5cdb9..fb92f16e6c91 100644
> --- a/lib/handle.c
> +++ b/lib/handle.c
> @@ -128,8 +128,7 @@ nbd_close (struct nbd_handle *h)
>    /* Free user callbacks first. */
>    nbd_unlocked_clear_debug_callback (h);
>  
> -  string_vector_iter (&h->querylist, (void *) free);
> -  free (h->querylist.ptr);
> +  string_vector_empty (&h->querylist);
>    free (h->bs_entries);
>    nbd_internal_reset_size_and_flags (h);
>    for (i = 0; i < h->meta_contexts.len; ++i)
> @@ -139,8 +138,7 @@ nbd_close (struct nbd_handle *h)
>    free_cmd_list (h->cmds_to_issue);
>    free_cmd_list (h->cmds_in_flight);
>    free_cmd_list (h->cmds_done);
> -  string_vector_iter (&h->argv, (void *) free);
> -  free (h->argv.ptr);
> +  string_vector_empty (&h->argv);
>    if (h->sact_sockpath) {
>      if (h->pid > 0)
>        kill (h->pid, SIGTERM);
> @@ -164,8 +162,7 @@ nbd_close (struct nbd_handle *h)
>    free (h->tls_certificates);
>    free (h->tls_username);
>    free (h->tls_psk_file);
> -  string_vector_iter (&h->request_meta_contexts, (void *) free);
> -  free (h->request_meta_contexts.ptr);
> +  string_vector_empty (&h->request_meta_contexts);
>    free (h->hname);
>    pthread_mutex_destroy (&h->lock);
>    free (h);
> @@ -379,8 +376,7 @@ nbd_unlocked_get_meta_context (struct nbd_handle *h,
size_t i)
>  int
>  nbd_unlocked_clear_meta_contexts (struct nbd_handle *h)
>  {
> -  string_vector_iter (&h->request_meta_contexts, (void *) free);
> -  string_vector_reset (&h->request_meta_contexts);
> +  string_vector_empty (&h->request_meta_contexts);
>    return 0;
>  }
>  
> diff --git a/lib/utils.c b/lib/utils.c
> index c229ebfc6106..bba4b3846e77 100644
> --- a/lib/utils.c
> +++ b/lib/utils.c
> @@ -93,8 +93,7 @@ nbd_internal_set_argv (struct nbd_handle *h, char **argv)
>      return -1;
>    }
>  
> -  string_vector_iter (&h->argv, (void *) free);
> -  string_vector_reset (&h->argv);
> +  string_vector_empty (&h->argv);
>  
>    if (nbd_internal_copy_string_list (&h->argv, argv) == -1) {
>      set_error (errno, "realloc");
> @@ -110,8 +109,7 @@ nbd_internal_set_argv (struct nbd_handle *h, char
**argv)
>  int
>  nbd_internal_set_querylist (struct nbd_handle *h, char **queries)
>  {
> -  string_vector_iter (&h->querylist, (void *) free);
> -  string_vector_reset (&h->querylist);
> +  string_vector_empty (&h->querylist);
>  
>    if (queries) {
>      if (nbd_internal_copy_string_list (&h->querylist, queries) ==
-1) {
> diff --git a/common/utils/test-vector.c b/common/utils/test-vector.c
> index 55c8ebeb8a1e..05ac5cec3dba 100644
> --- a/common/utils/test-vector.c
> +++ b/common/utils/test-vector.c
> @@ -151,8 +151,7 @@ test_string_vector (void)
>      printf ("%s\n", v.ptr[i]);
>    assert (i == 10);
>  
> -  string_vector_iter (&v, (void*)free);
> -  string_vector_reset (&v);
> +  string_vector_empty (&v);
>  }
>  
>  static void
> diff --git a/info/show.c b/info/show.c
> index 4bf596715cf9..e6c9b9bf1243 100644
> --- a/info/show.c
> +++ b/info/show.c
> @@ -275,8 +275,7 @@ show_one_export (struct nbd_handle *nbd, const char
*desc,
>        fprintf (fp, "\t},\n");
>    }
>  
> -  string_vector_iter (&contexts, (void *) free);
> -  free (contexts.ptr);
> +  string_vector_empty (&contexts);
>    free (content);
>    free (export_name);
>    free (export_desc);
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Wed, Feb 15, 2023 at 03:11:34PM +0100, Laszlo Ersek
wrote:
> The "name##_iter" function is used 11 times in libnbd; in all
those cases,
> "name" is "string_vector", and the "f"
callback is "free":
> 
>   string_vector_iter (..., (void *) free);
> 
> Casting "free" to (void*) is ugly. (Well-defined by POSIX, but
still.)
Tangentially related: casting function pointers in general may get
harder as more compilers move towards C23 and its newer rules (see for
example
https://lists.gnu.org/archive/html/bug-gnulib/2023-02/msg00055.html or
this gcc 13 bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108694
which highlights some of the warnings that newer compilers will start
warning us about).  While this patch focuses on avoiding casts between
fn(*)(type*) and void*, I don't know off-hand if C23 will permit
fn(*)(void*) as a compatible function pointer with fn(*)(type).
> 
> Furthermore, in all 11 cases, the freeing of the vector's strings is
> immediately followed by the release of the vector's array-of-pointers
too.
> (This additional step is not expressed consistently across libnbd: it's
> sometimes spelled as free(vec.ptr), sometimes as
> string_vector_reset(&vec).)
As Rich pointed out, just because libnbd is always passing 'free' does
not mean that nbdkit is doing likewise, and we want to keep this file
shared between the two projects.  Being able to conditionally add the
cleanup function (where we don't want it on non-pointer vectors) makes
sense, but removing the iterator function at the same time is a step
too far.
> 
> Remove the generic "name##_iter" function definition, and
introduce
> "name##_empty", which performs both steps at the same time.
Convert the
> call sites. (Note that the converted code performs more cleanup steps in
> some cases than strictly necessary, but the extra work is harmless, and
> arguably beneficial for clarity / consistency.)
Agreed that the extra cleanup is not going to affect our hot path.
> 
> Expose the "name##_empty" function definition with a new,
separate macro:
> DEFINE_VECTOR_EMPTY(). The existent DEFINE_VECTOR_TYPE() macro permits
> such element types that are not pointers, or are pointers to const- and/or
> volatile-qualified objects. Whereas "name##_empty" requires that
the
> elements be pointers to dynamically allocated, non-const, non-volatile
> objects.
> 
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> +++ b/common/utils/vector.h
>  
> +/* This macro should only be used if:
> + * - the vector contains pointers, and
> + * - the pointed-to objects are:
> + *   - neither const- nor volatile-qualified, and
> + *   - allocated with malloc() or equivalent.
> + */
> +#define DEFINE_VECTOR_EMPTY(name)                                      \
> +  /* Call free() on each element of the vector, then reset the vector. \
> +   */                                                                  \
> +  static inline void __attribute__ ((__unused__))                      \
> +  name##_empty (name *v)                                               \
> +  {                                                                    \
> +    size_t i;                                                          \
> +    for (i = 0; i < v->len; ++i)                                    
\
> +      free (v->ptr[i]);                                               
\
> +    name##_reset (v);                                                  \
> +  }
> +
Thinking higher-level now, your new macro is something where we have
to do a two-step declaration of macro types where we want the new
function.  Would it make more sense to change the signature of the
DEFINE_VECTOR_TYPE() macro to take a third argument containing the
function name to call on cleanup paths, with the ability to easily
write/reuse a no-op function for vectors that don't need to call
free(), where we can then unconditionally declare name##_empty() that
will work with all vector types?  That is, should we consider instead
doing something like:

DEFINE_VECTOR_TYPE (string_vector, char *, free);

DEFINE_VECTOR_TYPE (int_vector, int, noop);
> +++ b/generator/states-connect-socket-activation.c
> @@ -91,8 +91,7 @@ prepare_socket_activation_environment (string_vector
*env)
>  
>   err:
>    set_error (errno, "malloc");
> -  string_vector_iter (env, (void *) free);
> -  free (env->ptr);
> +  string_vector_empty (env);
>    return -1;
At any rate, I like how the callers look improved, even if we aren't
quite settled on how the vector.h changes should end up.

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