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

On 2/15/23 21:27, Eric Blake 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.)
> 
> 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).
My understanding is that, per C99 at least, ret_type(*)(void*) is
compatible with ret_type(*)(type*) precisely if void* is compatible with
type* (6.7.5.3p15).

Whether void* is compatible with type* depends on... ugh, that's hard to
deduce from the standard. 6.7.5.1p2 says, "For two pointer types to be
compatible, both shall be identically qualified and both shall
be pointers to compatible types". I don't think "void" (as a
type in
itself) is compatible with any type!

Now, there is one particular statement on void* -- 6.2.5p27 says, "A
pointer to void shall have the same representation and alignment
requirements as a pointer to a character type."

(I think the statements about *converting* void* to type* and vice versa
do not apply here; AFAICT "compatibility" is about reinterpreting the
bit patterns, not converting values.)

In Annex I (Common warnings, "informative"), the following is listed:
"An implicit narrowing conversion is encountered, such as the assignment
of [...] a pointer to void to a pointer to any type other than a
character type".

All in all I don't think ret_type(*)(type*) is compatible with
ret_type(*)(void*) in the general case, and that's why in this patch I
didn't want to go more general than I absolutely needed to.
> 
>>
>> 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.
OK.
> 
>>
>> 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);
My counter-arguments:

- this requires updates to all existent DEFINE_VECTOR_TYPE macro
invocations,

- with "noop" passed to _reset, _reset and _empty become effectively
the
same, so we get (at least partially) duplicate APIs,

- this would be a step towards combinatorial explosion

- if "noop" does nothing, then why call it on each element of the
vector
anyway? It's not only the function call that becomes superfluous in the
loop bodym with the function being "noop", but the loop *itself*
becomes
superfluous. So then we might want to compare the function pointer
against "noop" outside of the loop... and that way we get a bunch of
complications :)

I chose this approach because it is purely additive and precisely as
generic/specific as it needs to be. We already have 11 use cases, so I
don't think it's *too* specific.
> 
>> +++ 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.
> 
Thanks!
Laszlo

On Mon, Feb 20, 2023 at 06:03:13PM +0100, Laszlo Ersek
wrote:
> On 2/15/23 21:27, Eric Blake 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.)
> > 
> > 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).
> 
> My understanding is that, per C99 at least, ret_type(*)(void*) is
> compatible with ret_type(*)(type*) precisely if void* is compatible with
> type* (6.7.5.3p15).
> 
> Whether void* is compatible with type* depends on... ugh, that's hard
to
> deduce from the standard. 6.7.5.1p2 says, "For two pointer types to be
> compatible, both shall be identically qualified and both shall
> be pointers to compatible types". I don't think "void"
(as a type in
> itself) is compatible with any type!
> 
> Now, there is one particular statement on void* -- 6.2.5p27 says, "A
> pointer to void shall have the same representation and alignment
> requirements as a pointer to a character type."
> 
> (I think the statements about *converting* void* to type* and vice versa
> do not apply here; AFAICT "compatibility" is about reinterpreting
the
> bit patterns, not converting values.)
> 
> In Annex I (Common warnings, "informative"), the following is
listed:
> "An implicit narrowing conversion is encountered, such as the
assignment
> of [...] a pointer to void to a pointer to any type other than a
> character type".
> 
> All in all I don't think ret_type(*)(type*) is compatible with
> ret_type(*)(void*) in the general case, and that's why in this patch I
> didn't want to go more general than I absolutely needed to.
Thanks for at least trying to find something definitive in the
standard.  Now you know why I skipped researching this particular
issue - it's not straightforward to figure out when function pointers
with differing parameter types are compatible.
> > 
> > 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);
> 
> My counter-arguments:
> 
> - this requires updates to all existent DEFINE_VECTOR_TYPE macro
> invocations,
> 
> - with "noop" passed to _reset, _reset and _empty become
effectively the
> same, so we get (at least partially) duplicate APIs,
> 
> - this would be a step towards combinatorial explosion
> 
> - if "noop" does nothing, then why call it on each element of the
vector
> anyway? It's not only the function call that becomes superfluous in the
> loop bodym with the function being "noop", but the loop *itself*
becomes
> superfluous. So then we might want to compare the function pointer
> against "noop" outside of the loop... and that way we get a bunch
of
> complications :)
> 
> I chose this approach because it is purely additive and precisely as
> generic/specific as it needs to be. We already have 11 use cases, so I
> don't think it's *too* specific.
We may still want some division of:

DEFINE_VECTOR_TYPE (int_vector, int);
DEFINE_POINTER_VECTOR_TYPE (string_vector, char *, free);

where under the hood, DEFINE_POINTER_VECTOR_TYPE(type, base, fn)
invokes both DEFINE_VECTOR_TYPE(type, base) and
DEFINE_VECTOR_EMPTY(type, fn), or whatever we name the second
function.
>From your other mail in this subthread:
> >> +#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?
ADD_VECTOR_EMPTY_METHOD() instead of DEFINE_VECTOR_EMPTY() works for
me.  Whether we hard-code 'free()' as the lone function that will be
utilized in the generated TYPE_vector_empty(), or if the macro takes
fn as a parameter, is up to you.

Comparing:

DEFINE_VECTOR_TYPE(string_vector, char *);
ADD_VECTOR_EMPTY_METHOD(string_vector);

vs.

DEFINE_VECTOR_TYPE(string_vector, char *);
ADD_VECTOR_EMPTY_METHOD(string_vector, free);

does either one make it more obvious that we are doing a two-step
definition (first of the type, then of the added cleanup method)?  And
if so, does my idea of a single wrapper function that calls both
intermediate macros make sense so that the 11 pointer vector types in
libnbd are still one-liner macro calls, without penalizing the scalar
vector types in nbdkit?

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