[Libguestfs] [libnbd] Slight API inconsistency

[Eric Blake]

On 7/11/19 10:43 AM, Martin Kletzander wrote:

>> If we ever need a callback that returns void or something other than
>> int, we can worry about that later. But for now, ALL callbacks should be
>> returning int, and the documentation should then cover what that return
>> value influences (whether it is ignored, as in the debug callback, or
>> whether it controls the use of *error if -1).
>>
> 
> Oh, good to know, in that case it should be easy to fix.  `void (*` is
> used in the man pages even for the block_status callback, I think it
> might be all.  Same for the set_debug_callback.
> 

Where are you seeing that? I'm seeing:

        int nbd_block_status (struct nbd_handle *h, uint64_t count,
uint64_t offset, void *data, int (*extent) (void *data, const char
*metacontext, uint64_t offset, uint32_t *entries, size_t nr_entries, int
*error), uint32_t flags);

which is rather long, but definitely uses a return of int, not void, for
the extent callback.

Or is this the man page of an older installed libnbd, perhaps before we
switched to all int returns in commit c75e4580 (0.1.3)?

But in double-checking, I _do_ see that close_callback DOES have a void
return; but it is not generated (but rather hand-written, to make it
possible to generate persistent callbacks elsewhere), and can stay void;
it is only the generated callbacks that all return int.

> One more question then.  For callbacks that get saved (i.e.
> `CallbackPersist`),
> should the caller take care of figuring out when it will not get called
> again
> any more?  I am asking regarding the memory deallocation of the opaque
> pointer.

The persistent callbacks are:
set_debug_callback - this callback must remain callable for the life of
the libnbd handle.

*_notify callbacks - these only have to remain callable until the point
where they are called. They are marked persistent because they must
exist between aio_FOO_notify and aio_command_completed for a given
command handle, but it is in fact useful to free() the memory used by
the callback during the notify method.

pread_chunk and block_status_extent callbacks - these only have to
remain callable until the handle that they are associated with has been
retired by aio_command_completed. In fact, one of the goals of the
*_notify addition was to allow the notify function to also clean up the
resources that multiple calls to the chunk or extent callbacks needed to
reuse.


> 
> Maybe it is guaranteed that:
> 
> a) for `aio_*` functions the callback will be called only once and

The *_notify callback will be called at most once (zero times if
aio_pread_notify failed, once otherwise regardless of whether the pread
succeeds or fails).  The chunk/extent callbacks will be called as many
times as the server sends that type of data, but will not be called
after the point of the notify callback or the aio_command_completed that
finishes that command.

> 
> b) all the other functions (currently only `set_debug_callback`) need to be
>    either deregistered or the data can be deregistered only after
> `nbd_close()`
>    was called?

The debug callback is callable even during nbd_close; so you can't
reclaim its resources until after nbd_close completes.  We do have
nbd_add_close_callback() (where the callback returns void, because it is
not generated) to help in that aspect.

> 
> I can't really see that handled in the bindings and I wanted to know
> that in order to finish (or actually just move a little bit) with the
> Rust bindings.

The python bindings may serve as a good example of how to manage data
cleanups (actually, I'm wondering if I can improve the python bindings
by taking advantage of the _notify callbacks, rather than it's current
setup of a LOT of nbd_add_close_callback() calls that hold on to memory
for much longer).


-- 
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/20190711/a20676f8/attachment.sig>