9:48 a.m.
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