[Libguestfs] libnbd: When are callbacks freed

[Richard W.M. Jones]

On Thu, Jul 13, 2023 at 09:55:00AM -0500, Eric Blake wrote:
> On Thu, Jul 13, 2023 at 09:36:58AM -0500, Eric Blake wrote:
> > > The user needs a way to know if an error occurred. So the completion
> > > callback must be called if the asynchronous function did not fail (returned
> > > 0). If the completion callback should be called, with the error parameter
> > > set, even when the asynchronous function immediately failed with a non-zero
> > > return value is another question. I see two possibilities: Either the
> > > completion callback should always be called. Or it should be called iff the
> > > asynchronous function returned 0 (did not fail).
> > 
> > Good point.
> > 
> > Our documentation currently states (docs/libnbd.pod) that the
> > completion callback is ALWAYS called, but this is inconsistent with
> > current code - you are correct that at present, the completion
> > callback is NOT called if the aio command returns non-zero (easy test:
> > call nbd_aio_block_status before nbd_connect*).  I'm game to updating
> > that part of the documentation to match existing practice (changing
> > our guarantee to the completion callback will be called once iff the
> > aio command reported success), since our testsuite wasn't covering it,
> > and it is probably an easier fix than munging the generator to call
> > completion.callback even for aio failures.  Meanwhile, the .free
> > callback MUST be called unconditionally, and I think our testsuite is
> > already covering that.
> > 
> > Life-cycle wise, I see the following sequence as being something we
> > could usefully rely on (although we don't yet have enough testsuite
> > coverage to prove that we already have it).  Note that it is not only
> > important how many times things are called, but I would like it if we
> > can guarantee the specific ordering between them (neither .free should
> > be called until all .callback opportunities are complete finished, to
> > avoid any use-after-free issues regardless of which of the two .free
> > slots the programmer actually uses):
> > 
> > - if aio command fails:
> > mid-command .callback: 0 calls
> > completion .callback: 0 calls
> 
> This is the part that is inconsistent with our docs added in commit
> 6f4dcdab, but which matches current code from all the tests I've come
> up with so far.
> 
> > mid-command .free: 1 call
> > completion .free: 1 call
> > 
> > - if aio command succeeds:
> > mid-command .callback: 0, 1, or multiple times
> > completion .callback: 1 call
> > mid-command .free: 1 call
> > completion .free: 1 call
> > 
> > What I'm not sure of yet (without more code inspection) is whether we
> > can sometimes call completion.callback after mid-command.free.
> 
> Commit 6f4dcdab documents that we guarantee this order, and I'm trying
> to preserve that guarantee.  Thus, I think the only thing up for
> debate is whether completion.callback can be skipped if the
> corresponding aio command failed early (because that is the one case
> where you KNOW there was an error without needing a completion
> callback to tell you that fact), but that it does make sense as being
> closer to what our code base already does.
> 
> Ramifications to user code: for aio commands that take both a
> mid-command and completion callback, the user can share the same
> allocated data structure between both callbacks, and use their choice
> of mid-command.free or completion.free to clean up the allocation.
> But if the user instead tries to uss completion.callback to clean up
> the allocation, then they must ALSO manually clean up the allocation
> if the aio command itself reported -1 (that is, it is better to stick
> cleanup in .free than in complete.callback).

Also this is why we have the .free method in callbacks.  It's the
place to do final cleanup which is guaranteed to be called after the
last use.  I don't recall us making any other guarantee apart from
this one.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top