[Libguestfs] [PATCH libnbd 2/3] lib: Implement closure lifetimes.
Richard W.M. Jones
rjones at redhat.com
Wed Jul 24 16:52:22 UTC 2019
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/
More information about the Libguestfs
mailing list