Libguestfs - Jul 2019 - Re: [PATCH libnbd 2/3] lib: Implement closure lifetimes.

On 7/24/19 7:17 AM, Richard W.M. Jones wrote:
> Previously closures had a crude flag which tells if they are
> persistent or transient.  Transient closures (flag = false) last for
> the lifetime of the currently called libnbd function.  Persistent
> closures had an indefinite lifetime which could last for as long as
> the handle.  In language bindings handling persistent closures was
> wasteful as we needed to register a "close callback" to free the
> closure when the handle is closed.  But if you had submitted thousands
> of asynchronous commands you would end up registering thousands of
> close callbacks.
> 
> We actually have far more information about the lifetime of closures.
> We know precisely when they will no longer be needed by the library.
> Callers can use this information to free up associated user data
> earlier.  In particular in language bindings we can remove roots /
> decrement reference counts at the right place to free the closure,
> without waiting for the handle to be closed.
> 
> The solution to this is to introduce the concept of a closure
> lifetime.  The callback is called with an extra valid_flag parameter
> which is a bitmap containing LIBNBD_CALLBACK_VALID and/or
> LIBNBD_CALLBACK_FREE.  LIBNBD_CALLBACK_VALID corresponds to a normal
> call of the callback function by the library.  After the library has
> finished with the callback (declaring that this callback will never be
> needed or called again), it is called once more with
> valid_flag == LIBNBD_CALLBACK_FREE.
> 
> (Note it is also possible for the library to call the callback with
> valid_flag == LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, meaning it's
the
> last valid call.)
> 
> 
> diff --git a/docs/libnbd.pod b/docs/libnbd.pod
> index 631bb3b..e9a6030 100644
> --- a/docs/libnbd.pod
> +++ b/docs/libnbd.pod
> @@ -487,7 +487,59 @@ C<nbd_set_debug_callback>,
C<nbd_pread_callback>).  Libnbd can call
>  these functions while processing.
>  
>  Callbacks have an opaque C<void *user_data> pointer.  This is passed
> -as the first parameter to the callback.
> +as the second parameter to the callback.
> +
> +=head2 Callback lifetimes
> +
> +All callbacks have an C<int valid_flag> parameter which is used to
> +help with the lifetime of the callback.  C<valid_flag> contains the
> +I<logical or> of:
Again, worth mentioning that this explicitly only for the C bindings?
> +
> +=over 4
> +
> +=item C<LIBNBD_CALLBACK_VALID>
> +
> +The callback parameters are valid and this is a normal callback.
> +
> +=item C<LIBNBD_CALLBACK_FREE>
> +
> +This is the last time the library will call this function.  Any data
> +associated with the callback can be freed.
> +
> +=item other bits
> +
> +Other bits in C<valid_flag> should be ignored.
Are we guaranteeing never to set other bits, or that other bits will be
0 for now and only set later if the client opts in to whatever the new
bits are useful for?  But I'm okay with leaving this sentence as worded.
> +
> +=back
> +
> +For example C<nbd_set_debug_callback> sets up a callback which you
> +could define like this:
> +
> + int my_debug_fn (int valid_flag, void *user_data,
> +                  const char *context, const char *msg)
> + {
> +   if (valid_flag & LIBNBD_CALLBACK_VALID) {
> +     printf ("context = %s, msg = %s\n", context, msg);
> +   }
> +   if (valid_flag & LIBNBD_CALLBACK_FREE) {
> +     /* If you need to free something, for example: */
> +     free (user_data);
> +   }
> +   return 0;
> + }
> +
> +If you don't care about the freeing feature then it is also correct to
> +write this:
> +
> + int my_debug_fn (int valid_flag, void *user_data,
> +                  const char *context, const char *msg)
> + {
> +   if (!(valid_flag & LIBNBD_CALLBACK_VALID)) return 0;
> +
> +   // Rest of callback as normal.
> + }
Both examples are good.

