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

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(a)redhat.com&gt; > --- > 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(a)redhat.com > https://listman.redhat.com/mailman/listinfo/libguestfs