[Libguestfs] [PATCH libnbd v2 04/10] lib: Permit .callback = NULL, .free != NULL.

[Eric Blake]

On 8/15/19 4:56 AM, Richard W.M. Jones wrote:
> Previously the .free function of a callback was not called if the
> .callback field was NULL, because the callback as a whole would be
> considered to be "null".
> 
> This change allows you to register callbacks where the .callback field
> is NULL, but the .free field is != NULL, meaning that the callback is
> freed after the last time it would have been used.
> 
> This is mainly convenient for language bindings where we sometimes
> want to register a free function to clean up a persistent buffer, but
> we don't need the associated completion callback to be actually
> called.
> ---
>  docs/libnbd.pod | 15 +++++++++++++++
>  lib/internal.h  | 18 +++++++++---------
>  2 files changed, 24 insertions(+), 9 deletions(-)
> 

> +++ b/lib/internal.h
> @@ -274,20 +274,20 @@ struct command {
>  };
>  
>  /* Test if a callback is "null" or not, and set it to null. */
> -#define CALLBACK_IS_NULL(cb)     ((cb).callback == NULL)
> +#define CALLBACK_IS_NULL(cb)     ((cb).callback == NULL && (cb).free == NULL)

Semantic change. In generator, you used CALLBACK_IS_NULL() for both
Closure and OClosure.  For OClosure, the new semantics are still
correct.  But for Closure, we now no longer return EFAULT when the
callback itself is missing but a .free was provided.  This changes
pread_structured and block_status to accept NULL for the callback; which
is probably not a good idea.

In short, there are some points in the code that care only whether
.callback is NULL, and others that care whether both pointers are NULL.


>  #define CALLBACK_IS_NOT_NULL(cb) (! CALLBACK_IS_NULL ((cb)))
> -#define SET_CALLBACK_TO_NULL(cb) ((cb).callback = NULL)
> +#define SET_CALLBACK_TO_NULL(cb) ((cb).callback = NULL, (cb).free = NULL)
>  
>  /* Call a callback. */
> -#define CALL_CALLBACK(cb, ...) \
> -  (cb).callback ((cb).user_data, ##__VA_ARGS__)
> +#define CALL_CALLBACK(cb, ...)                                          \
> +  ((cb).callback != NULL ? (cb).callback ((cb).user_data, ##__VA_ARGS__) : 0)

This one is nice, though.

>  
>  /* Free a callback. */
> -#define FREE_CALLBACK(cb)                                               \
> -  do {                                                                  \
> -    if (CALLBACK_IS_NOT_NULL (cb) && (cb).free != NULL)                 \
> -      (cb).free ((cb).user_data);                                       \
> -    SET_CALLBACK_TO_NULL (cb);                                          \
> +#define FREE_CALLBACK(cb)                               \
> +  do {                                                  \
> +    if ((cb).free != NULL)                              \
> +      (cb).free ((cb).user_data);                       \
> +    SET_CALLBACK_TO_NULL (cb);                          \

The SET_CALLBACK_TO_NULL usage is mostly dead code during command
retirement (as we are about to free() the struct containing cb in the
first place), but still vital when clearing the debug callback.  So this
part is also fine.

>    } while (0)
>  
>  /* aio.c */
> 

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190815/893cf70d/attachment.sig>