similar to: [PATCH libnbd 0/2] Use free callback to dereference NBD.Buffer.

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

Basically the same as this patch series, but for Python: https://www.redhat.com/archives/libguestfs/2019-August/msg00235.html plus adding the 590 asynch test at the end. Rich.

Patches 1-4 are basically uncontroversial, straightforward refactoring and IMHO we should just push them. Possibly 1-3 should be squashed together, but I posted them separately so they are easier to review. Patches 5 and 6 together implement OClosure. Patch 5 adds the feature and is simple to understand. Patch 6 changes the Closure completion callbacks into OClosure, but because it doesn't

v1 was here: https://www.redhat.com/archives/libguestfs/2019-August/msg00168.html I pushed uncontroversial patches 1-4 v2: - The implementation of OClosure (new patch 1) in Python is fixed. - Patch 2 (old patch 5) is unchanged. - I added a new API for removing debug callbacks. I think this approach has some advantages over using OClosure. - I didn't yet do any work on changing the

By using the completion free callback function we don't need to manually free the buffer. Note that after this patch OClosures are always called from OCaml, because we might need the side effect of freeing the buffer. This reverts part of commit fef1c281a65d061127bf178e5f8cfca0a2475c56. --- generator/generator | 59 +++++++++++++++++++++----------- ocaml/buffer.c

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

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 implements most of what I wrote here: https://www.redhat.com/archives/libguestfs/2019-July/msg00213.html

v1 was here: https://www.redhat.com/archives/libguestfs/2019-July/thread.html#00231 The changes address everything that Eric picked up in his review of the first two patches. I have also added two more patches (4 and 5) which respectively fix docs and change int status -> unsigned status, as discussed. Passes make, check, check-valgrind. Rich.

Inspection of the generated code showed several places where we did not release references on all error paths. In particular, switching things to always jump to an out: label, even when the underlying C function cannot fail, makes it easier to clean up when the user passes wrong types to a function call. One of the easiest triggers without this patch was this one-liner: $ nbdsh -c

On 8/13/19 10:37 AM, Richard W.M. Jones wrote: > The original nbd_aio_* (non-callback) functions are removed and > replaced with the renamed callback variants. > > This is a simple mechanical change to the API: > > (1) Any existing call to nbd_aio_*_callback can simply be renamed to > nbd_aio_* > > (2) Any existing call to nbd_aio_* must have two extra NULL

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

These are some simplifications to the generator. They don't probably make much sense on their own, but they are preparatory to better handling of enums, and or'd lists of flags. Rich.

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

As promised in my earlier thread on libnbd completion callback question. Eric Blake (2): generator: Refactor handling of closures in unlocked functions generator: Free closures on failure docs/libnbd.pod | 2 +- generator/C.ml | 48 +++++++++++------ generator/C.mli | 1 + lib/debug.c | 7 +-- lib/opt.c | 31 ++++++-----

On 8/13/19 5:06 AM, Richard W.M. Jones wrote: > An optional Closure parameter, but otherwise works the same way as > Closure. > @@ -3778,6 +3777,7 @@ let generate_lib_api_c () = > ) args; > List.iter ( > function > + | OClosure { cbname } -> pr ", %s_callback ? \"<fun>\" : \"NULL\"" cbname Well, it also permits a

As before, but this one has working Python bindings. OCaml still TBD. Rich.

Now runs a complete set of tests, notably including the AIO test. File descriptors are passed in and out as plain ints (instead of *os.File) for a couple of reasons: (1) We have to pass the plain int to syscall.Select. (2) Turning an fd into an os.File causes golang to set the blocking flag which is deeply unhelpful. Rich.

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

h.nbd_connect_command was leaking a python object per parameter, and also leaks memory on failure. In fact, it was possible to segfault on something as trivial as: nbdsh -c 'h.connect_command(["true",1])' h.nbd_pread was leaking a read buffer on every single synchronous read. My previous patch to address closure callbacks had a bug in h.nbd_pread_structured and similar: if the