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