Libguestfs - Jun 2019 - Re: [libnbd PATCH v2 2/5] states: Wire in a read callback

On Thu, Jun 20, 2019 at 08:45:05PM -0500, Eric Blake
wrote:
> If any callback fails, and if no prior error was set, then the
> callback's failure becomes the failure reason for the overall
> read. (Note that this is different from the block status callback,
> which for now quits using the callback on subsequent chunks if an
> earlier chunk failed - we may decide to change one or the other for
> consistency before the API is stable.)
> 
> Nothing actually passes a callback function yet, so for now this is no
> functional change; but this will make it possible for the next patch
> to add an 'nbd_aio_pread_structred' API.
I don't think non-C callbacks will be able to reliably set errno, but
that's something we can solve in future.  Therefore ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On 6/25/19 4:13 AM, Richard W.M. Jones wrote:
> On Thu, Jun 20, 2019 at 08:45:05PM -0500, Eric Blake wrote:
>> If any callback fails, and if no prior error was set, then the
>> callback's failure becomes the failure reason for the overall
>> read. (Note that this is different from the block status callback,
>> which for now quits using the callback on subsequent chunks if an
>> earlier chunk failed - we may decide to change one or the other for
>> consistency before the API is stable.)
>>
>> Nothing actually passes a callback function yet, so for now this is no
>> functional change; but this will make it possible for the next patch
>> to add an 'nbd_aio_pread_structred' API.
> 
> I don't think non-C callbacks will be able to reliably set errno, but
> that's something we can solve in future.  Therefore ACK.
Perhaps we could change the API to:

cmd->cb.fn.read (cmd->cb.opaque, cmd->data, cmd->count,
cmd->offset,
                 &error, LIBNBD_READ_XXX)

where the caller is not only passed the current error, but also can
store into that variable to affect the return error (rather than the
return error being implied from whatever is left in errno).

Of course, that necessitates passing a pointer to an integer to the
callback, rather than a bare integer, so it requires a bit more surgery
to the generator.  And it may still not be the best interface for other
language bindings - for example, I'm not sure how python would represent
an in/out pointer to integer.  In nbdkit, the solution was that we had a
utility function nbdkit_set_error() which was more easily bound to other
languages; the various plugins then declare whether they can reliably
set errno, or whether errno is unreliable and nbdkit_set_error() should
be the sole source of requested error status instead.  So the similar
interface here is that callbacks are always passed just a bare integer
on input, and then call a utility function to request a specific error
on output (where we then have a way to decide whether to use the value
in errno, or just a hard-coded default, if the utility function was not
called).

As we are free to change API at a later date up until we declare stable
API, I'm going ahead and pushing this one as-is.

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

On Tue, Jun 25, 2019 at 09:57:22AM -0500, Eric Blake
wrote:
> On 6/25/19 4:13 AM, Richard W.M. Jones wrote:
> > On Thu, Jun 20, 2019 at 08:45:05PM -0500, Eric Blake wrote:
> >> If any callback fails, and if no prior error was set, then the
> >> callback's failure becomes the failure reason for the overall
> >> read. (Note that this is different from the block status callback,
> >> which for now quits using the callback on subsequent chunks if an
> >> earlier chunk failed - we may decide to change one or the other
for
> >> consistency before the API is stable.)
> >>
> >> Nothing actually passes a callback function yet, so for now this
is no
> >> functional change; but this will make it possible for the next
patch
> >> to add an 'nbd_aio_pread_structred' API.
> > 
> > I don't think non-C callbacks will be able to reliably set errno,
but
> > that's something we can solve in future.  Therefore ACK.
> 
> Perhaps we could change the API to:
> 
> cmd->cb.fn.read (cmd->cb.opaque, cmd->data, cmd->count,
cmd->offset,
>                  &error, LIBNBD_READ_XXX)
> 
> where the caller is not only passed the current error, but also can
> store into that variable to affect the return error (rather than the
> return error being implied from whatever is left in errno).
>
> Of course, that necessitates passing a pointer to an integer to the
> callback, rather than a bare integer, so it requires a bit more surgery
> to the generator.  And it may still not be the best interface for other
> language bindings - for example, I'm not sure how python would
represent
> an in/out pointer to integer.  In nbdkit, the solution was that we had a
> utility function nbdkit_set_error() which was more easily bound to other
> languages; the various plugins then declare whether they can reliably
> set errno, or whether errno is unreliable and nbdkit_set_error() should
> be the sole source of requested error status instead.  So the similar
> interface here is that callbacks are always passed just a bare integer
> on input, and then call a utility function to request a specific error
> on output (where we then have a way to decide whether to use the value
> in errno, or just a hard-coded default, if the utility function was not
> called).
In OCaml this would map to a reference (‘int ref’ specifically).  In
Python I think we can use ‘ctypes.c_int’ which is a mutable int (the
normal Python int is not mutable).

I'll see if I can make a patch to the generator to add this feature.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/