On 6/29/19 8:28 AM, Eric Blake wrote: > As mentioned in the previous patch, there are situations where an aio > client wants instant notification when a given command is complete, > rather than having to maintain a separate data structure to track all > in-flight commands and then iterate over that structure to learn which > commands are complete. It's also desirable when writing a server > validation program (such as for checking structured reads for > compliance) to be able to clean up the associated opaque data and have > a final chance to change the overall command status. > > Introduce new nbd_aio_FOO_notify functions for each command. Rewire > the existing nbd_aio_FOO to forward to the new command. (Perhaps the > generator could reduce some of the boilerplate duplication, if a later > patch wants to refactor this). > --- > docs/libnbd.pod | 22 +++- > generator/generator | 278 +++++++++++++++++++++++++++++++++++++++++--- > lib/rw.c | 99 ++++++++++++++-- > 3 files changed, 374 insertions(+), 25 deletions(-)
Responding here to track what we found on IRC:
> +
> + "aio_pread_structured_notify", {
> + default_call with
> + args = [ BytesPersistOut ("buf", "count"); UInt64 "offset";
> + Opaque "data";
> + CallbackPersist ("chunk", [ Opaque "data";
> + BytesIn ("subbuf", "count");
> + UInt64 "offset";
> + Mutable (Int "error");
> + Int "status" ]);
> + CallbackPersist ("notify", [ Opaque "data"; Int64 "handle";
> + Mutable (Int "error") ]);
The code generated for this function,
> + "aio_block_status_notify", {
> + default_call with
> + args = [ UInt64 "count"; UInt64 "offset";
> + Opaque "data";
> + CallbackPersist ("extent", [Opaque "data"; String "metacontext";
> + UInt64 "offset";
> + ArrayAndLen (UInt32 "entries",
> + "nr_entries");
> + Mutable (Int "error") ]);
> + CallbackPersist ("notify", [ Opaque "data"; Int64 "handle";
> + Mutable (Int "error") ]);
> + Flags "flags" ];
and for this is broken. Right now, looking at the generated
python/methods.c, the generator malloc()s two separate wrapper structs,
but only populates the chunk->data (or extent->data) field pertaining to
the shared Opaque "data". Which means when we eventually get around to
calling the notify callback notify(notify_data->data, ...), we are
passing an uninitialized pointer instead of the malloc()d C wrapper
holding the user's Python pointer.
Of course, deferring the cleanup to nbd_add_close_callback is also an
issue (if a client issues 1000 nbd_aio_block_status commands, we've
queued up 1000 malloc()d wrappers that don't get freed until the
connection hangs up, rather than freeing each one as soon as possible by
using the C nbd_aio_block_stattus_callback even for the Python
nbd.aio_block_status).
And it doesn't help that I failed to add a
python/t/5??-pread-callback.py unit test (and similar for all the other
APIs), where a pread-structured and/or block-status unit test would have
caught the bug. Looks like we've got some more generator tweaking to do.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
