[Libguestfs] [libnbd PATCH 6/8] states: Add nbd_pread_callback API

[Eric Blake]

On 6/20/19 3:43 AM, Richard W.M. Jones wrote:

>> +
>> +  "pread_callback", {
>> +    default_call with
>> +    args = [ BytesOut ("buf", "count"); UInt64 "offset";
>> +             Opaque "data";
>> +             Callback ("chunk", [Opaque "data";
>> +                                 BytesIn ("buf", "count"); UInt64 "offset";
>> +                                 Int "status";]);
>> +             Flags "flags" ];
>> +    ret = RErr;
>> +    permitted_states = [ Connected ];
>> +    shortdesc = "read from the NBD server";
>> +    longdesc = "\
> 
> As I mentioned re the previous commit, I think the implicit errno is
> problematic, and an explicit int parameter would be preferable.

Yep, you convinced me, and I'm respinning to add that in.

>  I
> think I also have bikeshedding concerns about the name of this
> function.  Do you have any better suggestions, such as
> 'pread_structured', 'structured_pread', 'pread_discontinuous', ...?

I see how 'pread_callback' is a lame name, especially given my thoughts
in a later message about how it would be really nice to have ALL of the
aio functions pass in a callback function that gets invoked the moment
the command is no longer in flight.  If we use the naming scheme
nbd_aio_XXX_callback as passing in the callback function to invoke on
completion, then we definitely need a different name for THIS function
(as nbd_aio_pread_callback_callback doesn't work ;)  I'm happy to go
with nbd_aio_structured_pread.  And with that, we could have the full set:


nbd_aio_pread (nbd, buf, count, offset, flags)
nbd_aio_pread_callback (nbd, buf, count, offset, data,
                        cleanup_cb, flags)
nbd_aio_structured_pread (nbd, buf, count, offset, data,
                          chunk_cb, flags)
nbd_aio_structured_pread_callback (nbd, buf, count, offset, data,
                                   chunk_cb, cleanup_cb, flags)

where chunk_cb (data, buf, count, offset, error, status) is called
per-chunk, and then cleanup_cb (data, error) is called on conclusion,
and which solves the issue of where to call free(data).

> 
> Given that, the overall idea looks sound to me.

Glad to hear it.

> 
> I guess we anticipate that the status field might contain other values
> in future?  I think we should say so either way so that callees can be
> aware of what they need to do if passed an unknown value.

I suspect that any future valid chunk replies in the NBD protocol would
require opt-in handshaking from the client. So ideally, a server will
never send an unexpected chunk that the client hasn't first mentioned a
willingness to receive; I'll document it that way.

> 
> The changes which add Int support for callbacks looks like they ought
> to go into a separate patch (will at least make them easier to
> review):

Will split.


>> @@ -4021,6 +4110,8 @@ let print_ocaml_binding (name, { args; ret }) =
>>           | BytesIn (n, len) ->
>>              pr "  %sv = caml_alloc_string (%s);\n" n len;
>>              pr "  memcpy (String_val (%sv), %s, %s);\n" n n len
>> +         | Int n ->
>> +            pr "  %sv = caml_copy_int32 (%s);\n" n n
> 
> This is wrong, it should be:
> 
>   %sv = Val_int (%s);

I was copying what UInt64 did a few lines later.  I'll admit that I did
not test how the OCaml bindings actually worked, maybe I'll be able to
get further and actually modify the OCaml example code to actually
invoke the callback to prove everything passed through correctly.

> 
> Foo_bar means convert a "bar" to a "foo".  In this case you need to
> convert a C int to an OCaml value.  This macro turns into a single bit
> shift, see my series of articles here:
> 
> https://rwmj.wordpress.com/2009/08/04/ocaml-internals/

Thanks, that helps.

-- 
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/20190620/beef5b0d/attachment.sig>