Re: [Libguestfs] [PATCH libnbd 1/7] api: Add semi-private function for freeing persistent data.

On 8/12/19 1:13 PM, Richard W.M. Jones wrote: 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); } 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) '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). 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. 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. 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. Yes, if we have to compare them at all. But I'm hoping we don't have to. 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). 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