similar to: [libnbd PATCH 0/2] Fix memory leak with closures

I'm posting this now, as I'm at the end of a workday and I got things working for manual experimentation. Still to do: - write interop tests for qemu-nbd and nbdkit (including my proposed patch addition of qemu-nbd -A to show qemu:allocation-depth) - figure out if we can make 'nbdinfo --map' use the new API to automatically select all contexts advertised by the server Eric Blake

This is my proposal for fixing the documentation to match practice (namely, that completion.callback is not invoked in the cases where the aio call itself reports errors); we could instead try to go the other direction and tweak the generator to guarantee that both completion.callback and completion.free are reached no matter what, but that felt more invasive to me. Eric Blake (2): api: Tighten

In v2: - now based on my proposal to add LIBNBD_SHUTDOWN_IMMEDIATE - four flags instead of two: STRICT_FLAGS is new (patch 4), and STRICT_BOUNDS is separate from STRICT_ZERO_SIZE (patch 5) - various refactorings for more shared code and less duplication Eric Blake (5): api: Add xxx_MASK constant for each Flags type generator: Refactor filtering of accepted OFlags api: Add

In v2: ack'ed preliminary patches have been pushed, and I've added a lot of testsuite coverage as well as putting the new API to use in nbdinfo. Eric Blake (2): api: Add nbd_opt_list_meta_context info: List available meta-contexts lib/internal.h | 1 + generator/API.ml | 84 ++++++++-

We can easily demonstrate a memory leak when repeatedly calling nbd_aio_pread from nbdsh in the wrong state. True, that isn't something a sane client would normally do, but it's worth fixing. The culprit: when nbd_aio_pread returns a cookie, we guarantee to clean up the closure, but if we fail early and never scheduled the command, nothing ever cleans up the closure. We could document

Patches 1 & 2 are rather complex, but the end result is that we pass closures + user_data + free function in single struct parameters as I described previously in this email: https://www.redhat.com/archives/libguestfs/2019-August/msg00210.html Patch 3 adds a convenient FREE_CALLBACK macro which seems a worthwhile simplification if you buy into 1 & 2. Patch 4 adds another macro which is

This is a combination of these two earlier series: https://www.redhat.com/archives/libguestfs/2019-August/msg00235.html https://www.redhat.com/archives/libguestfs/2019-August/msg00240.html plus changes to allow .callback = NULL / .free != NULL, and to reduce the complexity of freeing callbacks. Although it's rather long there's nothing complex here. We might consider squashing some

We have a memory leak when a function with a closure exits early prior to registering that closure through some path that will guarantee cleanup. The easiest way to fix it is to guarantee that once a closure is passed into a public API, it will be cleaned regardless of whether that API succeeds or fails. But to avoid cleaning the closure more than once, we need to refactor our internal handling,

On 8/13/19 5:36 PM, Richard W.M. Jones wrote: > The definition of functions that take a callback is changed so that > the callback and user_data are combined into a single structure, eg: > > int64_t nbd_aio_pread (struct nbd_handle *h, > void *buf, size_t count, uint64_t offset, > - int (*completion_callback) (/*..*/), void *user_data, > +

On Mon, Sep 07, 2020 at 04:46:39PM -0500, Eric Blake wrote: > We have a memory leak when a function with a closure exits early prior > to registering that closure through some path that will guarantee > cleanup. The easiest way to fix it is to guarantee that once a > closure is passed into a public API, it will be cleaned regardless of > whether that API succeeds or fails. But to

This is a subset of my v2 posting, but limited to just the NBD_OPT_LIST handling. The biggest change since v2 is the addition of added unit testing in all four language bindings (C, python, ocaml, golang). The tests require nbdkit built from git on PATH, and may not be entirely idiomatic, but I at least validated that they catch issues (for example, adding an exit statement near the end of the

On Tue, May 30, 2023 at 04:06:32PM +0200, Laszlo Ersek wrote: > On 5/25/23 15:00, Eric Blake wrote: > > Support sending 64-bit requests if extended headers were negotiated. > > This includes setting NBD_CMD_FLAG_PAYLOAD_LEN any time we send an > > extended NBD_CMD_WRITE; this is such a fundamental part of the > > protocol that for now it is easier to silently ignore

Well, I'm not quite done (I still want to get nbdinfo to work on a single nbd connection for all cases when reading the heads of the file is not required), but I'm happy with patches 1-11, and 12-13 show where I'm headed for getting NBD_OPT_INFO to work. Posting now to see if some of the earlier patches are ready to commit while I continue working on the latter half. Eric Blake (13):

On 8/14/19 4:21 AM, Richard W.M. Jones wrote: >>> The nbd_aio_pread function can now be called using: >>> >>> nbd_aio_pread (nbd, buf, sizeof buf, offset, >>> (nbd_completion_callback) { .callback = my_fn, >>> .user_data = my_data }, >> >> Is it worth arranging the C struct to

I think I've addressed everything that was raised in review. Some of the highlights: - Callbacks should be freed reliably along all exit paths. - There's a simple test of closure lifetimes. - I've tried to use VALID|FREE in all the places where I'm confident that it's safe and correct to do. There may be more places. Note this is an optimization and shouldn't

This applies on top of the OClosure v2 series posted a few minutes ago. Rich.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2172516 v1: https://listman.redhat.com/archives/libguestfs/2023-April/031375.html In v2, move the declaration of the "p" helper variable next to the top of the function. Thanks! Laszlo Laszlo Ersek (2): generator/C: print_wrapper: use helper variable for permitted state check generator/C: lib/api.c: indent arg list 2

As mentioned in commits 176fc4ea and 609c25f0, our original plan in adding a flags argument to nbd_shutdown was to let us specify different behaviors at the libnbd level, rather than NBD protocol flags (for that, the user has nbd_aio_disconnect). But when we later parameterized OFlags to accept various bitmasks (commit f891340b), we failed to mark nbd_shutdown as using a different bitmask than

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.

We have been inconsistent on how much we reject client-side without even consulting the server, vs. how much we depend on the server to detect failure (even if our request can be deemed undefined per NBD protocol). I'd like to change it so that by default, we reject as much as we can client-side for less traffic, but where the user can also change things on the fly for server-side integration