Libguestfs - Jul 2019 - Re: [libnbd] Slight API inconsistency

The callback (e.g. for `nbd_block_status`) now has a support for returning
errors thanks to the last parameter (`int *error`), so it was switched to
returning void.  But that was not switched everywhere and some code expects it
to return `int`.  Yet another inconsistency is in the debug callback, which is
supposed to return `void`, I guess, but due to the way the generator is
implemented it's defined to return `int` instead.

So my question is, should all callbacks just return nothing and if there is a
need for some information to get back they will just use a pointer to some data
(like with the `int *error`)?  Or do we need different return types for
callbacks and should `Callback` and `CallbackPersist` be defined as:
`string * arg list * ret` ?

Have a nice day,
Martin

On 7/11/19 9:23 AM, Martin Kletzander wrote:
> The callback (e.g. for `nbd_block_status`) now has a support for returning
> errors thanks to the last parameter (`int *error`), so it was switched to
> returning void.
No, the callback still returns int.  Where are you seeing it return
void, because that's wrong.  The documentation states that *error is
ignored except when the return is -1.
>  But that was not switched everywhere and some code
> expects it
> to return `int`.  Yet another inconsistency is in the debug callback,
> which is
> supposed to return `void`, I guess, but due to the way the generator is
> implemented it's defined to return `int` instead.
ALL callbacks (should) return int. Even when the return value is
ignored. And I don't think that's worth changing.
> 
> So my question is, should all callbacks just return nothing and if there
> is a
> need for some information to get back they will just use a pointer to
> some data
> (like with the `int *error`)?  Or do we need different return types for
> callbacks and should `Callback` and `CallbackPersist` be defined as:
> `string * arg list * ret` ?
> 
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).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Thu, Jul 11, 2019 at 09:35:02AM -0500, Eric Blake
wrote:
>On 7/11/19 9:23 AM, Martin Kletzander wrote:
>> The callback (e.g. for `nbd_block_status`) now has a support for
returning
>> errors thanks to the last parameter (`int *error`), so it was switched
to
>> returning void.
>
>No, the callback still returns int.  Where are you seeing it return
>void, because that's wrong.  The documentation states that *error is
>ignored except when the return is -1.
>
>>  But that was not switched everywhere and some code
>> expects it
>> to return `int`.  Yet another inconsistency is in the debug callback,
>> which is
>> supposed to return `void`, I guess, but due to the way the generator is
>> implemented it's defined to return `int` instead.
>
>ALL callbacks (should) return int. Even when the return value is
>ignored. And I don't think that's worth changing.
>
>>
>> So my question is, should all callbacks just return nothing and if
there
>> is a
>> need for some information to get back they will just use a pointer to
>> some data
>> (like with the `int *error`)?  Or do we need different return types for
>> callbacks and should `Callback` and `CallbackPersist` be defined as:
>> `string * arg list * ret` ?
>>
>
>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.

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.

Maybe it is guaranteed that:

 a) for `aio_*` functions the callback will be called only once and

 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?

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.

Thanks,
Martin