[PATCH libnbd 0/7] Add free callbacks and remove valid_flag.
As proposed here: https://www.redhat.com/archives/libguestfs/2019-August/msg00130.html I didn't actually read Eric's replies to that yet because I've been concentrating on writing these patches all day. Anyway here they are and I'll look at what Eric said about the proposal next. Rich.
This adds a C-only semi-private function for freeing various types of persistent data passed to libnbd. There are some similarities with nbd_add_close_callback which we removed in commit 7f191b150b52ed50098976309a6af883d245fc56. --- generator/generator | 57 ++++++++++++++++++++ lib/Makefile.am | 1 + lib/free.c | 129 ++++++++++++++++++++++++++++++++++++++++++++ lib/handle.c | 6 +++ lib/internal.h | 14 +++++ 5 files changed, 207 insertions(+) diff --git a/generator/generator b/generator/generator index c52200b..d6b9352 100755 --- a/generator/generator +++ b/generator/generator @@ -3284,6 +3284,7 @@ let generate_lib_libnbd_syms () = pr "LIBNBD_%d.%d {\n" major minor; pr " global:\n"; if (major, minor) = (1, 0) then ( + pr " nbd_add_free_callback;\n"; pr " nbd_create;\n"; pr " nbd_close;\n"; pr " nbd_get_errno;\n"; @@ -3581,6 +3582,13 @@ let generate_include_libnbd_h () = pr "extern int nbd_get_errno (void);\n"; pr "#define LIBNBD_HAVE_NBD_GET_ERRNO 1\n"; pr "\n"; + pr "typedef void (*nbd_free_callback) (void *ptr, void *user_data);\n"; + pr "extern int nbd_add_free_callback (struct nbd_handle *h,\n"; + pr " void *ptr,\n"; + pr " nbd_free_callback cb,\n"; + pr " void *user_data);\n"; + pr "#define LIBNBD_HAVE_NBD_ADD_FREE_CALLBACK 1\n"; + pr "\n"; print_closure_typedefs (); List.iter ( fun (name, { args; optargs; ret }) -> @@ -4005,6 +4013,55 @@ C<E<lt>errno.hE<gt>>. List.iter print_api handle_calls; pr "\ +=head1 FREE CALLBACKS + +B<Note:> The API described in this section is only +available from C. It is designed to help when writing +bindings to libnbd in other programming languages. +As such it is B<not> covered by the usual API stability +guarantee offered by other parts of the C API. Use it with care. + +Some pointers passed to libnbd calls are saved in the +S<C<struct nbd_handle>>. These include pointers to buffers +passed to C<nbd_aio_pread>, C<nbd_aio_pwrite>, etc., +and pointers to the C<user_data> for callbacks. If you +want to know when it is safe to free these objects then +you can register a free callback using: + + typedef void (*nbd_free_callback) (void *ptr, void *user_data); + int nbd_add_free_callback (struct nbd_handle *h, + void *ptr, + nbd_free_callback cb, + void *user_data); + +C<ptr> is a pointer to the object (ie. the buffer or +callback C<user_data>). C<cb (ptr)> is called when libnbd +no longer holds a reference to that object. + +To illustrate how this works with an example: + + struct write_completed_user_data *my_user_data; + void *buf; + + my_user_data = malloc (sizeof *my_user_data); + buf = malloc (512); + nbd_add_free_callback (nbd, my_user_data, my_free, NULL); + nbd_add_free_callback (nbd, buf, my_free, NULL); + nbd_aio_pwrite_callback (nbd, buf, 512, 0, + write_completed, my_user_data, 0); + +where C<my_free> is a simple wrapper around the L<free(3)> +function (we are not using the extra C<user_data> pointer +in this case): + + void my_free (void *ptr, void *) + { + free (ptr); + } + +This would free both C<my_user_data> and C<buf> once libnbd +has finished with them. + =head1 SEE ALSO L<libnbd(3)>. diff --git a/lib/Makefile.am b/lib/Makefile.am index 3657b9f..08e9d25 100644 --- a/lib/Makefile.am +++ b/lib/Makefile.am @@ -42,6 +42,7 @@ libnbd_la_SOURCES = \ disconnect.c \ errors.c \ flags.c \ + free.c \ handle.c \ internal.h \ is-state.c \ diff --git a/lib/free.c b/lib/free.c new file mode 100644 index 0000000..67ba752 --- /dev/null +++ b/lib/free.c @@ -0,0 +1,129 @@ +/* NBD client library in userspace + * Copyright (C) 2013-2019 Red Hat Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <errno.h> +#include <assert.h> + +#include "internal.h" + +/* This is not generated because we don't want to offer it to other + * programming languages. Also it's a semi-private API liable to + * evolve in its exact semantics. + */ +int +nbd_add_free_callback (struct nbd_handle *h, void *ptr, + nbd_free_callback cb, void *user_data) +{ + int ret = -1; + size_t i; + + if (ptr == NULL) + return 0; + + nbd_internal_set_error_context ("nbd_add_free_callback"); + pthread_mutex_lock (&h->lock); + + /* Extend the list of callbacks in the handle. */ + if (h->nr_free_callbacks >= h->alloc_free_callbacks) { + size_t new_alloc; + struct free_callback *new_callbacks; + + if (h->alloc_free_callbacks == 0) + new_alloc = 8; + else + new_alloc = 2 * h->alloc_free_callbacks; + + new_callbacks = realloc (h->free_callbacks, + sizeof (struct free_callback) * new_alloc); + if (new_callbacks == NULL) { + set_error (errno, "realloc"); + goto out; + } + h->alloc_free_callbacks = new_alloc; + h->free_callbacks = new_callbacks; + } + + /* Need to keep the list sorted by pointer so we can use bsearch. + * Insert the new entry before the i'th entry. + * + * Note the same pointer can be added multiple times (eg. if a + * buffer is shared between commands), and that is not a bug. Which + * free function is called is undefined, but they should all be + * called eventually. + */ + for (i = 0; i < h->nr_free_callbacks; ++i) { + if (ptr <= h->free_callbacks[i].ptr) + break; + } + memmove (&h->free_callbacks[i+1], &h->free_callbacks[i], + (h->nr_free_callbacks - i) * sizeof (struct free_callback)); + + h->free_callbacks[i].ptr = ptr; + h->free_callbacks[i].cb = cb; + h->free_callbacks[i].user_data = user_data; + h->nr_free_callbacks++; + + ret = 0; + out: + pthread_mutex_unlock (&h->lock); + return ret; +} + +static int +compare_free_callbacks (const void *v1, const void *v2) +{ + const void *ptr = v1; + const struct free_callback *cb = v2; + + if (ptr < cb->ptr) return -1; + else if (ptr > cb->ptr) return 1; + else return 0; +} + +/* Called before the library frees 'ptr', where 'ptr' is something + * that might potentially be associated with a free callback. This is + * called often so must be fast. + */ +void +nbd_internal_free_callback (struct nbd_handle *h, void *ptr) +{ + struct free_callback *free_cb; + + if (ptr == NULL) + return; + + free_cb = bsearch (ptr, h->free_callbacks, h->nr_free_callbacks, + sizeof (struct free_callback), + compare_free_callbacks); + if (free_cb) { + assert (ptr == free_cb->ptr); + + free_cb->cb (ptr, free_cb->user_data); + + /* Remove it from the free list. */ + memmove (free_cb, free_cb+1, + sizeof (struct free_callback) * + (&h->free_callbacks[h->nr_free_callbacks] - (free_cb+1))); + h->nr_free_callbacks--; + } +} diff --git a/lib/handle.c b/lib/handle.c index 054c8a7..ae0d196 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -126,6 +126,12 @@ nbd_close (struct nbd_handle *h) free_cmd_list (h->cmds_to_issue); free_cmd_list (h->cmds_in_flight); free_cmd_list (h->cmds_done); + + /* Any remaining free callbacks indicate an error. */ + if (h->nr_free_callbacks != 0) + abort (); + free (h->free_callbacks); + nbd_internal_free_string_list (h->argv); free (h->unixsocket); free (h->hostname); diff --git a/lib/internal.h b/lib/internal.h index 301b798..d8b0eed 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -47,6 +47,7 @@ struct meta_context; struct socket; struct command; +struct free_callback; struct nbd_handle { /* Unique name assigned to this handle for debug messages @@ -87,6 +88,10 @@ struct nbd_handle { nbd_debug_callback debug_callback; void *debug_data; + /* Free callbacks kept in pointer order. */ + struct free_callback *free_callbacks; + size_t nr_free_callbacks, alloc_free_callbacks; + /* State machine. * * The actual current state is ‘state’. ‘public_state’ is updated @@ -220,6 +225,12 @@ struct nbd_handle { bool disconnect_request; /* True if we've queued NBD_CMD_DISC */ }; +struct free_callback { + void *ptr; + nbd_free_callback cb; + void *user_data; +}; + struct meta_context { struct meta_context *next; /* Linked list. */ char *name; /* Name of meta context. */ @@ -318,6 +329,9 @@ extern int nbd_internal_set_size_and_flags (struct nbd_handle *h, uint64_t exportsize, uint16_t eflags); +/* free.c */ +extern void nbd_internal_free_callback (struct nbd_handle *h, void *ptr); + /* is-state.c */ extern bool nbd_internal_is_state_created (enum state state); extern bool nbd_internal_is_state_connecting (enum state state); -- 2.22.0
On 8/12/19 11:08 AM, Richard W.M. Jones wrote:
This adds a C-only semi-private function for freeing various types of persistent data passed to libnbd.
There are some similarities with nbd_add_close_callback which we removed in commit 7f191b150b52ed50098976309a6af883d245fc56. ---
+=head1 FREE CALLBACKS + +B<Note:> The API described in this section is only +available from C. It is designed to help when writing +bindings to libnbd in other programming languages. +As such it is B<not> covered by the usual API stability +guarantee offered by other parts of the C API. Use it with care. + +Some pointers passed to libnbd calls are saved in the +S<C<struct nbd_handle>>. These include pointers to buffers +passed to C<nbd_aio_pread>, C<nbd_aio_pwrite>, etc., +and pointers to the C<user_data> for callbacks. If you +want to know when it is safe to free these objects then +you can register a free callback using: + + typedef void (*nbd_free_callback) (void *ptr, void *user_data); + int nbd_add_free_callback (struct nbd_handle *h, + void *ptr, + nbd_free_callback cb, + void *user_data);
Do we want to insist on a user_data argument? Libvirt, for example, states that free callbacks are passed only the pointer to be freed (as you can already pack whatever you need into that pointer alone, rather than needing yet another void *user_data); if we do not take a user_data here, then...
+ +C<ptr> is a pointer to the object (ie. the buffer or +callback C<user_data>). C<cb (ptr)> is called when libnbd
As written, your patch uses C<cb (ptr, user_data)>, so either this text is wrong, or you really don't need user_data.
+ +where C<my_free> is a simple wrapper around the L<free(3)> +function (we are not using the extra C<user_data> pointer +in this case): + + void my_free (void *ptr, void *) + { + free (ptr); + }
...without user_data, you wouldn't need a my_free wrapper, but could just pass free() in the example.
+++ b/lib/free.c @@ -0,0 +1,129 @@
+/* This is not generated because we don't want to offer it to other + * programming languages. Also it's a semi-private API liable to + * evolve in its exact semantics. + */ +int +nbd_add_free_callback (struct nbd_handle *h, void *ptr, + nbd_free_callback cb, void *user_data)
At least the comment caveat gives us flexibility to change our mind on whether to provide a user_data argument.
+{ + int ret = -1; + size_t i; + + if (ptr == NULL) + return 0; + + nbd_internal_set_error_context ("nbd_add_free_callback"); + pthread_mutex_lock (&h->lock); + + /* Extend the list of callbacks in the handle. */ + if (h->nr_free_callbacks >= h->alloc_free_callbacks) { + size_t new_alloc; + struct free_callback *new_callbacks; + + if (h->alloc_free_callbacks == 0) + new_alloc = 8; + else + new_alloc = 2 * h->alloc_free_callbacks; + + new_callbacks = realloc (h->free_callbacks, + sizeof (struct free_callback) * new_alloc);
Should we start relying on reallocarray() to guarantee no multiplication overflow? (Not yet in POSIX, but it has been proposed: http://austingroupbugs.net/view.php?id=1218)
+ if (new_callbacks == NULL) { + set_error (errno, "realloc"); + goto out; + } + h->alloc_free_callbacks = new_alloc; + h->free_callbacks = new_callbacks; + } + + /* Need to keep the list sorted by pointer so we can use bsearch. + * Insert the new entry before the i'th entry. + * + * Note the same pointer can be added multiple times (eg. if a + * buffer is shared between commands), and that is not a bug. Which + * free function is called is undefined, but they should all be + * called eventually.
or in other words, if the free function is actually a reference counter that only does something interesting when the count reaches 0, then things still work.
+ */ + for (i = 0; i < h->nr_free_callbacks; ++i) { + if (ptr <= h->free_callbacks[i].ptr)
Comparing 2 pointers that are not allocated as part of the same array is undefined in C. To be safe, you're better off writing this as: if ((intptr_t)ptr <= (intptr_t)h->free_callbacks[i].ptr) or even storing intptr_t rather than void* in your list of pointers needing callbacks.
+ break; + }
This is a linear search O(n) for where to insert. Why not bsearch() for O(log n), particularly since you document your intent to use bsearch() for later lookups?
+ memmove (&h->free_callbacks[i+1], &h->free_callbacks[i], + (h->nr_free_callbacks - i) * sizeof (struct free_callback));
Hmm - the use of memmove() is O(n); we'd need a different data structure (for example red-black tree or hash table) if we wanted list manipulations to not be the chokepoint. So, as long as you are already stuck with an O(n) list manipulation, using O(n) lookup is not making the problem any worse.
+ + h->free_callbacks[i].ptr = ptr; + h->free_callbacks[i].cb = cb; + h->free_callbacks[i].user_data = user_data; + h->nr_free_callbacks++; + + ret = 0; + out: + pthread_mutex_unlock (&h->lock); + return ret; +} + +static int +compare_free_callbacks (const void *v1, const void *v2) +{ + const void *ptr = v1; + const struct free_callback *cb = v2; + + if (ptr < cb->ptr) return -1; + else if (ptr > cb->ptr) return 1;
Again, undefined in C; comparing intptr_t is defined, but direct comparison of unrelated pointers could cause a compiler to mis-optimize (even if it happens to currently work in practice).
+ else return 0; +} + +/* Called before the library frees 'ptr', where 'ptr' is something + * that might potentially be associated with a free callback. This is + * called often so must be fast. + */ +void +nbd_internal_free_callback (struct nbd_handle *h, void *ptr) +{ + struct free_callback *free_cb; + + if (ptr == NULL) + return; + + free_cb = bsearch (ptr, h->free_callbacks, h->nr_free_callbacks, + sizeof (struct free_callback), + compare_free_callbacks);
Here, you've got O(log n) lookup.
+ if (free_cb) { + assert (ptr == free_cb->ptr); + + free_cb->cb (ptr, free_cb->user_data); + + /* Remove it from the free list. */ + memmove (free_cb, free_cb+1, + sizeof (struct free_callback) * + (&h->free_callbacks[h->nr_free_callbacks] - (free_cb+1)));
but then slow it down with O(n) list manipulation. I'm still not sold on whether this is any better than our existing completion callbacks coupled with VALID|FREE (where at least we didn't suffer from O(n) lookups); or whether we want to instead explore taking an explicit free function pointer as part of a C closure (that is, each Closure maps to a three-tuple of C arguments for main function, free function, and user_data). But as you said in the cover letter, having the whole series to review will let us choose between our options, so I'll see what the rest of the series is able to accomplish with this. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Mon, Aug 12, 2019 at 11:29:10AM -0500, Eric Blake wrote:
On 8/12/19 11:08 AM, Richard W.M. Jones wrote:
+ typedef void (*nbd_free_callback) (void *ptr, void *user_data); + int nbd_add_free_callback (struct nbd_handle *h, + void *ptr, + nbd_free_callback cb, + void *user_data);
Do we want to insist on a user_data argument? Libvirt, for example, states that free callbacks are passed only the pointer to be freed (as you can already pack whatever you need into that pointer alone, rather than needing yet another void *user_data); if we do not take a user_data here, then...
Indeed, and that was my original implementation, and it would be usable in the simple case (free). However it doesn't work for OCaml buffers (nor for Python). In OCaml we need to free the GC root, but that is separate from the buffer pointer. The generated code for an OCaml persistent buffers eventually looks like this: /* The function may save a reference to the Buffer, so we * must treat it as a possible GC root. */ value *buf_user_data; buf_user_data = malloc (sizeof (value)); if (buf_user_data == NULL) caml_raise_out_of_memory (); *buf_user_data = bufv; caml_register_generational_global_root (buf_user_data); struct nbd_buffer *buf_buf = NBD_buffer_val (bufv); const void *buf = buf_buf->data; size_t count = buf_buf->len; if (nbd_add_free_callback (h, (void *)buf, free_root, buf_user_data) == -1) caml_raise_out_of_memory (); Notice that the free_root function needs the pointer to the GC root, but you can't get to that from the buffer pointer. A similar problem will exist in Python, but I've not got around to writing that code yet.
+C<ptr> is a pointer to the object (ie. the buffer or +callback C<user_data>). C<cb (ptr)> is called when libnbd
As written, your patch uses C<cb (ptr, user_data)>, so either this text is wrong, or you really don't need user_data.
Yes it should be cb (ptr, user_data). I'll tighten it up because there are now two user_datas.
+ new_callbacks = realloc (h->free_callbacks, + sizeof (struct free_callback) * new_alloc);
Should we start relying on reallocarray() to guarantee no multiplication overflow? (Not yet in POSIX, but it has been proposed: http://austingroupbugs.net/view.php?id=1218)
I guess - didn't know about it. Seems like it's not available in glibc yet?
+ if (new_callbacks == NULL) { + set_error (errno, "realloc"); + goto out; + } + h->alloc_free_callbacks = new_alloc; + h->free_callbacks = new_callbacks; + } + + /* Need to keep the list sorted by pointer so we can use bsearch. + * Insert the new entry before the i'th entry. + * + * Note the same pointer can be added multiple times (eg. if a + * buffer is shared between commands), and that is not a bug. Which + * free function is called is undefined, but they should all be + * called eventually.
or in other words, if the free function is actually a reference counter that only does something interesting when the count reaches 0, then things still work.
Yes this is another reason why this API is trickier to use than it seems. But this is really necessary if you share a buffer between several AIO calls as I discovered during implementation.
+ */ + for (i = 0; i < h->nr_free_callbacks; ++i) { + if (ptr <= h->free_callbacks[i].ptr)
Comparing 2 pointers that are not allocated as part of the same array is undefined in C. To be safe, you're better off writing this as:
if ((intptr_t)ptr <= (intptr_t)h->free_callbacks[i].ptr)
or even storing intptr_t rather than void* in your list of pointers needing callbacks.
OK.
+ break; + }
This is a linear search O(n) for where to insert. Why not bsearch() for O(log n), particularly since you document your intent to use bsearch() for later lookups?
Yes indeed, good idea!
+ memmove (&h->free_callbacks[i+1], &h->free_callbacks[i], + (h->nr_free_callbacks - i) * sizeof (struct free_callback));
Hmm - the use of memmove() is O(n); we'd need a different data structure (for example red-black tree or hash table) if we wanted list manipulations to not be the chokepoint. So, as long as you are already stuck with an O(n) list manipulation, using O(n) lookup is not making the problem any worse.
h->nr_free_callbacks is actually reasonably small in real code (usually <= 20). It's somewhat related to the number of commands in flight multiplied by a small constant.
+ + h->free_callbacks[i].ptr = ptr; + h->free_callbacks[i].cb = cb; + h->free_callbacks[i].user_data = user_data; + h->nr_free_callbacks++; + + ret = 0; + out: + pthread_mutex_unlock (&h->lock); + return ret; +} + +static int +compare_free_callbacks (const void *v1, const void *v2) +{ + const void *ptr = v1; + const struct free_callback *cb = v2; + + if (ptr < cb->ptr) return -1; + else if (ptr > cb->ptr) return 1;
Again, undefined in C; comparing intptr_t is defined, but direct comparison of unrelated pointers could cause a compiler to mis-optimize (even if it happens to currently work in practice).
OK. We could store the pointers in the struct as intptr_t ?
+ else return 0; +} + +/* Called before the library frees 'ptr', where 'ptr' is something + * that might potentially be associated with a free callback. This is + * called often so must be fast. + */ +void +nbd_internal_free_callback (struct nbd_handle *h, void *ptr) +{ + struct free_callback *free_cb; + + if (ptr == NULL) + return; + + free_cb = bsearch (ptr, h->free_callbacks, h->nr_free_callbacks, + sizeof (struct free_callback), + compare_free_callbacks);
Here, you've got O(log n) lookup.
+ if (free_cb) { + assert (ptr == free_cb->ptr); + + free_cb->cb (ptr, free_cb->user_data); + + /* Remove it from the free list. */ + memmove (free_cb, free_cb+1, + sizeof (struct free_callback) * + (&h->free_callbacks[h->nr_free_callbacks] - (free_cb+1)));
but then slow it down with O(n) list manipulation.
Right, but only when we actually call the callback. For C callers we will basically never use this. h->nr_free_callbacks will be 0 and we will do a frequent bsearch on a zero-sized array. It's only expected to be used from other programming languages.
I'm still not sold on whether this is any better than our existing completion callbacks coupled with VALID|FREE (where at least we didn't suffer from O(n) lookups); or whether we want to instead explore taking an explicit free function pointer as part of a C closure (that is, each Closure maps to a three-tuple of C arguments for main function, free function, and user_data). But as you said in the cover letter, having the whole series to review will let us choose between our options, so I'll see what the rest of the series is able to accomplish with this.
I think we shouldn't worry about whether the implementation is efficient since we can always improve that based on feedback from perf. It's really about whether this is a better API for freeing or not. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On 8/12/19 1:13 PM, Richard W.M. Jones wrote:
On Mon, Aug 12, 2019 at 11:29:10AM -0500, Eric Blake wrote:
On 8/12/19 11:08 AM, Richard W.M. Jones wrote:
+ typedef void (*nbd_free_callback) (void *ptr, void *user_data); + int nbd_add_free_callback (struct nbd_handle *h, + void *ptr, + nbd_free_callback cb, + void *user_data);
Do we want to insist on a user_data argument? Libvirt, for example, states that free callbacks are passed only the pointer to be freed (as you can already pack whatever you need into that pointer alone, rather than needing yet another void *user_data); if we do not take a user_data here, then...
Indeed, and that was my original implementation, and it would be usable in the simple case (free).
However it doesn't work for OCaml buffers (nor for Python). In OCaml we need to free the GC root, but that is separate from the buffer pointer.
The generated code for an OCaml persistent buffers eventually looks like this:
/* The function may save a reference to the Buffer, so we * must treat it as a possible GC root. */ value *buf_user_data; buf_user_data = malloc (sizeof (value)); if (buf_user_data == NULL) caml_raise_out_of_memory (); *buf_user_data = bufv; caml_register_generational_global_root (buf_user_data); struct nbd_buffer *buf_buf = NBD_buffer_val (bufv); const void *buf = buf_buf->data; size_t count = buf_buf->len; if (nbd_add_free_callback (h, (void *)buf, free_root, buf_user_data) == -1) caml_raise_out_of_memory ();
Notice that the free_root function needs the pointer to the GC root, but you can't get to that from the buffer pointer.
But still, can't we create a single C struct that tracks both the buffer and the GC root, and then pass the address of that C struct as the one pointer to the free function? In other words, I'm trying to get to the point of something like: struct closure_wrap { /* track anything for buffer GC root and refcounting */ value *buf; /* track anything for the OCaml callback, if one was present */ value *callback; }; struct closure_wrap *wrap = malloc (sizeof *wrap); wrap->buf = malloc (sizeof (*wrap->buf)); caml_register_generational_global_root (wrap->buf); struct nbd_buffer *buf_buf = NBD_buffer_val (bufv); const void *buf = buf_buf->data; size_t count = buf_buf->len; if (in nbd_aio_pread) { wrap->callback = NULL; res = nbd_aio_pread_callback(h, buf, count, offset, NULL, free_cb, wrap, flags); } else /* in nbd_aio_pread_callback */ { wrap->callback = ...; res = nbd_aio_pread_callback(h, buf, count, offset, completion_cb, free_cb, wrap, flags); } where completion_cb(wrap) calls the OCaml callback (and doesn't get used if there is no OCaml callback), and where free_cb(wrap) gets called at the end of the lifetime for buf, wrap->callback (if any), AND wrap itself, so that we have: void free_cb(void *ptr) { struct closure_wrap *wrap = ptr; caml_remove_generational_global_root (wrap->buf); if (wrap->callback) { unregister the OCaml callback... } free (wrap); }
A similar problem will exist in Python, but I've not got around to writing that code yet.
+C<ptr> is a pointer to the object (ie. the buffer or +callback C<user_data>). C<cb (ptr)> is called when libnbd
As written, your patch uses C<cb (ptr, user_data)>, so either this text is wrong, or you really don't need user_data.
Yes it should be cb (ptr, user_data). I'll tighten it up because there are now two user_datas.
But again, we may be able to get by with just one pointer. (I'm still hoping the idea of a three-tuple in C: main callback, free callback, and user data - is sufficient to give language bindings the flexibility they need to pack up a C struct containing everything needing cleanup at the end of the lifetime, whether or not there is also a language callback in play)
+ new_callbacks = realloc (h->free_callbacks, + sizeof (struct free_callback) * new_alloc);
Should we start relying on reallocarray() to guarantee no multiplication overflow? (Not yet in POSIX, but it has been proposed: http://austingroupbugs.net/view.php?id=1218)
I guess - didn't know about it. Seems like it's not available in glibc yet?
'man realloc' on Fedora 29 lists reallocarray under _GNU_SOURCE, but fails to say which glibc introduced it, and 'man reallocarray' is missing (bug in the man pages).
+ + /* Need to keep the list sorted by pointer so we can use bsearch. + * Insert the new entry before the i'th entry. + * + * Note the same pointer can be added multiple times (eg. if a + * buffer is shared between commands), and that is not a bug. Which + * free function is called is undefined, but they should all be + * called eventually.
or in other words, if the free function is actually a reference counter that only does something interesting when the count reaches 0, then things still work.
Yes this is another reason why this API is trickier to use than it seems. But this is really necessary if you share a buffer between several AIO calls as I discovered during implementation.
Normally, a user doesn't share the same buffer on the same handle - but we do test that for our speed tests (where we don't care about buffer contents so much as how many API calls can we fire off, so having cross-contamination between API calls into the same buffer in those tests is okay). But you are also right that it is going to be common to read into a buffer from one handle, then write that buffer out to another handle - so our actions on capturing the buffer during aio_pread, and then releasing it when retiring that command, really are centered on ref-counting whatever the language handed us, and only free()ing the actual C struct we use to tie everything together under a single pointer.
+ */ + for (i = 0; i < h->nr_free_callbacks; ++i) { + if (ptr <= h->free_callbacks[i].ptr)
Comparing 2 pointers that are not allocated as part of the same array is undefined in C. To be safe, you're better off writing this as:
if ((intptr_t)ptr <= (intptr_t)h->free_callbacks[i].ptr)
or even storing intptr_t rather than void* in your list of pointers needing callbacks.
OK.
And if I'm right about my idea of having each Closure become a 3-tuple in C, where we can use a common free_cb(ptr) at the time the command is retired for both the language nbd.aio_pread and for nbd.aio_pread_callback, then pointer comparisons is moot because we don't need to track a list of pointers, but rather just a free_cb alongside each user_data.
+ break; + }
This is a linear search O(n) for where to insert. Why not bsearch() for O(log n), particularly since you document your intent to use bsearch() for later lookups?
Yes indeed, good idea!
+ memmove (&h->free_callbacks[i+1], &h->free_callbacks[i], + (h->nr_free_callbacks - i) * sizeof (struct free_callback));
Hmm - the use of memmove() is O(n); we'd need a different data structure (for example red-black tree or hash table) if we wanted list manipulations to not be the chokepoint. So, as long as you are already stuck with an O(n) list manipulation, using O(n) lookup is not making the problem any worse.
h->nr_free_callbacks is actually reasonably small in real code (usually <= 20). It's somewhat related to the number of commands in flight multiplied by a small constant.
And my argument is that we are not in control of that number - a user can write a Python script that deliberately calls nbd.aio_pread 1000 times before ever calling nbd.poll to finally advance the state machine.
+static int +compare_free_callbacks (const void *v1, const void *v2) +{ + const void *ptr = v1; + const struct free_callback *cb = v2; + + if (ptr < cb->ptr) return -1; + else if (ptr > cb->ptr) return 1;
Again, undefined in C; comparing intptr_t is defined, but direct comparison of unrelated pointers could cause a compiler to mis-optimize (even if it happens to currently work in practice).
OK. We could store the pointers in the struct as intptr_t ?
Yes, if we have to compare them at all. But I'm hoping we don't have to.
+ free_cb = bsearch (ptr, h->free_callbacks, h->nr_free_callbacks, + sizeof (struct free_callback), + compare_free_callbacks);
Here, you've got O(log n) lookup.
+ if (free_cb) { + assert (ptr == free_cb->ptr); + + free_cb->cb (ptr, free_cb->user_data); + + /* Remove it from the free list. */ + memmove (free_cb, free_cb+1, + sizeof (struct free_callback) * + (&h->free_callbacks[h->nr_free_callbacks] - (free_cb+1)));
but then slow it down with O(n) list manipulation.
Right, but only when we actually call the callback.
For C callers we will basically never use this. h->nr_free_callbacks will be 0 and we will do a frequent bsearch on a zero-sized array. It's only expected to be used from other programming languages.
Okay, so the effect to C is minimal, but the effect to other languages may be noticeable (since there we will be registering a callback for every aio call).
I'm still not sold on whether this is any better than our existing completion callbacks coupled with VALID|FREE (where at least we didn't suffer from O(n) lookups); or whether we want to instead explore taking an explicit free function pointer as part of a C closure (that is, each Closure maps to a three-tuple of C arguments for main function, free function, and user_data). But as you said in the cover letter, having the whole series to review will let us choose between our options, so I'll see what the rest of the series is able to accomplish with this.
I think we shouldn't worry about whether the implementation is efficient since we can always improve that based on feedback from perf. It's really about whether this is a better API for freeing or not.
Getting rid of the VALID|FREE seems worthwhile, and _since_ that is already an API/ABI change, we can _also_ make the API/ABI change from Closure in C being a 2-tuple (fn, ptr) to being a 3-tuple (fn, free_fn, ptr). The tricky part that remains is how to convince the generator that the code for Python nbd.aio_pread AND for nbd.aio_pread_callback both need to use the C nbd_aio_pread_callback, for the sake of the completion's free_cb() to do the necessary refcount cleanup on the buf, at which point we no longer need a separate registration of free callbacks. We then have the design question of whether to make an OClosure type, where C has two functions nbd_aio_pread and nbd_aio_pread_callback for convenience, but where other languages have only a single nbd.aio_pread where the callback parameter is optional (the Closure type for pread_structured chunk and for block_status extent will still be mandatory; it is only the completion callback that is currently causing us twice the API because we are treating it as pseudo-optional). Or maybe we just require C clients of nbd_aio_pread to always provide parameters for callbacks, but document that the completion callback and free callback pointers may be NULL. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Mon, Aug 12, 2019 at 01:53:56PM -0500, Eric Blake wrote:
We then have the design question of whether to make an OClosure type, where C has two functions nbd_aio_pread and nbd_aio_pread_callback for convenience, but where other languages have only a single nbd.aio_pread where the callback parameter is optional (the Closure type for pread_structured chunk and for block_status extent will still be mandatory; it is only the completion callback that is currently causing us twice the API because we are treating it as pseudo-optional). Or maybe we just require C clients of nbd_aio_pread to always provide parameters for callbacks, but document that the completion callback and free callback pointers may be NULL.
I'll try to see what OClosure (and then getting rid of the _callback variants, adding to the non-callback variants OClosure) will look like tomorrow. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
On Mon, Aug 12, 2019 at 11:00:01PM +0100, Richard W.M. Jones wrote:
On Mon, Aug 12, 2019 at 01:53:56PM -0500, Eric Blake wrote:
We then have the design question of whether to make an OClosure type, where C has two functions nbd_aio_pread and nbd_aio_pread_callback for convenience, but where other languages have only a single nbd.aio_pread where the callback parameter is optional (the Closure type for pread_structured chunk and for block_status extent will still be mandatory; it is only the completion callback that is currently causing us twice the API because we are treating it as pseudo-optional). Or maybe we just require C clients of nbd_aio_pread to always provide parameters for callbacks, but document that the completion callback and free callback pointers may be NULL.
I'll try to see what OClosure (and then getting rid of the _callback variants, adding to the non-callback variants OClosure) will look like tomorrow.
I should say that while OClosure has the possibility of solving the freeing problem for buffers and closures attached to AIO commands, and may also be a good idea to get rid of the *_callback variants, it doesn't solve the problem in general. In particular there's no way to free the debug function closure, which means it would be leaked in language bindings. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On 8/13/19 2:33 AM, Richard W.M. Jones wrote:
On Mon, Aug 12, 2019 at 11:00:01PM +0100, Richard W.M. Jones wrote:
On Mon, Aug 12, 2019 at 01:53:56PM -0500, Eric Blake wrote:
We then have the design question of whether to make an OClosure type, where C has two functions nbd_aio_pread and nbd_aio_pread_callback for convenience, but where other languages have only a single nbd.aio_pread where the callback parameter is optional (the Closure type for pread_structured chunk and for block_status extent will still be mandatory; it is only the completion callback that is currently causing us twice the API because we are treating it as pseudo-optional). Or maybe we just require C clients of nbd_aio_pread to always provide parameters for callbacks, but document that the completion callback and free callback pointers may be NULL.
I'll try to see what OClosure (and then getting rid of the _callback variants, adding to the non-callback variants OClosure) will look like tomorrow.
I should say that while OClosure has the possibility of solving the freeing problem for buffers and closures attached to AIO commands, and may also be a good idea to get rid of the *_callback variants, it doesn't solve the problem in general.
In particular there's no way to free the debug function closure, which means it would be leaked in language bindings.
Not if we keep our current 2-tuple per Closure/OClosure. But the moment we switch the C bindings to have a 3-tuple per Closure, the debug function becomes: nbd_set_debug_callback (h, debug_cb, free_cb, user_data); with libnbd calling debug_cb(user_data) as often as desired, and free_cb(user_data) exactly once when installing a different callback or reaching nbd_close(). The language bindings then increment the refcount on the language callback and pack it into a malloc'd C struct during nbd_set_debug_callback; then debug_cb(user_data) grabs the language object out of the C struct for calling back into the language, and free_cb(user_data) decrements the refcount on the language object and then free()s the C struct. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
When retiring a command test for a free_callback associated with their buffer. If there is one call it. This allows language bindings to use this mechanism to automatically decrement a reference to the persistent buffer (note: this patch does not implement this). The vast majority of this change is simply passing around the handle so we have it when we call nbd_internal_free_callback in lib/aio.c. --- generator/states-reply.c | 2 +- generator/states.c | 2 +- lib/aio.c | 10 ++++++++-- lib/handle.c | 10 +++++----- lib/internal.h | 3 ++- 5 files changed, 17 insertions(+), 10 deletions(-) diff --git a/generator/states-reply.c b/generator/states-reply.c index 09adfed..389317e 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -194,7 +194,7 @@ save_reply_state (struct nbd_handle *h) h->cmds_in_flight = cmd->next; cmd->next = NULL; if (retire) - nbd_internal_retire_and_free_command (cmd); + nbd_internal_retire_and_free_command (h, cmd); else { if (h->cmds_done_tail != NULL) h->cmds_done_tail = h->cmds_done_tail->next = cmd; diff --git a/generator/states.c b/generator/states.c index 9ed57ae..a11c1d1 100644 --- a/generator/states.c +++ b/generator/states.c @@ -142,7 +142,7 @@ void abort_commands (struct nbd_handle *h, if (cmd->error == 0) cmd->error = ENOTCONN; if (retire) - nbd_internal_retire_and_free_command (cmd); + nbd_internal_retire_and_free_command (h, cmd); else { cmd->next = NULL; if (h->cmds_done_tail) diff --git a/lib/aio.c b/lib/aio.c index c141de6..1d26be9 100644 --- a/lib/aio.c +++ b/lib/aio.c @@ -29,7 +29,8 @@ /* Internal function which retires and frees a command. */ void -nbd_internal_retire_and_free_command (struct command *cmd) +nbd_internal_retire_and_free_command (struct nbd_handle *h, + struct command *cmd) { /* Free the callbacks. */ if (cmd->type == NBD_CMD_BLOCK_STATUS && cmd->cb.fn.extent) @@ -42,6 +43,11 @@ nbd_internal_retire_and_free_command (struct command *cmd) cmd->cb.completion (LIBNBD_CALLBACK_FREE, cmd->cb.user_data, NULL); + /* Free the persistent buffer if there is one and if there's an + * associated free callback. + */ + nbd_internal_free_callback (h, cmd->data); + free (cmd); } @@ -109,7 +115,7 @@ nbd_unlocked_aio_command_completed (struct nbd_handle *h, else h->cmds_done = cmd->next; - nbd_internal_retire_and_free_command (cmd); + nbd_internal_retire_and_free_command (h, cmd); /* If the command was successful, return true. */ if (error == 0) diff --git a/lib/handle.c b/lib/handle.c index ae0d196..0f50e38 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -32,13 +32,13 @@ #include "internal.h" static void -free_cmd_list (struct command *list) +free_cmd_list (struct nbd_handle *h, struct command *list) { struct command *cmd, *cmd_next; for (cmd = list; cmd != NULL; cmd = cmd_next) { cmd_next = cmd->next; - nbd_internal_retire_and_free_command (cmd); + nbd_internal_retire_and_free_command (h, cmd); } } @@ -123,9 +123,9 @@ nbd_close (struct nbd_handle *h) free (m->name); free (m); } - free_cmd_list (h->cmds_to_issue); - free_cmd_list (h->cmds_in_flight); - free_cmd_list (h->cmds_done); + free_cmd_list (h, h->cmds_to_issue); + free_cmd_list (h, h->cmds_in_flight); + free_cmd_list (h, h->cmds_done); /* Any remaining free callbacks indicate an error. */ if (h->nr_free_callbacks != 0) diff --git a/lib/internal.h b/lib/internal.h index d8b0eed..42e6921 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -288,7 +288,8 @@ struct command { }; /* aio.c */ -extern void nbd_internal_retire_and_free_command (struct command *); +extern void nbd_internal_retire_and_free_command (struct nbd_handle *, + struct command *); /* crypto.c */ extern struct socket *nbd_internal_crypto_create_session (struct nbd_handle *, struct socket *oldsock); -- 2.22.0
On 8/12/19 11:08 AM, Richard W.M. Jones wrote:
When retiring a command test for a free_callback associated with their buffer. If there is one call it. This allows language bindings to use this mechanism to automatically decrement a reference to the persistent buffer (note: this patch does not implement this).
The vast majority of this change is simply passing around the handle so we have it when we call nbd_internal_free_callback in lib/aio.c. ---
+++ b/lib/aio.c @@ -29,7 +29,8 @@
/* Internal function which retires and frees a command. */ void -nbd_internal_retire_and_free_command (struct command *cmd) +nbd_internal_retire_and_free_command (struct nbd_handle *h, + struct command *cmd) { /* Free the callbacks. */ if (cmd->type == NBD_CMD_BLOCK_STATUS && cmd->cb.fn.extent) @@ -42,6 +43,11 @@ nbd_internal_retire_and_free_command (struct command *cmd) cmd->cb.completion (LIBNBD_CALLBACK_FREE, cmd->cb.user_data, NULL);
+ /* Free the persistent buffer if there is one and if there's an + * associated free callback. + */ + nbd_internal_free_callback (h, cmd->data); +
Looks reasonable. However, I'm also wondering if the later patches to adjust the language bindings can instead guarantee a C struct which includes both the buffer (which needs decrementing) and the language-specific callback (or NULL), where the language binding for both nbd.aio_pread and nbd.aio_pread_complete call the underlying C nbd_aio_pread_complete, so we don't need to manage an O(n) list of pointers with associated callbacks. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
By using the free callback mechanism we don't need to manually free the buffer. This reverts part of commit fef1c281a65d061127bf178e5f8cfca0a2475c56. --- generator/generator | 43 +++++++++++++++++++++----------- ocaml/buffer.c | 22 ---------------- ocaml/examples/asynch_copy.ml | 4 +-- ocaml/libnbd-ocaml.pod | 22 ---------------- ocaml/nbd-c.h | 8 ++++++ ocaml/tests/test_590_aio_copy.ml | 4 +-- 6 files changed, 39 insertions(+), 64 deletions(-) diff --git a/generator/generator b/generator/generator index d6b9352..92ce170 100755 --- a/generator/generator +++ b/generator/generator @@ -4961,10 +4961,6 @@ module Buffer : sig (** Allocate an uninitialized buffer. The parameter is the size in bytes. *) - val free : t -> unit - (** The buffer must be manually freed when it is no longer used. - An AIO completion callback is usually a good place. *) - val to_bytes : t -> bytes (** Copy buffer to an OCaml [bytes] object. *) @@ -5062,7 +5058,6 @@ let () = module Buffer = struct type t external alloc : int -> t = \"nbd_internal_ocaml_buffer_alloc\" - external free : t -> unit = \"nbd_internal_ocaml_buffer_free\" external to_bytes : t -> bytes = \"nbd_internal_ocaml_buffer_to_bytes\" external of_bytes : bytes -> t = \"nbd_internal_ocaml_buffer_of_bytes\" external size : t -> int = \"nbd_internal_ocaml_buffer_size\" @@ -5244,10 +5239,8 @@ let print_ocaml_binding (name, { args; optargs; ret }) = pr " caml_enter_blocking_section ();\n"; pr " }\n"; pr "\n"; - pr " if (valid_flag & LIBNBD_CALLBACK_FREE) {\n"; - pr " caml_remove_generational_global_root ((value *)user_data);\n"; - pr " free (user_data);\n"; - pr " }\n"; + pr " if (valid_flag & LIBNBD_CALLBACK_FREE)\n"; + pr " free_root (NULL, user_data);\n"; pr "\n"; pr " return ret;\n"; pr "}\n"; @@ -5313,17 +5306,39 @@ let print_ocaml_binding (name, { args; optargs; ret }) = | BytesIn (n, count) -> pr " const void *%s = Bytes_val (%sv);\n" n n; pr " size_t %s = caml_string_length (%sv);\n" count n + | BytesOut (n, count) -> + pr " void *%s = Bytes_val (%sv);\n" n n; + pr " size_t %s = caml_string_length (%sv);\n" count n | BytesPersistIn (n, count) -> + pr " /* The function may save a reference to the Buffer, so we\n"; + pr " * must treat it as a possible GC root.\n"; + pr " */\n"; + pr " value *%s_user_data;\n" n; + pr " %s_user_data = malloc (sizeof (value));\n" n; + pr " if (%s_user_data == NULL) caml_raise_out_of_memory ();\n" n; + pr " *%s_user_data = %sv;\n" n n; + pr " caml_register_generational_global_root (%s_user_data);\n" n; pr " struct nbd_buffer *%s_buf = NBD_buffer_val (%sv);\n" n n; pr " const void *%s = %s_buf->data;\n" n n; - pr " size_t %s = %s_buf->len;\n" count n - | BytesOut (n, count) -> - pr " void *%s = Bytes_val (%sv);\n" n n; - pr " size_t %s = caml_string_length (%sv);\n" count n + pr " size_t %s = %s_buf->len;\n" count n; + pr " if (nbd_add_free_callback (h, (void *)%s,\n" n; + pr " free_root, %s_user_data) == -1)\n" n; + pr " caml_raise_out_of_memory ();\n" | BytesPersistOut (n, count) -> + pr " /* The function may save a reference to the Buffer, so we\n"; + pr " * must treat it as a possible GC root.\n"; + pr " */\n"; + pr " value *%s_user_data;\n" n; + pr " %s_user_data = malloc (sizeof (value));\n" n; + pr " if (%s_user_data == NULL) caml_raise_out_of_memory ();\n" n; + pr " *%s_user_data = %sv;\n" n n; + pr " caml_register_generational_global_root (%s_user_data);\n" n; pr " struct nbd_buffer *%s_buf = NBD_buffer_val (%sv);\n" n n; pr " void *%s = %s_buf->data;\n" n n; - pr " size_t %s = %s_buf->len;\n" count n + pr " size_t %s = %s_buf->len;\n" count n; + pr " if (nbd_add_free_callback (h, %s,\n" n; + pr " free_root, %s_user_data) == -1)\n" n; + pr " caml_raise_out_of_memory ();\n" | Closure { cbname } -> pr " /* The function may save a reference to the closure, so we\n"; pr " * must treat it as a possible GC root.\n"; diff --git a/ocaml/buffer.c b/ocaml/buffer.c index 837eece..0c0fe00 100644 --- a/ocaml/buffer.c +++ b/ocaml/buffer.c @@ -36,10 +36,6 @@ nbd_buffer_finalize (value bv) { struct nbd_buffer *b = NBD_buffer_val (bv); - /* Usually if this frees the buffer it indicates a bug in the - * program. The buffer should have been manually freed before - * this. But there's nothing we can do here, so free it. - */ free (b->data); } @@ -60,21 +56,6 @@ nbd_internal_ocaml_buffer_alloc (value sizev) CAMLreturn (rv); } -/* Free an NBD persistent buffer (only the C buffer, not the - * OCaml object). - */ -value -nbd_internal_ocaml_buffer_free (value bv) -{ - CAMLparam1 (bv); - struct nbd_buffer *b = NBD_buffer_val (bv); - - free (b->data); - b->data = NULL; - - CAMLreturn (Val_unit); -} - /* Copy an NBD persistent buffer to an OCaml bytes. */ value nbd_internal_ocaml_buffer_to_bytes (value bv) @@ -83,9 +64,6 @@ nbd_internal_ocaml_buffer_to_bytes (value bv) CAMLlocal1 (rv); struct nbd_buffer *b = NBD_buffer_val (bv); - if (b->data == NULL) /* Buffer has been freed. */ - caml_invalid_argument ("NBD.Buffer.to_bytes used on freed buffer"); - rv = caml_alloc_string (b->len); memcpy (Bytes_val (rv), b->data, b->len); diff --git a/ocaml/examples/asynch_copy.ml b/ocaml/examples/asynch_copy.ml index 7dbe976..50357f9 100644 --- a/ocaml/examples/asynch_copy.ml +++ b/ocaml/examples/asynch_copy.ml @@ -31,11 +31,9 @@ let asynch_copy src dst = in (* This callback is called when any pwrite to the destination - * has completed. We have to manually free the buffer here - * as it is no longer used. + * has completed. *) let write_completed buf _ = - NBD.Buffer.free buf; (* By returning 1 here we auto-retire the pwrite command. *) 1 in diff --git a/ocaml/libnbd-ocaml.pod b/ocaml/libnbd-ocaml.pod index f5f968c..5e161c6 100644 --- a/ocaml/libnbd-ocaml.pod +++ b/ocaml/libnbd-ocaml.pod @@ -34,28 +34,6 @@ which is the printable error message. The second is the raw C<errno>, if available (see C<nbd_get_errno>). The raw C<errno> is not compatible with errors in the OCaml C<Unix> module unfortunately. -=head1 AIO BUFFERS - -Some libnbd asynchronous I/O (AIO) calls require a buffer parameter -which persists after the call finishes. For example C<NBD.aio_pread> -starts a command which continues reading into the C<NBD.Buffer.t> -parameter in subsequent calls after the original C<aio_pread> call -returns. - -For these cases you must create a C<NBD.Buffer.t> and ensure that it -is not garbage collected until the command completes. The easiest way -to do this is to use the C<*_callback> variants and free the buffer in -the callback: - - let buf = NBD.Buffer.alloc 512 in - NBD.aio_pread_callback h buf 0_L ( - (* This is called when the command has completed. *) - fun _ -> - NBD.Buffer.free buf; - (* Returning 1 from this callback auto-retires the command. *) - 1 - ) - =head1 EXAMPLES This directory contains examples written in OCaml: diff --git a/ocaml/nbd-c.h b/ocaml/nbd-c.h index ffd51d2..a84d64e 100644 --- a/ocaml/nbd-c.h +++ b/ocaml/nbd-c.h @@ -107,4 +107,12 @@ struct callback_data { extern char **nbd_internal_ocaml_string_list (value); extern value nbd_internal_ocaml_alloc_int32_array (uint32_t *, size_t); +/* Free a generational global root and its container. */ +static inline void +free_root (void *ptr /* unused */, void *rootp) +{ + caml_remove_generational_global_root ((value *)rootp); + free (rootp); +} + #endif /* LIBNBD_NBD_C_H */ diff --git a/ocaml/tests/test_590_aio_copy.ml b/ocaml/tests/test_590_aio_copy.ml index 290ca93..effc299 100644 --- a/ocaml/tests/test_590_aio_copy.ml +++ b/ocaml/tests/test_590_aio_copy.ml @@ -53,11 +53,9 @@ let asynch_copy src dst = in (* This callback is called when any pwrite to the destination - * has completed. We have to manually free the buffer here - * as it is no longer used. + * has completed. *) let write_completed buf _ = - NBD.Buffer.free buf; bytes_written := !bytes_written + NBD.Buffer.size buf; (* By returning 1 here we auto-retire the pwrite command. *) 1 -- 2.22.0
On 8/12/19 11:08 AM, Richard W.M. Jones wrote:
By using the free callback mechanism we don't need to manually free the buffer.
This reverts part of commit fef1c281a65d061127bf178e5f8cfca0a2475c56. --- generator/generator | 43 +++++++++++++++++++++----------- ocaml/buffer.c | 22 ---------------- ocaml/examples/asynch_copy.ml | 4 +-- ocaml/libnbd-ocaml.pod | 22 ---------------- ocaml/nbd-c.h | 8 ++++++ ocaml/tests/test_590_aio_copy.ml | 4 +-- 6 files changed, 39 insertions(+), 64 deletions(-)
diff --git a/generator/generator b/generator/generator index d6b9352..92ce170 100755 --- a/generator/generator +++ b/generator/generator @@ -4961,10 +4961,6 @@ module Buffer : sig (** Allocate an uninitialized buffer. The parameter is the size in bytes. *)
- val free : t -> unit - (** The buffer must be manually freed when it is no longer used. - An AIO completion callback is usually a good place. *) -
Getting rid of this public function is nice. Hopefully, how we get rid of it is internal enough that we can switch between either a list of free functions at the C level, or else a smarter forwarding of aio_pread to the C nbd_aio_pread_completion, without affecting what else leaks through to the OCaml view.
@@ -5244,10 +5239,8 @@ let print_ocaml_binding (name, { args; optargs; ret }) = pr " caml_enter_blocking_section ();\n"; pr " }\n"; pr "\n"; - pr " if (valid_flag & LIBNBD_CALLBACK_FREE) {\n"; - pr " caml_remove_generational_global_root ((value *)user_data);\n"; - pr " free (user_data);\n"; - pr " }\n"; + pr " if (valid_flag & LIBNBD_CALLBACK_FREE)\n"; + pr " free_root (NULL, user_data);\n"; pr "\n";
Here, I'm unfamiliar enough with the OCaml GC rules to know if you're doing it right, but trust that you have been testing with valgrind or similar with a forced GC run to catch obvious mistakes.
@@ -5313,17 +5306,39 @@ let print_ocaml_binding (name, { args; optargs; ret }) = | BytesIn (n, count) -> pr " const void *%s = Bytes_val (%sv);\n" n n; pr " size_t %s = caml_string_length (%sv);\n" count n + | BytesOut (n, count) -> + pr " void *%s = Bytes_val (%sv);\n" n n; + pr " size_t %s = caml_string_length (%sv);\n" count n | BytesPersistIn (n, count) -> + pr " /* The function may save a reference to the Buffer, so we\n"; + pr " * must treat it as a possible GC root.\n"; + pr " */\n"; + pr " value *%s_user_data;\n" n; + pr " %s_user_data = malloc (sizeof (value));\n" n; + pr " if (%s_user_data == NULL) caml_raise_out_of_memory ();\n" n; + pr " *%s_user_data = %sv;\n" n n; + pr " caml_register_generational_global_root (%s_user_data);\n" n; pr " struct nbd_buffer *%s_buf = NBD_buffer_val (%sv);\n" n n; pr " const void *%s = %s_buf->data;\n" n n; - pr " size_t %s = %s_buf->len;\n" count n - | BytesOut (n, count) -> - pr " void *%s = Bytes_val (%sv);\n" n n; - pr " size_t %s = caml_string_length (%sv);\n" count n + pr " size_t %s = %s_buf->len;\n" count n; + pr " if (nbd_add_free_callback (h, (void *)%s,\n" n;
Is the cast to void* necessary? I guess so - it looks like you are casting away const.
+ pr " free_root, %s_user_data) == -1)\n" n;
Hmm. In patch 1, I questioned if we need a separate user_data argument when registering a free callback. But here, you are keying off of the last use of one pointer (the buffer), but with a different pointer (the user_data wrapper) to be passed to the free function...
+++ b/ocaml/nbd-c.h @@ -107,4 +107,12 @@ struct callback_data { extern char **nbd_internal_ocaml_string_list (value); extern value nbd_internal_ocaml_alloc_int32_array (uint32_t *, size_t);
+/* Free a generational global root and its container. */ +static inline void +free_root (void *ptr /* unused */, void *rootp) +{ + caml_remove_generational_global_root ((value *)rootp); + free (rootp); +}
...and indeed, the free function is ignoring the first pointer and instead acting on the second. If we change the C signature to take only a single pointer, then you have to be a bit more creative on h ow to pass the rootp as the pointer to be adjusted when the buffer lifetime ends. So maybe this is the answer to my question in patch 1 as to why you needed two arguments. Or maybe it is all the more reason to try harder to get the generator to call nbd_aio_pread_callback even when the OCaml language did not pass a callback. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Mechanical change: Wherever we call any closure with the LIBNBD_CALLBACK_FREE function, we also call nbd_internal_free_callback with the closure's user_data. This allows calls to associate a free callback with any closure via its user_data pointer. --- generator/states-reply-simple.c | 1 + generator/states-reply-structured.c | 24 ++++++++++++++++++------ generator/states-reply.c | 1 + generator/states.c | 1 + lib/aio.c | 4 ++++ lib/debug.c | 4 +++- lib/handle.c | 4 +++- 7 files changed, 31 insertions(+), 8 deletions(-) diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index 9b249ab..9684bc4 100644 --- a/generator/states-reply-simple.c +++ b/generator/states-reply-simple.c @@ -69,6 +69,7 @@ cmd->data, cmd->count, cmd->offset, LIBNBD_READ_DATA, &error) == -1) cmd->error = error ? error : EPROTO; + nbd_internal_free_callback (h, cmd->cb.fn_user_data); cmd->cb.fn.chunk = NULL; /* because we've freed it */ } diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index cdd9f10..b016cd7 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -317,8 +317,10 @@ valid_flags (struct nbd_handle *h) 0, offset, LIBNBD_READ_ERROR, &scratch) == -1) if (cmd->error == 0) cmd->error = scratch; - if (valid & LIBNBD_CALLBACK_FREE) + if (valid & LIBNBD_CALLBACK_FREE) { cmd->cb.fn.chunk = NULL; /* because we've freed it */ + nbd_internal_free_callback (h, cmd->cb.fn_user_data); + } } } @@ -408,8 +410,10 @@ valid_flags (struct nbd_handle *h) LIBNBD_READ_DATA, &error) == -1) if (cmd->error == 0) cmd->error = error ? error : EPROTO; - if (valid & LIBNBD_CALLBACK_FREE) + if (valid & LIBNBD_CALLBACK_FREE) { cmd->cb.fn.chunk = NULL; /* because we've freed it */ + nbd_internal_free_callback (h, cmd->cb.fn_user_data); + } } SET_NEXT_STATE (%FINISH); @@ -473,8 +477,10 @@ valid_flags (struct nbd_handle *h) LIBNBD_READ_HOLE, &error) == -1) if (cmd->error == 0) cmd->error = error ? error : EPROTO; - if (valid & LIBNBD_CALLBACK_FREE) + if (valid & LIBNBD_CALLBACK_FREE) { cmd->cb.fn.chunk = NULL; /* because we've freed it */ + nbd_internal_free_callback (h, cmd->cb.fn_user_data); + } } SET_NEXT_STATE(%FINISH); @@ -527,8 +533,10 @@ valid_flags (struct nbd_handle *h) &h->bs_entries[1], (length-4) / 4, &error) == -1) if (cmd->error == 0) cmd->error = error ? error : EPROTO; - if (valid & LIBNBD_CALLBACK_FREE) + if (valid & LIBNBD_CALLBACK_FREE) { cmd->cb.fn.extent = NULL; /* because we've freed it */ + nbd_internal_free_callback (h, cmd->cb.fn_user_data); + } } else /* Emit a debug message, but ignore it. */ @@ -545,12 +553,16 @@ valid_flags (struct nbd_handle *h) flags = be16toh (h->sbuf.sr.structured_reply.flags); if (flags & NBD_REPLY_FLAG_DONE) { - if (cmd->type == NBD_CMD_BLOCK_STATUS && cmd->cb.fn.extent) + if (cmd->type == NBD_CMD_BLOCK_STATUS && cmd->cb.fn.extent) { cmd->cb.fn.extent (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data, NULL, 0, NULL, 0, NULL); - if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk) + nbd_internal_free_callback (h, cmd->cb.fn_user_data); + } + if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk) { cmd->cb.fn.chunk (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data, NULL, 0, 0, 0, NULL); + nbd_internal_free_callback (h, cmd->cb.fn_user_data); + } cmd->cb.fn.chunk = NULL; SET_NEXT_STATE (%^FINISH_COMMAND); } diff --git a/generator/states-reply.c b/generator/states-reply.c index 389317e..d5cba1a 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -175,6 +175,7 @@ save_reply_state (struct nbd_handle *h) assert (cmd->type != NBD_CMD_DISC); r = cmd->cb.completion (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, cmd->cb.user_data, &error); + nbd_internal_free_callback (h, cmd->cb.user_data); cmd->cb.completion = NULL; /* because we've freed it */ switch (r) { case -1: diff --git a/generator/states.c b/generator/states.c index a11c1d1..313d2c9 100644 --- a/generator/states.c +++ b/generator/states.c @@ -128,6 +128,7 @@ void abort_commands (struct nbd_handle *h, assert (cmd->type != NBD_CMD_DISC); r = cmd->cb.completion (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, cmd->cb.user_data, &error); + nbd_internal_free_callback (h, cmd->cb.user_data); cmd->cb.completion = NULL; /* because we've freed it */ switch (r) { case -1: diff --git a/lib/aio.c b/lib/aio.c index 1d26be9..8aa4547 100644 --- a/lib/aio.c +++ b/lib/aio.c @@ -43,6 +43,10 @@ nbd_internal_retire_and_free_command (struct nbd_handle *h, cmd->cb.completion (LIBNBD_CALLBACK_FREE, cmd->cb.user_data, NULL); + /* Free the closures if there's an associated free callback. */ + nbd_internal_free_callback (h, cmd->cb.fn_user_data); + nbd_internal_free_callback (h, cmd->cb.user_data); + /* Free the persistent buffer if there is one and if there's an * associated free callback. */ diff --git a/lib/debug.c b/lib/debug.c index ad4d9cb..34d4184 100644 --- a/lib/debug.c +++ b/lib/debug.c @@ -42,9 +42,11 @@ int nbd_unlocked_set_debug_callback (struct nbd_handle *h, nbd_debug_callback debug_callback, void *data) { - if (h->debug_callback) + if (h->debug_callback) { /* ignore return value */ h->debug_callback (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL); + nbd_internal_free_callback (h, h->debug_data); + } h->debug_callback = debug_callback; h->debug_data = data; return 0; diff --git a/lib/handle.c b/lib/handle.c index 0f50e38..5a47bd8 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -113,8 +113,10 @@ nbd_close (struct nbd_handle *h) return; /* Free user callbacks first. */ - if (h->debug_callback) + if (h->debug_callback) { h->debug_callback (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL); + nbd_internal_free_callback (h, h->debug_data); + } h->debug_callback = NULL; free (h->bs_entries); -- 2.22.0
On 8/12/19 11:08 AM, Richard W.M. Jones wrote:
Mechanical change: Wherever we call any closure with the LIBNBD_CALLBACK_FREE function, we also call nbd_internal_free_callback with the closure's user_data. This allows calls to associate a free callback with any closure via its user_data pointer. ---
+++ b/lib/aio.c @@ -43,6 +43,10 @@ nbd_internal_retire_and_free_command (struct nbd_handle *h, cmd->cb.completion (LIBNBD_CALLBACK_FREE, cmd->cb.user_data, NULL);
+ /* Free the closures if there's an associated free callback. */ + nbd_internal_free_callback (h, cmd->cb.fn_user_data);
This calls the fn_user_data callback too many times if it was already reached. Instead, you need: if (cmd->type == NBD_CMD_BLOCK_STATUS && cmd->cb.fn.extent) { cmd->cb.fn.extent (_FREE...); nbd_internal_free_callback (h, cmd->cb.fn_user_data); } else if (cmd->type == NBD_CMD_BLOCK_READ && cmd->cb.fn.chunk) { cmd->cb.fn.chunk (_FREE...); nbd_internal_free_callback (h, cmd->cb.fn_user_data); } -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Instead of using the valid_flag == FREE mechanism, use a free callback to free each closure root. --- generator/generator | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/generator/generator b/generator/generator index 92ce170..109fad6 100755 --- a/generator/generator +++ b/generator/generator @@ -5239,9 +5239,6 @@ let print_ocaml_binding (name, { args; optargs; ret }) = pr " caml_enter_blocking_section ();\n"; pr " }\n"; pr "\n"; - pr " if (valid_flag & LIBNBD_CALLBACK_FREE)\n"; - pr " free_root (NULL, user_data);\n"; - pr "\n"; pr " return ret;\n"; pr "}\n"; pr "\n" @@ -5348,7 +5345,11 @@ let print_ocaml_binding (name, { args; optargs; ret }) = pr " if (%s_user_data == NULL) caml_raise_out_of_memory ();\n" cbname; pr " *%s_user_data = %sv;\n" cbname cbname; pr " caml_register_generational_global_root (%s_user_data);\n" cbname; - pr " const void *%s_callback = %s_%s_wrapper;\n" cbname name cbname + pr " const void *%s_callback = %s_%s_wrapper;\n" cbname name cbname; + pr " if (nbd_add_free_callback (h, %s_user_data,\n" cbname; + pr " free_root, %s_user_data) == -1)\n" + cbname; + pr " caml_raise_out_of_memory ();\n" | Enum (n, { enum_prefix }) -> pr " int %s = %s_val (%sv);\n" n enum_prefix n | Flags (n, { flag_prefix }) -> -- 2.22.0
On 8/12/19 11:08 AM, Richard W.M. Jones wrote:
Instead of using the valid_flag == FREE mechanism, use a free callback to free each closure root. --- generator/generator | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
Looks like a straight swap; at this point in the series, valid_flag & FREE is called exactly as many times as a free callback; and moving to a free callback would mean a later patch could get rid of valid_flag if we wanted (although that's a C API change). Still, if we're able to reliably call the C nbd_aio_pread_callback even from the OCaml nbd.aio_pread, then the valid_flag = FREE mechanism results in fewer function calls than having to register a free callback for each pointer to be cleaned up. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Same as the previous commit, but for Python. --- generator/generator | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/generator/generator b/generator/generator index 109fad6..a0322ee 100755 --- a/generator/generator +++ b/generator/generator @@ -4315,9 +4315,6 @@ let print_python_binding name { args; optargs; ret; may_set_error } = ) cbargs; pr " }\n"; pr "\n"; - pr " if (valid_flag & LIBNBD_CALLBACK_FREE)\n"; - pr " Py_DECREF ((PyObject *)user_data);\n"; - pr "\n"; pr " return ret;\n"; pr "}\n"; pr "\n" @@ -4454,6 +4451,11 @@ let print_python_binding name { args; optargs; ret; may_set_error } = pr " PyErr_SetString (PyExc_TypeError,\n"; pr " \"callback parameter %s is not callable\");\n" cbname; pr " return NULL;\n"; + pr " }\n"; + pr " if (nbd_add_free_callback (h, %s_user_data,\n" cbname; + pr " decref, NULL) == -1) {\n"; + pr " PyErr_NoMemory ();\n"; + pr " return NULL;\n"; pr " }\n" | Enum _ -> () | Flags (n, _) -> pr " %s_u32 = %s;\n" n n @@ -4600,6 +4602,12 @@ let generate_python_methods_c () = pr "\n"; pr "#include <methods.h>\n"; pr "\n"; + pr "static void\n"; + pr "decref (void *ptr, void *user_data)\n"; + pr "{\n"; + pr " Py_DECREF (ptr);\n"; + pr "}\n"; + pr "\n"; List.iter ( fun (name, fn) -> print_python_binding name fn -- 2.22.0
On 8/12/19 11:08 AM, Richard W.M. Jones wrote:
Same as the previous commit, but for Python. --- generator/generator | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
@@ -4454,6 +4451,11 @@ let print_python_binding name { args; optargs; ret; may_set_error } = pr " PyErr_SetString (PyExc_TypeError,\n"; pr " \"callback parameter %s is not callable\");\n" cbname; pr " return NULL;\n"; + pr " }\n"; + pr " if (nbd_add_free_callback (h, %s_user_data,\n" cbname; + pr " decref, NULL) == -1) {\n"; + pr " PyErr_NoMemory ();\n"; + pr " return NULL;\n"; pr " }\n" | Enum _ -> () | Flags (n, _) -> pr " %s_u32 = %s;\n" n n @@ -4600,6 +4602,12 @@ let generate_python_methods_c () = pr "\n"; pr "#include <methods.h>\n"; pr "\n"; + pr "static void\n"; + pr "decref (void *ptr, void *user_data)\n"; + pr "{\n"; + pr " Py_DECREF (ptr);\n"; + pr "}\n";
Well, in the OCaml case, you were actively using the fact that ptr and user_data might be different, and freeing the second; where in the Python case you are only ever caring about the ptr argument and always passing NULL for the second (meaning you had to write a shim around Py_DecRef, as hidden under the Py_DECREF macro). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
The freeing feature can now be done by associating a free callback with a closure's user_data, so having the valid_flag is no longer necessary and it can be completely removed. This mostly reverts commit 2d9b98e96772e282d51dafac07f16387dadc8afa. --- TODO | 2 - docs/libnbd.pod | 64 ++-------------- examples/glib-main-loop.c | 14 +--- examples/strict-structured-reads.c | 63 +++++++--------- generator/generator | 84 +++++++++------------ generator/states-reply-simple.c | 3 +- generator/states-reply-structured.c | 45 ++++-------- generator/states-reply.c | 3 +- generator/states.c | 3 +- interop/dirty-bitmap.c | 5 +- interop/structured-read.c | 5 +- lib/aio.c | 11 --- lib/debug.c | 7 +- lib/handle.c | 4 +- tests/closure-lifetimes.c | 109 +++++++++++++++++----------- tests/meta-base-allocation.c | 7 +- tests/oldstyle.c | 5 +- tests/server-death.c | 5 +- 18 files changed, 165 insertions(+), 274 deletions(-) diff --git a/TODO b/TODO index 8e067c0..03fda1f 100644 --- a/TODO +++ b/TODO @@ -26,8 +26,6 @@ Improve function trace output so that: Suggested API improvements: general: - synchronous APIs that have a timeout or can be cancelled - - separate free_callback corresponding to every Closure - (instead of using valid_flag) connecting: - nbd_connect_tcp: allow control over whether IPv4 or IPv6 is desired diff --git a/docs/libnbd.pod b/docs/libnbd.pod index 51b1a03..0068b84 100644 --- a/docs/libnbd.pod +++ b/docs/libnbd.pod @@ -608,59 +608,6 @@ as the second parameter to the callback. The opaque pointer is only used from the C API, since in other languages you can use closures to achieve the same outcome. -=head2 Callback lifetimes - -All callbacks have an C<unsigned valid_flag> parameter which is used -to help with the lifetime of the callback. C<valid_flag> contains the -I<logical or> of: - -=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. - -=back - -For example C<nbd_set_debug_callback> sets up a callback which you -could define like this: - - int my_debug_fn (unsigned 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 (unsigned valid_flag, void *user_data, - const char *context, const char *msg) - { - if (!(valid_flag & LIBNBD_CALLBACK_VALID)) return 0; - - // Rest of callback as normal. - } - -The valid flag is only present in the C API. It is not needed when -using garbage-collected programming languages. - =head2 Callbacks and locking The callbacks are invoked at a point where the libnbd lock is held; as @@ -673,10 +620,10 @@ All of the low-level commands have a completion callback variant that registers a callback function used right before the command is marked complete. -When C<valid_flag> includes C<LIBNBD_CALLBACK_VALID>, and the -completion callback returns C<1>, the command is automatically retired -(there is no need to call C<nbd_aio_command_completed>); for any other -return value, the command still needs to be retired. +When the completion callback returns C<1>, the command is +automatically retired (there is no need to call +C<nbd_aio_command_completed>); for any other return value, the command +still needs to be retired. =head2 Callbacks with C<int *error> parameter @@ -688,8 +635,7 @@ all of the completion callbacks, include a parameter C<error> containing the value of any error detected so far; if the callback function fails, it should assign back into C<error> and return C<-1> to change the resulting error of the overall command. Assignments -into C<error> are ignored for any other return value or when -C<valid_flag> did not contain C<LIBNBD_CALLBACK_VALID>; similarly, +into C<error> are ignored for any other return value; similarly, assigning C<0> into C<error> does not have an effect. =head1 SEE ALSO diff --git a/examples/glib-main-loop.c b/examples/glib-main-loop.c index 05a59e3..216eb5e 100644 --- a/examples/glib-main-loop.c +++ b/examples/glib-main-loop.c @@ -268,9 +268,9 @@ static GMainLoop *loop; static void connected (struct NBDSource *source); static gboolean read_data (gpointer user_data); -static int finished_read (unsigned valid_flag, void *vp, int *error); +static int finished_read (void *vp, int *error); static gboolean write_data (gpointer user_data); -static int finished_write (unsigned valid_flag, void *vp, int *error); +static int finished_write (void *vp, int *error); int main (int argc, char *argv[]) @@ -394,13 +394,10 @@ read_data (gpointer user_data) /* This callback is called from libnbd when any read command finishes. */ static int -finished_read (unsigned valid_flag, void *vp, int *error) +finished_read (void *vp, int *error) { struct buffer *buffer = vp; - if (!(valid_flag & LIBNBD_CALLBACK_VALID)) - return 0; - if (gssrc == NULL) return 0; @@ -442,13 +439,10 @@ write_data (gpointer user_data) /* This callback is called from libnbd when any write command finishes. */ static int -finished_write (unsigned valid_flag, void *vp, int *error) +finished_write (void *vp, int *error) { struct buffer *buffer = vp; - if (!(valid_flag & LIBNBD_CALLBACK_VALID)) - return 0; - if (gsdest == NULL) return 0; diff --git a/examples/strict-structured-reads.c b/examples/strict-structured-reads.c index b3880b7..701b216 100644 --- a/examples/strict-structured-reads.c +++ b/examples/strict-structured-reads.c @@ -51,16 +51,13 @@ static int64_t total_bytes; static int total_success; static int -read_chunk (unsigned valid_flag, void *opaque, +read_chunk (void *opaque, const void *bufv, size_t count, uint64_t offset, unsigned status, int *error) { struct data *data = opaque; struct range *r, **prev; - if (!(valid_flag & LIBNBD_CALLBACK_VALID)) - return 0; - /* libnbd guarantees this: */ assert (offset >= data->offset); assert (offset + count <= data->offset + data->count); @@ -131,46 +128,41 @@ read_chunk (unsigned valid_flag, void *opaque, } static int -read_verify (unsigned valid_flag, void *opaque, int *error) +read_verify (void *opaque, int *error) { int ret = 0; - if (valid_flag & LIBNBD_CALLBACK_VALID) { - struct data *data = opaque; + struct data *data = opaque; - ret = -1; - total_reads++; - total_chunks += data->chunks; - if (*error) - goto cleanup; - assert (data->chunks > 0); - if (data->flags & LIBNBD_CMD_FLAG_DF) { - total_df_reads++; - if (data->chunks > 1) { - fprintf (stderr, "buggy server: too many chunks for DF flag\n"); - *error = EPROTO; - goto cleanup; - } - } - if (data->remaining && !*error) { - fprintf (stderr, "buggy server: not enough chunks on success\n"); + ret = -1; + total_reads++; + total_chunks += data->chunks; + if (*error) + goto cleanup; + assert (data->chunks > 0); + if (data->flags & LIBNBD_CMD_FLAG_DF) { + total_df_reads++; + if (data->chunks > 1) { + fprintf (stderr, "buggy server: too many chunks for DF flag\n"); *error = EPROTO; goto cleanup; } - total_bytes += data->count; - total_success++; - ret = 0; - - cleanup: - while (data->remaining) { - struct range *r = data->remaining; - data->remaining = r->next; - free (r); - } } + if (data->remaining && !*error) { + fprintf (stderr, "buggy server: not enough chunks on success\n"); + *error = EPROTO; + goto cleanup; + } + total_bytes += data->count; + total_success++; + ret = 0; - if (valid_flag & LIBNBD_CALLBACK_FREE) - free (opaque); + cleanup: + while (data->remaining) { + struct range *r = data->remaining; + data->remaining = r->next; + free (r); + } return ret; } @@ -235,6 +227,7 @@ main (int argc, char *argv[]) *r = (struct range) { .first = offset, .last = offset + maxsize, }; *d = (struct data) { .offset = offset, .count = maxsize, .flags = flags, .remaining = r, }; + /* XXX *d and *r are both leaked. */ if (nbd_aio_pread_structured_callback (nbd, buf, sizeof buf, offset, read_chunk, d, read_verify, d, flags) == -1) { diff --git a/generator/generator b/generator/generator index a0322ee..0187c02 100755 --- a/generator/generator +++ b/generator/generator @@ -3190,7 +3190,7 @@ module C : sig val generate_docs_libnbd_api_pod : unit -> unit val print_arg_list : ?handle:bool -> ?types:bool -> arg list -> optarg list -> unit - val print_cbarg_list : ?valid_flag:bool -> ?types:bool -> cbarg list -> unit + val print_cbarg_list : ?types:bool -> cbarg list -> unit val errcode_of_ret : ret -> string option val type_of_ret : ret -> string end = struct @@ -3430,13 +3430,8 @@ let print_extern name args optargs ret = print_call name args optargs ret; pr ";\n" -let print_cbarg_list ?(valid_flag = true) ?(types = true) cbargs = +let print_cbarg_list ?(types = true) cbargs = pr "("; - if valid_flag then ( - if types then pr "unsigned "; - pr "valid_flag"; - pr ", "; - ); if types then pr "void *"; pr "user_data"; @@ -3567,9 +3562,6 @@ let generate_include_libnbd_h () = pr "#define %-40s %d\n" n i ) constants; pr "\n"; - pr "#define LIBNBD_CALLBACK_VALID 1\n"; - pr "#define LIBNBD_CALLBACK_FREE 2\n"; - pr "\n"; pr "extern struct nbd_handle *nbd_create (void);\n"; pr "#define LIBNBD_HAVE_NBD_CREATE 1\n"; pr "\n"; @@ -4226,26 +4218,25 @@ let print_python_binding name { args; optargs; ret; may_set_error } = pr "{\n"; pr " int ret = 0;\n"; pr "\n"; - pr " if (valid_flag & LIBNBD_CALLBACK_VALID) {\n"; - pr " PyGILState_STATE py_save = PyGILState_UNLOCKED;\n"; - pr " PyObject *py_args, *py_ret;\n"; + pr " PyGILState_STATE py_save = PyGILState_UNLOCKED;\n"; + pr " PyObject *py_args, *py_ret;\n"; List.iter ( function | CBArrayAndLen (UInt32 n, len) -> - pr " PyObject *py_%s = PyList_New (%s);\n" n len; - pr " for (size_t i = 0; i < %s; ++i)\n" len; - pr " PyList_SET_ITEM (py_%s, i, PyLong_FromUnsignedLong (%s[i]));\n" n n + pr " PyObject *py_%s = PyList_New (%s);\n" n len; + pr " for (size_t i = 0; i < %s; ++i)\n" len; + pr " PyList_SET_ITEM (py_%s, i, PyLong_FromUnsignedLong (%s[i]));\n" n n | CBBytesIn _ | CBInt _ | CBInt64 _ -> () | CBMutable (Int n) -> - pr " PyObject *py_%s_modname = PyUnicode_FromString (\"ctypes\");\n" n; - pr " if (!py_%s_modname) { PyErr_PrintEx (0); return -1; }\n" n; - pr " PyObject *py_%s_mod = PyImport_Import (py_%s_modname);\n" n n; - pr " Py_DECREF (py_%s_modname);\n" n; - pr " if (!py_%s_mod) { PyErr_PrintEx (0); return -1; }\n" n; - pr " PyObject *py_%s = PyObject_CallMethod (py_%s_mod, \"c_int\", \"i\", *%s);\n" n n n; - pr " if (!py_%s) { PyErr_PrintEx (0); return -1; }\n" n; + pr " PyObject *py_%s_modname = PyUnicode_FromString (\"ctypes\");\n" n; + pr " if (!py_%s_modname) { PyErr_PrintEx (0); return -1; }\n" n; + pr " PyObject *py_%s_mod = PyImport_Import (py_%s_modname);\n" n n; + pr " Py_DECREF (py_%s_modname);\n" n; + pr " if (!py_%s_mod) { PyErr_PrintEx (0); return -1; }\n" n; + pr " PyObject *py_%s = PyObject_CallMethod (py_%s_mod, \"c_int\", \"i\", *%s);\n" n n n; + pr " if (!py_%s) { PyErr_PrintEx (0); return -1; }\n" n; | CBString _ | CBUInt _ | CBUInt64 _ -> () @@ -4253,7 +4244,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = ) cbargs; pr "\n"; - pr " py_args = Py_BuildValue (\"(\""; + pr " py_args = Py_BuildValue (\"(\""; List.iter ( function | CBArrayAndLen (UInt32 n, len) -> pr " \"O\"" @@ -4278,42 +4269,41 @@ let print_python_binding name { args; optargs; ret; may_set_error } = | CBArrayAndLen _ | CBMutable _ -> assert false ) cbargs; pr ");\n"; - pr " Py_INCREF (py_args);\n"; + pr " Py_INCREF (py_args);\n"; pr "\n"; - pr " if (PyEval_ThreadsInitialized ())\n"; - pr " py_save = PyGILState_Ensure ();\n"; + pr " if (PyEval_ThreadsInitialized ())\n"; + pr " py_save = PyGILState_Ensure ();\n"; pr "\n"; - pr " py_ret = PyObject_CallObject ((PyObject *)user_data, py_args);\n"; + pr " py_ret = PyObject_CallObject ((PyObject *)user_data, py_args);\n"; pr "\n"; - pr " if (PyEval_ThreadsInitialized ())\n"; - pr " PyGILState_Release (py_save);\n"; + pr " if (PyEval_ThreadsInitialized ())\n"; + pr " PyGILState_Release (py_save);\n"; pr "\n"; - pr " Py_DECREF (py_args);\n"; + pr " Py_DECREF (py_args);\n"; pr "\n"; - pr " if (py_ret != NULL) {\n"; - pr " Py_DECREF (py_ret); /* return value is discarded */\n"; - pr " }\n"; - pr " else {\n"; - pr " ret = -1;\n"; - pr " PyErr_PrintEx (0); /* print exception */\n"; - pr " };\n"; + pr " if (py_ret != NULL) {\n"; + pr " Py_DECREF (py_ret); /* return value is discarded */\n"; + pr " }\n"; + pr " else {\n"; + pr " ret = -1;\n"; + pr " PyErr_PrintEx (0); /* print exception */\n"; + pr " };\n"; pr "\n"; List.iter ( function | CBArrayAndLen (UInt32 n, _) -> - pr " Py_DECREF (py_%s);\n" n + pr " Py_DECREF (py_%s);\n" n | CBMutable (Int n) -> - pr " PyObject *py_%s_ret = PyObject_GetAttrString (py_%s, \"value\");\n" n n; - pr " *%s = PyLong_AsLong (py_%s_ret);\n" n n; - pr " Py_DECREF (py_%s_ret);\n" n; - pr " Py_DECREF (py_%s);\n" n + pr " PyObject *py_%s_ret = PyObject_GetAttrString (py_%s, \"value\");\n" n n; + pr " *%s = PyLong_AsLong (py_%s_ret);\n" n n; + pr " Py_DECREF (py_%s_ret);\n" n; + pr " Py_DECREF (py_%s);\n" n | CBBytesIn _ | CBInt _ | CBInt64 _ | CBString _ | CBUInt _ | CBUInt64 _ -> () | CBArrayAndLen _ | CBMutable _ -> assert false ) cbargs; - pr " }\n"; pr "\n"; pr " return ret;\n"; pr "}\n"; @@ -5159,7 +5149,7 @@ let print_ocaml_binding (name, { args; optargs; ret }) = pr "/* Wrapper for %s callback of %s. */\n" cbname name; pr "static int\n"; pr "%s_%s_wrapper_locked " name cbname; - C.print_cbarg_list ~valid_flag:false cbargs; + C.print_cbarg_list cbargs; pr "\n"; pr "{\n"; pr " CAMLparam0 ();\n"; @@ -5239,13 +5229,11 @@ let print_ocaml_binding (name, { args; optargs; ret }) = pr "{\n"; pr " int ret = 0;\n"; pr "\n"; - pr " if (valid_flag & LIBNBD_CALLBACK_VALID) {\n"; pr " caml_leave_blocking_section ();\n"; pr " ret = %s_%s_wrapper_locked " name cbname; - C.print_cbarg_list ~valid_flag:false ~types:false cbargs; + C.print_cbarg_list ~types:false cbargs; pr ";\n"; pr " caml_enter_blocking_section ();\n"; - pr " }\n"; pr "\n"; pr " return ret;\n"; pr "}\n"; diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index 9684bc4..11354e5 100644 --- a/generator/states-reply-simple.c +++ b/generator/states-reply-simple.c @@ -64,8 +64,7 @@ int error = 0; assert (cmd->error == 0); - if (cmd->cb.fn.chunk (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, - cmd->cb.fn_user_data, + if (cmd->cb.fn.chunk (cmd->cb.fn_user_data, cmd->data, cmd->count, cmd->offset, LIBNBD_READ_DATA, &error) == -1) cmd->error = error ? error : EPROTO; diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index b016cd7..846cc35 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -18,17 +18,6 @@ /* State machine for parsing structured replies from the server. */ -static unsigned -valid_flags (struct nbd_handle *h) -{ - unsigned valid = LIBNBD_CALLBACK_VALID; - uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags); - - if (flags & NBD_REPLY_FLAG_DONE) - valid |= LIBNBD_CALLBACK_FREE; - return valid; -} - /*----- End of prologue. -----*/ /* STATE MACHINE */ { @@ -306,18 +295,18 @@ valid_flags (struct nbd_handle *h) } if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk) { int scratch = error; - unsigned valid = valid_flags (h); + uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags); /* Different from successful reads: inform the callback about the * current error rather than any earlier one. If the callback fails * without setting errno, then use the server's error below. */ - if (cmd->cb.fn.chunk (valid, cmd->cb.fn_user_data, + if (cmd->cb.fn.chunk (cmd->cb.fn_user_data, cmd->data + (offset - cmd->offset), 0, offset, LIBNBD_READ_ERROR, &scratch) == -1) if (cmd->error == 0) cmd->error = scratch; - if (valid & LIBNBD_CALLBACK_FREE) { + if (flags & NBD_REPLY_FLAG_DONE) { cmd->cb.fn.chunk = NULL; /* because we've freed it */ nbd_internal_free_callback (h, cmd->cb.fn_user_data); } @@ -402,15 +391,15 @@ valid_flags (struct nbd_handle *h) assert (cmd); /* guaranteed by CHECK */ if (cmd->cb.fn.chunk) { int error = cmd->error; - unsigned valid = valid_flags (h); + uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags); - if (cmd->cb.fn.chunk (valid, cmd->cb.fn_user_data, + if (cmd->cb.fn.chunk (cmd->cb.fn_user_data, cmd->data + (offset - cmd->offset), length - sizeof offset, offset, LIBNBD_READ_DATA, &error) == -1) if (cmd->error == 0) cmd->error = error ? error : EPROTO; - if (valid & LIBNBD_CALLBACK_FREE) { + if (flags & NBD_REPLY_FLAG_DONE) { cmd->cb.fn.chunk = NULL; /* because we've freed it */ nbd_internal_free_callback (h, cmd->cb.fn_user_data); } @@ -469,15 +458,15 @@ valid_flags (struct nbd_handle *h) memset (cmd->data + offset, 0, length); if (cmd->cb.fn.chunk) { int error = cmd->error; - unsigned valid = valid_flags (h); + uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags); - if (cmd->cb.fn.chunk (valid, cmd->cb.fn_user_data, + if (cmd->cb.fn.chunk (cmd->cb.fn_user_data, cmd->data + offset, length, cmd->offset + offset, LIBNBD_READ_HOLE, &error) == -1) if (cmd->error == 0) cmd->error = error ? error : EPROTO; - if (valid & LIBNBD_CALLBACK_FREE) { + if (flags & NBD_REPLY_FLAG_DONE) { cmd->cb.fn.chunk = NULL; /* because we've freed it */ nbd_internal_free_callback (h, cmd->cb.fn_user_data); } @@ -526,14 +515,14 @@ valid_flags (struct nbd_handle *h) if (meta_context) { /* Call the caller's extent function. */ int error = cmd->error; - unsigned valid = valid_flags (h); + uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags); - if (cmd->cb.fn.extent (valid, cmd->cb.fn_user_data, + if (cmd->cb.fn.extent (cmd->cb.fn_user_data, meta_context->name, cmd->offset, &h->bs_entries[1], (length-4) / 4, &error) == -1) if (cmd->error == 0) cmd->error = error ? error : EPROTO; - if (valid & LIBNBD_CALLBACK_FREE) { + if (flags & NBD_REPLY_FLAG_DONE) { cmd->cb.fn.extent = NULL; /* because we've freed it */ nbd_internal_free_callback (h, cmd->cb.fn_user_data); } @@ -553,16 +542,10 @@ valid_flags (struct nbd_handle *h) flags = be16toh (h->sbuf.sr.structured_reply.flags); if (flags & NBD_REPLY_FLAG_DONE) { - if (cmd->type == NBD_CMD_BLOCK_STATUS && cmd->cb.fn.extent) { - cmd->cb.fn.extent (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data, - NULL, 0, NULL, 0, NULL); + if (cmd->type == NBD_CMD_BLOCK_STATUS && cmd->cb.fn.extent) nbd_internal_free_callback (h, cmd->cb.fn_user_data); - } - if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk) { - cmd->cb.fn.chunk (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data, - NULL, 0, 0, 0, NULL); + if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk) nbd_internal_free_callback (h, cmd->cb.fn_user_data); - } cmd->cb.fn.chunk = NULL; SET_NEXT_STATE (%^FINISH_COMMAND); } diff --git a/generator/states-reply.c b/generator/states-reply.c index d5cba1a..1e0e92c 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -173,8 +173,7 @@ save_reply_state (struct nbd_handle *h) int r; assert (cmd->type != NBD_CMD_DISC); - r = cmd->cb.completion (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, - cmd->cb.user_data, &error); + r = cmd->cb.completion (cmd->cb.user_data, &error); nbd_internal_free_callback (h, cmd->cb.user_data); cmd->cb.completion = NULL; /* because we've freed it */ switch (r) { diff --git a/generator/states.c b/generator/states.c index 313d2c9..4d3a93c 100644 --- a/generator/states.c +++ b/generator/states.c @@ -126,8 +126,7 @@ void abort_commands (struct nbd_handle *h, int r; assert (cmd->type != NBD_CMD_DISC); - r = cmd->cb.completion (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, - cmd->cb.user_data, &error); + r = cmd->cb.completion (cmd->cb.user_data, &error); nbd_internal_free_callback (h, cmd->cb.user_data); cmd->cb.completion = NULL; /* because we've freed it */ switch (r) { diff --git a/interop/dirty-bitmap.c b/interop/dirty-bitmap.c index aca0564..fbd6f29 100644 --- a/interop/dirty-bitmap.c +++ b/interop/dirty-bitmap.c @@ -42,14 +42,11 @@ struct data { }; static int -cb (unsigned valid_flag, void *opaque, const char *metacontext, uint64_t offset, +cb (void *opaque, const char *metacontext, uint64_t offset, uint32_t *entries, size_t len, int *error) { struct data *data = opaque; - if (!(valid_flag & LIBNBD_CALLBACK_VALID)) - return 0; - /* libnbd does not actually verify that a server is fully compliant * to the spec; the asserts marked [qemu-nbd] are thus dependent on * the fact that qemu-nbd is compliant. diff --git a/interop/structured-read.c b/interop/structured-read.c index 0b189d1..3e62b05 100644 --- a/interop/structured-read.c +++ b/interop/structured-read.c @@ -48,16 +48,13 @@ struct data { }; static int -read_cb (unsigned valid_flag, void *opaque, +read_cb (void *opaque, const void *bufv, size_t count, uint64_t offset, unsigned status, int *error) { struct data *data = opaque; const char *buf = bufv; - if (!(valid_flag & LIBNBD_CALLBACK_VALID)) - return 0; - /* The NBD spec allows chunks to be reordered; we are relying on the * fact that qemu-nbd does not do so. */ diff --git a/lib/aio.c b/lib/aio.c index 8aa4547..4cc61cb 100644 --- a/lib/aio.c +++ b/lib/aio.c @@ -32,17 +32,6 @@ void nbd_internal_retire_and_free_command (struct nbd_handle *h, struct command *cmd) { - /* Free the callbacks. */ - if (cmd->type == NBD_CMD_BLOCK_STATUS && cmd->cb.fn.extent) - cmd->cb.fn.extent (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data, - NULL, 0, NULL, 0, NULL); - if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk) - cmd->cb.fn.chunk (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data, - NULL, 0, 0, 0, NULL); - if (cmd->cb.completion) - cmd->cb.completion (LIBNBD_CALLBACK_FREE, cmd->cb.user_data, - NULL); - /* Free the closures if there's an associated free callback. */ nbd_internal_free_callback (h, cmd->cb.fn_user_data); nbd_internal_free_callback (h, cmd->cb.user_data); diff --git a/lib/debug.c b/lib/debug.c index 34d4184..bd0c465 100644 --- a/lib/debug.c +++ b/lib/debug.c @@ -42,11 +42,8 @@ int nbd_unlocked_set_debug_callback (struct nbd_handle *h, nbd_debug_callback debug_callback, void *data) { - if (h->debug_callback) { - /* ignore return value */ - h->debug_callback (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL); + if (h->debug_callback) nbd_internal_free_callback (h, h->debug_data); - } h->debug_callback = debug_callback; h->debug_data = data; return 0; @@ -80,7 +77,7 @@ nbd_internal_debug (struct nbd_handle *h, const char *fs, ...) if (h->debug_callback) /* ignore return value */ - h->debug_callback (LIBNBD_CALLBACK_VALID, h->debug_data, context, msg); + h->debug_callback (h->debug_data, context, msg); else fprintf (stderr, "libnbd: debug: %s: %s: %s\n", h->hname, context ? : "unknown", msg); diff --git a/lib/handle.c b/lib/handle.c index 5a47bd8..4c59622 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -113,10 +113,8 @@ nbd_close (struct nbd_handle *h) return; /* Free user callbacks first. */ - if (h->debug_callback) { - h->debug_callback (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL); + if (h->debug_callback) nbd_internal_free_callback (h, h->debug_data); - } h->debug_callback = NULL; free (h->bs_entries); diff --git a/tests/closure-lifetimes.c b/tests/closure-lifetimes.c index d8ea3f7..c1815b0 100644 --- a/tests/closure-lifetimes.c +++ b/tests/closure-lifetimes.c @@ -38,50 +38,59 @@ static char *nbdkit_delay[] = "delay-read=10", NULL }; -static unsigned debug_fn_valid; -static unsigned debug_fn_free; -static unsigned read_cb_valid; -static unsigned read_cb_free; -static unsigned completion_cb_valid; -static unsigned completion_cb_free; +static unsigned debug_fn_called; +static unsigned debug_fn_freed; +static unsigned read_cb_called; +static unsigned read_cb_freed; +static unsigned completion_cb_called; +static unsigned completion_cb_freed; static int -debug_fn (unsigned valid_flag, void *opaque, +debug_fn (void *opaque, const char *context, const char *msg) { - if (valid_flag & LIBNBD_CALLBACK_VALID) - debug_fn_valid++; - if (valid_flag & LIBNBD_CALLBACK_FREE) - debug_fn_free++; + debug_fn_called++; return 0; } +static void +debug_fn_free (void *ptr, void *opaque) +{ + debug_fn_freed++; +} + static int -read_cb (unsigned valid_flag, void *opaque, +read_cb (void *opaque, const void *subbuf, size_t count, uint64_t offset, unsigned status, int *error) { - assert (read_cb_free == 0); + assert (read_cb_freed == 0); - if (valid_flag & LIBNBD_CALLBACK_VALID) - read_cb_valid++; - if (valid_flag & LIBNBD_CALLBACK_FREE) - read_cb_free++; + read_cb_called++; return 0; } +static void +read_cb_free (void *ptr, void *opaque) +{ + read_cb_freed++; +} + static int -completion_cb (unsigned valid_flag, void *opaque, int *error) +completion_cb (void *opaque, int *error) { - assert (completion_cb_free == 0); + assert (completion_cb_freed == 0); - if (valid_flag & LIBNBD_CALLBACK_VALID) - completion_cb_valid++; - if (valid_flag & LIBNBD_CALLBACK_FREE) - completion_cb_free++; + completion_cb_called++; return 0; } +static void +completion_cb_free (void *ptr, void *opaque) +{ + completion_cb_freed++; +} + #define NBD_ERROR \ do { \ fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ()); \ @@ -101,35 +110,44 @@ main (int argc, char *argv[]) nbd = nbd_create (); if (nbd == NULL) NBD_ERROR; - nbd_set_debug_callback (nbd, debug_fn, NULL); - assert (debug_fn_free == 0); + if (nbd_add_free_callback (nbd, &debug_fn, debug_fn_free, NULL) == -1) + NBD_ERROR; + nbd_set_debug_callback (nbd, debug_fn, &debug_fn); + assert (debug_fn_freed == 0); - nbd_set_debug_callback (nbd, debug_fn, NULL); - assert (debug_fn_free == 1); + if (nbd_add_free_callback (nbd, &debug_fn, debug_fn_free, NULL) == -1) + NBD_ERROR; + nbd_set_debug_callback (nbd, debug_fn, &debug_fn); + assert (debug_fn_freed == 1); - debug_fn_free = 0; + debug_fn_freed = 0; nbd_close (nbd); - assert (debug_fn_free == 1); + assert (debug_fn_freed == 1); /* Test command callbacks are freed when the command is retired. */ nbd = nbd_create (); if (nbd == NULL) NBD_ERROR; if (nbd_connect_command (nbd, nbdkit) == -1) NBD_ERROR; + if (nbd_add_free_callback (nbd, &read_cb, read_cb_free, NULL) == -1) + NBD_ERROR; + if (nbd_add_free_callback (nbd, &completion_cb, completion_cb_free, NULL) + == -1) + NBD_ERROR; cookie = nbd_aio_pread_structured_callback (nbd, buf, sizeof buf, 0, - read_cb, NULL, - completion_cb, NULL, 0); + read_cb, &read_cb, + completion_cb, &completion_cb, 0); if (cookie == -1) NBD_ERROR; - assert (read_cb_free == 0); - assert (completion_cb_free == 0); + assert (read_cb_freed == 0); + assert (completion_cb_freed == 0); while (!nbd_aio_command_completed (nbd, cookie)) { if (nbd_poll (nbd, -1) == -1) NBD_ERROR; } - assert (read_cb_valid == 1); - assert (completion_cb_valid == 1); - assert (read_cb_free == 1); - assert (completion_cb_free == 1); + assert (read_cb_called == 1); + assert (completion_cb_called == 1); + assert (read_cb_freed == 1); + assert (completion_cb_freed == 1); nbd_kill_command (nbd, 0); nbd_close (nbd); @@ -137,21 +155,26 @@ main (int argc, char *argv[]) /* Test command callbacks are freed if the handle is closed without * running the commands. */ - read_cb_valid = read_cb_free = - completion_cb_valid = completion_cb_free = 0; + read_cb_called = read_cb_freed = + completion_cb_called = completion_cb_freed = 0; nbd = nbd_create (); if (nbd == NULL) NBD_ERROR; if (nbd_connect_command (nbd, nbdkit_delay) == -1) NBD_ERROR; + if (nbd_add_free_callback (nbd, &read_cb, read_cb_free, NULL) == -1) + NBD_ERROR; + if (nbd_add_free_callback (nbd, &completion_cb, completion_cb_free, NULL) + == -1) + NBD_ERROR; cookie = nbd_aio_pread_structured_callback (nbd, buf, sizeof buf, 0, - read_cb, NULL, - completion_cb, NULL, 0); + read_cb, &read_cb, + completion_cb, &completion_cb, 0); if (cookie == -1) NBD_ERROR; nbd_kill_command (nbd, 0); nbd_close (nbd); - assert (read_cb_free == 1); - assert (completion_cb_free == 1); + assert (read_cb_freed == 1); + assert (completion_cb_freed == 1); exit (EXIT_SUCCESS); } diff --git a/tests/meta-base-allocation.c b/tests/meta-base-allocation.c index c36a77f..0219ce9 100644 --- a/tests/meta-base-allocation.c +++ b/tests/meta-base-allocation.c @@ -30,7 +30,7 @@ #include <libnbd.h> -static int check_extent (unsigned valid_flag, void *data, +static int check_extent (void *data, const char *metacontext, uint64_t offset, uint32_t *entries, size_t nr_entries, int *error); @@ -129,7 +129,7 @@ main (int argc, char *argv[]) } static int -check_extent (unsigned valid_flag, void *data, +check_extent (void *data, const char *metacontext, uint64_t offset, uint32_t *entries, size_t nr_entries, int *error) @@ -137,9 +137,6 @@ check_extent (unsigned valid_flag, void *data, size_t i; int id; - if (!(valid_flag & LIBNBD_CALLBACK_VALID)) - return 0; - id = * (int *)data; printf ("extent: id=%d, metacontext=%s, offset=%" PRIu64 ", " diff --git a/tests/oldstyle.c b/tests/oldstyle.c index afbda61..1382d06 100644 --- a/tests/oldstyle.c +++ b/tests/oldstyle.c @@ -37,15 +37,12 @@ static char wbuf[512] = { 1, 2, 3, 4 }, rbuf[512]; static const char *progname; static int -pread_cb (unsigned valid_flag, void *data, +pread_cb (void *data, const void *buf, size_t count, uint64_t offset, unsigned status, int *error) { int *calls; - if (!(valid_flag & LIBNBD_CALLBACK_VALID)) - return 0; - calls = data; ++*calls; diff --git a/tests/server-death.c b/tests/server-death.c index 7854527..256b45a 100644 --- a/tests/server-death.c +++ b/tests/server-death.c @@ -33,11 +33,8 @@ static bool trim_retired; static const char *progname; static int -callback (unsigned valid_flag, void *ignored, int *error) +callback (void *ignored, int *error) { - if (!(valid_flag & LIBNBD_CALLBACK_VALID)) - return 0; - if (*error != ENOTCONN) { fprintf (stderr, "%s: unexpected error in trim callback: %s\n", progname, strerror (*error)); -- 2.22.0
On 8/12/19 11:08 AM, Richard W.M. Jones wrote:
The freeing feature can now be done by associating a free callback with a closure's user_data, so having the valid_flag is no longer necessary and it can be completely removed.
This mostly reverts commit 2d9b98e96772e282d51dafac07f16387dadc8afa.
Aha - the end goal is to get rid of valid_flag. But still, we might be able to do that by instead having C expand Closure into a 3-tuple (callback, free_cb, user_data) where we call callback(user_data, args) as needed and then free_cb(user_data) when done, rather than having to add a list of free callback registrations. Definitely an API/ABI change (the fio code using the existing interface will need tweaking), but seems like a worthwhile goal, even if we change our minds about how best to get to this point. I'll have to play more with the ideas of making the generator smarter about using underlying nbd_aio_pread_callback even for the public Python/OCaml nbd.nbd_aio_pread as a way to create a C structure that can call free_cb(wrapper) to clean up the lifetime of both the buffer and any language callback reference. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Richard W.M. Jones