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