On 2/15/23 17:23, Richard W.M. Jones wrote:
> 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.
Good point -- the header is shared by nbdkit and libnbd, but (because
the latter are two separate projects) any single git-grep will cover
only one.
I'll restore _iter.
>
>> /* 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 ...
Yes, someone has to be! I knew it was virtually impossible for me to get
the name right at the first try :)
>
> Can we call it ADD_VECTOR_EMPTY_METHOD or similar/better?
Eric, any comments?
> 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.
Thanks!
Laszlo
>
> 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
>
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
