[Libguestfs] [libnbd PATCH 5/6] api: Add new nbd_aio_FOO_notify functions

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190715/99d9f042/attachment.sig>