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

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

On Thu, Aug 15, 2019 at 06:57:01AM -0500, Eric Blake
wrote:
> 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.
I'm unclear on this.  This is the point of the patch - that you can
register a callback which has no callback action but still performs
the free action, and that's not considered a null callback.  (It's
considered to be a callback that does nothing and returns 0.)  Does
something bad happen to those two calls in this case?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On 8/15/19 7:02 AM, Richard W.M. Jones wrote:
>>>  
>>>  /* 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.
> 
> I'm unclear on this.  This is the point of the patch - that you can
> register a callback which has no callback action but still performs
> the free action, and that's not considered a null callback.  (It's
> considered to be a callback that does nothing and returns 0.)  Does
> something bad happen to those two calls in this case?
I guess nothing bad happens (for block status, you have basically turned
your command into nothing more than a success/failure check of if the
server replied, but you don't get any status; for pread_structured,
lib/rw.c actually forwards .pread to .pread_structured with
.callback=NULL under the hood).  For debug, it means you can register a
notifier for when someone else sets/clears the debug notifier, but still
get the default debug notifications.

Ok, the semantic change doesn't hurt.

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