> +++ b/examples/strict-structured-reads.c
> @@ -51,12 +51,16 @@ static int64_t total_bytes;
>  static int total_success;
>  
>  static int
> -read_chunk (void *opaque, const void *bufv, size_t count, uint64_t offset,
> +read_chunk (int valid_flag, void *opaque,
> +            const void *bufv, size_t count, uint64_t offset,
>              int status, int *error)
>  {
>    struct data *data = opaque;
>    struct range *r, **prev;
>  
> +  if (!(valid_flag & LIBNBD_CALLBACK_VALID))
> +    return 0;
> +
This one is okay (since we pass the same opaque to both this function
and the later callback function, so have nothing to free at the last use
of this one)...
>    /* libnbd guarantees this: */
>    assert (offset >= data->offset);
>    assert (offset + count <= data->offset + data->count);
> @@ -127,11 +131,14 @@ read_chunk (void *opaque, const void *bufv, size_t
count, uint64_t offset,
>  }
>  
>  static int
> -read_verify (void *opaque, int64_t cookie, int *error)
> +read_verify (int valid_flag, void *opaque, int64_t cookie, int *error)
>  {
>    struct data *data = opaque;
>    int ret = -1;
>  
> +  if (!(valid_flag & LIBNBD_CALLBACK_VALID))
> +    return 0;
...but this one is wrong; it calls free(data) which should be deferred
to the valid_flag & LIBNBD_CALLBACK_FREE portion. (It looks like later
on in the patch, I see that the nbd_aio_FOO_callback callback is always
called exactly once with VALID|FREE, so your bug here is masked because
of that calling convention - but we should really fix this one to be a
better example).
> +++ b/generator/generator
> @@ -849,10 +849,7 @@ and arg >                                written by
the function *)
>  | BytesPersistIn of string * string (* same as above, but buffer persists
*)
>  | BytesPersistOut of string * string
> -| Closure of bool * closure (* function pointer + void *opaque
> -                               flag if true means callbacks persist
> -                               in the handle, false means they only
> -                               exist during the function call *)
> +| Closure of closure       (* function pointer + void *opaque *)
Re-echoing my comment in 1/3, writing this as 'Closure of string * arg
list' may seem cleaner (certainly less typing below).

> @@ -3196,6 +3180,12 @@ let rec print_arg_list ?(handle = false) ?(user_data
= false)
>      if types then pr "struct nbd_handle *";
>      pr "h"
>    );
> +  if valid_flag then (
> +    if !comma then pr ", ";
> +    comma := true;
> +    if types then pr "int ";
> +    pr "valid_flag";
Should this be 'unsigned int valid_flag', as bitmasks over signed values
have awkward semantics if you touch the sign bit?  But I'm okay with int
for now, as it matches 'int nbd_aio_get_direction(...)' and
nbd_pread_structured using 'int status' in its callback.

> @@ -3228,10 +3218,10 @@ let rec print_arg_list ?(handle = false)
?(user_data = false)
>           pr "%s, " n;
>           if types then pr "size_t ";
>           pr "%s" len
> -      | Closure (_, { cbname; cbargs }) ->
> +      | Closure { cbname; cbargs } ->
>           if types then (
>             pr "int (*%s) " cbname;
> -           print_arg_list ~user_data:true cbargs;
> +               print_arg_list ~valid_flag:true ~user_data:true cbargs;
Indentation.

> +    | Closure { cbname; cbargs } ->
>         pr "/* Wrapper for %s callback of %s. */\n" cbname name;
>         pr "static int\n";
>         pr "%s_%s_wrapper " name cbname;
> -       C.print_arg_list ~user_data:true cbargs;
> +       C.print_arg_list ~valid_flag:true ~user_data:true cbargs;
>         pr "\n";
>         pr "{\n";
> +       pr "  if (valid_flag & LIBNBD_CALLBACK_VALID) {\n";
>         pr "  int ret;\n";
>         pr "  PyGILState_STATE py_save = PyGILState_UNLOCKED;\n";
Do we care about the generated indentation looking sane?  (That can be a
followup patch, to minimize the churn on this one...)
>         pr "  PyObject *py_args, *py_ret;\n";
> @@ -3927,6 +3907,12 @@ let print_python_binding name { args; ret } >    
| UInt _ | UInt32 _ -> assert false
>           ) cbargs;
>         pr "  return ret;\n";
> +       pr "  }\n";
Ouch. Calling 'return ret' before checking for LIBNBD_CALLBACK_FREE is
liable to leak.  I think you have to defer the return...
> +       pr "\n";
> +       pr "  if (valid_flag & LIBNBD_CALLBACK_FREE)\n";
> +       pr "    Py_DECREF ((PyObject *)user_data);\n";
> +       pr "\n";
> +       pr "  return 0;\n";
...to here.

> @@ -4048,13 +4034,12 @@ let print_python_binding name { args; ret } >   
pr "))\n";
>    pr "    return NULL;\n";
>  
> -  (* If the parameter has persistent closures then we need to
> +  (* If the parameter has closures then we need to
>     * make sure the ref count remains positive.
>     *)
>    List.iter (
>      function
> -    | Closure (false, _) -> ()
> -    | Closure (true, { cbname }) ->
> +    | Closure { cbname } ->
Now that this match...
>         pr "  Py_INCREF (%s_user_data);\n" cbname
>      | _ -> ()
>    ) args;
> @@ -4086,7 +4071,7 @@ let print_python_binding name { args; ret } >     
pr "  %s = malloc (%s);\n" n count
>      | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
>         pr "  %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n
n
> -    | Closure (_, { cbname }) ->
> +    | Closure { cbname } ->
>         pr "  if (!PyCallable_Check (%s_user_data)) {\n" cbname;
>         pr "    PyErr_SetString (PyExc_TypeError,\n";
>         pr "                     \"callback parameter %s is not
callable\");\n" cbname;
...and this match are identical, should we group the code?

> +++ b/generator/states-reply-simple.c
I got this far in my review; I'll resume later...

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

On Wed, Jul 24, 2019 at 10:18:20AM -0500, Eric Blake
wrote:
> On 7/24/19 7:17 AM, Richard W.M. Jones wrote:
> > +=head2 Callback lifetimes
> > +
> > +All callbacks have an C<int valid_flag> parameter which is used
to
> > +help with the lifetime of the callback.  C<valid_flag> contains
the
> > +I<logical or> of:
> 
> Again, worth mentioning that this explicitly only for the C bindings?
Yes, I'll add that in another commit on top.
> > +=item other bits
> > +
> > +Other bits in C<valid_flag> should be ignored.
> 
> Are we guaranteeing never to set other bits, or that other bits will be
> 0 for now and only set later if the client opts in to whatever the new
> bits are useful for?  But I'm okay with leaving this sentence as
worded.
I can't at the moment imagine what other flags would be useful here,
but as long as we've told correct callers that they must ignore flags
that they don't know about it may be possible to add new features here
in future without breaking ABI.
> > +++ b/examples/strict-structured-reads.c
> > @@ -127,11 +131,14 @@ read_chunk (void *opaque, const void *bufv,
size_t count, uint64_t offset,
> >  }
> >  
> >  static int
> > -read_verify (void *opaque, int64_t cookie, int *error)
> > +read_verify (int valid_flag, void *opaque, int64_t cookie, int
*error)
> >  {
> >    struct data *data = opaque;
> >    int ret = -1;
> >  
> > +  if (!(valid_flag & LIBNBD_CALLBACK_VALID))
> > +    return 0;
> 
> ...but this one is wrong; it calls free(data) which should be deferred
> to the valid_flag & LIBNBD_CALLBACK_FREE portion. (It looks like later
> on in the patch, I see that the nbd_aio_FOO_callback callback is always
> called exactly once with VALID|FREE, so your bug here is masked because
> of that calling convention - but we should really fix this one to be a
> better example).
Ah, I see.  I can fix this in the commit.
> > @@ -3196,6 +3180,12 @@ let rec print_arg_list ?(handle = false)
?(user_data = false)
> >      if types then pr "struct nbd_handle *";
> >      pr "h"
> >    );
> > +  if valid_flag then (
> > +    if !comma then pr ", ";
> > +    comma := true;
> > +    if types then pr "int ";
> > +    pr "valid_flag";
> 
> Should this be 'unsigned int valid_flag', as bitmasks over signed
values
> have awkward semantics if you touch the sign bit?  But I'm okay with
int
> for now, as it matches 'int nbd_aio_get_direction(...)' and
> nbd_pread_structured using 'int status' in its callback.
Yes I can change it to unsigned - using a signed type was a mistake.

I think we can change nbd_pread_structured (and friends) to use either
unsigned or uint32_t which is consistent with other flag parameters.
However we can't easily change nbd_aio_get_direction as that needs to
be a signed type to indicate errors, at least without awkward casting.
I'll come up with a separate patch.
> > @@ -3228,10 +3218,10 @@ let rec print_arg_list ?(handle = false)
?(user_data = false)
> >           pr "%s, " n;
> >           if types then pr "size_t ";
> >           pr "%s" len
> > -      | Closure (_, { cbname; cbargs }) ->
> > +      | Closure { cbname; cbargs } ->
> >           if types then (
> >             pr "int (*%s) " cbname;
> > -           print_arg_list ~user_data:true cbargs;
> > +               print_arg_list ~valid_flag:true ~user_data:true
cbargs;
> 
> Indentation.
Ah OK, I thought I'd got most of these.  I had to rebase this patch
using -X ignore-space-change which is wonderful but doesn't get
indentation right.
> > +    | Closure { cbname; cbargs } ->
> >         pr "/* Wrapper for %s callback of %s. */\n" cbname
name;
> >         pr "static int\n";
> >         pr "%s_%s_wrapper " name cbname;
> > -       C.print_arg_list ~user_data:true cbargs;
> > +       C.print_arg_list ~valid_flag:true ~user_data:true cbargs;
> >         pr "\n";
> >         pr "{\n";
> > +       pr "  if (valid_flag & LIBNBD_CALLBACK_VALID)
{\n";
> >         pr "  int ret;\n";
> >         pr "  PyGILState_STATE py_save =
PyGILState_UNLOCKED;\n";
> 
> Do we care about the generated indentation looking sane?  (That can be a
> followup patch, to minimize the churn on this one...)
I fixed this in the same patch now.
> >         pr "  PyObject *py_args, *py_ret;\n";
> > @@ -3927,6 +3907,12 @@ let print_python_binding name { args; ret }
> >             | UInt _ | UInt32 _ -> assert false
> >           ) cbargs;
> >         pr "  return ret;\n";
> > +       pr "  }\n";
> 
> Ouch. Calling 'return ret' before checking for LIBNBD_CALLBACK_FREE
is
> liable to leak.  I think you have to defer the return...
Yup, fixed now.
> > +       pr "\n";
> > +       pr "  if (valid_flag & LIBNBD_CALLBACK_FREE)\n";
> > +       pr "    Py_DECREF ((PyObject *)user_data);\n";
> > +       pr "\n";
> > +       pr "  return 0;\n";
> 
> ...to here.
> 
> 
> > @@ -4048,13 +4034,12 @@ let print_python_binding name { args; ret }
> >    pr "))\n";
> >    pr "    return NULL;\n";
> >  
> > -  (* If the parameter has persistent closures then we need to
> > +  (* If the parameter has closures then we need to
> >     * make sure the ref count remains positive.
> >     *)
> >    List.iter (
> >      function
> > -    | Closure (false, _) -> ()
> > -    | Closure (true, { cbname }) ->
> > +    | Closure { cbname } ->
> 
> Now that this match...
> 
> >         pr "  Py_INCREF (%s_user_data);\n" cbname
> >      | _ -> ()
> >    ) args;
> > @@ -4086,7 +4071,7 @@ let print_python_binding name { args; ret } >
>         pr "  %s = malloc (%s);\n" n count
> >      | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
> >         pr "  %s_buf = nbd_internal_py_get_aio_buffer
(%s);\n" n n
> > -    | Closure (_, { cbname }) ->
> > +    | Closure { cbname } ->
> >         pr "  if (!PyCallable_Check (%s_user_data)) {\n"
cbname;
> >         pr "    PyErr_SetString (PyExc_TypeError,\n";
> >         pr "                     \"callback parameter %s is
not callable\");\n" cbname;
> 
> ...and this match are identical, should we group the code?
Yes, I'll fix this too.

Thanks for this - I'll post an updated patch with all the changes
requested in a moment.

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 7/24/19 11:52 AM, Richard W.M. Jones wrote:
>>> +  if valid_flag then (
>>> +    if !comma then pr ", ";
>>> +    comma := true;
>>> +    if types then pr "int ";
>>> +    pr "valid_flag";
>>
>> Should this be 'unsigned int valid_flag', as bitmasks over
signed values
>> have awkward semantics if you touch the sign bit?  But I'm okay
with int
>> for now, as it matches 'int nbd_aio_get_direction(...)' and
>> nbd_pread_structured using 'int status' in its callback.
> 
> Yes I can change it to unsigned - using a signed type was a mistake.
> 
> I think we can change nbd_pread_structured (and friends) to use either
> unsigned or uint32_t which is consistent with other flag parameters.
> However we can't easily change nbd_aio_get_direction as that needs to
> be a signed type to indicate errors, at least without awkward casting.
> I'll come up with a separate patch.
nbd_aio_get_direction is currently documented as 'can't fail', which
means it never returns negative. So that return type could perhaps be
fixed too.

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