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 <ler...@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@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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
