[Libguestfs] [libnbd PATCH 6/8] states: Add nbd_pread_callback API
Richard W.M. Jones
rjones at redhat.com
Thu Jun 20 08:43:58 UTC 2019
On Mon, Jun 17, 2019 at 07:07:56PM -0500, Eric Blake wrote:
> diff --git a/generator/generator b/generator/generator
> index 2614689..ce77f17 100755
> --- a/generator/generator
> +++ b/generator/generator
> @@ -1305,7 +1305,72 @@ Issue a read command to the NBD server for the range starting
> at C<offset> and ending at C<offset> + C<count> - 1. NBD
> can only read all or nothing using this call. The call
> returns when the data has been read fully into C<buf> or there is an
> -error.
> +error. See also C<nbd_pread_callback>, if finer visibility is
> +required into the server's replies.
> +
> +The C<flags> parameter must be C<0> for now (it exists for future NBD
> +protocol extensions).";
> + };
> +
> + "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. 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', ...?
Given that, the overall idea looks sound to me.
> +of C<buf> within the original buffer). The C<status> parameter is
> +one of
> +
> +=over 4
> +
> +=item C<LIBNBD_READ_DATA> = 1
> +
> +C<buf> was populated with C<count> bytes of data.
> +
> +=item C<LIBNBD_READ_HOLE> = 2
> +
> +C<buf> represents a hole, and contains C<count> NUL bytes.
> +
> +=item C<LIBNBD_READ_ERR> = 3
> +
> +C<count> is 0, and C<buf> represents the offset of where an error
> +occurred. The error is visible in C<errno> or by calling
> +C<nbd_get_errno>.
> +
> +=back
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.
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):
> @@ -3264,7 +3349,8 @@ let print_python_binding name { args; ret } =
> pr " PyObject *py_%s = PyList_New (%s);\n" n len;
> pr " for (size_t i = 0; i < %s; ++i)\n" len;
> pr " PyList_SET_ITEM (py_%s, i, PyLong_FromUnsignedLong (%s[i]));\n" n n
> - | BytesIn _ -> ()
> + | BytesIn _
> + | Int _ -> ()
> | Opaque n ->
> pr " struct %s_%s_data *_data = %s;\n" name cb_name n
> | String n
> @@ -3272,7 +3358,7 @@ let print_python_binding name { args; ret } =
> (* The following not yet implemented for callbacks XXX *)
> | ArrayAndLen _ | Bool _ | BytesOut _
> | BytesPersistIn _ | BytesPersistOut _ | Callback _ | CallbackPersist _
> - | Flags _ | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
> + | Flags _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
> | UInt _ | UInt32 _ -> assert false
> ) args;
> pr "\n";
> @@ -3282,13 +3368,14 @@ let print_python_binding name { args; ret } =
> function
> | ArrayAndLen (UInt32 n, len) -> pr " \"O\""
> | BytesIn (n, len) -> pr " \"y#\""
> + | Int n -> pr " \"i\""
> | Opaque n -> pr " \"O\""
> | String n -> pr " \"s\""
> | UInt64 n -> pr " \"K\""
> (* The following not yet implemented for callbacks XXX *)
> | ArrayAndLen _ | Bool _ | BytesOut _
> | BytesPersistIn _ | BytesPersistOut _ | Callback _ | CallbackPersist _
> - | Flags _ | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
> + | Flags _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
> | UInt _ | UInt32 _ -> assert false
> ) args;
> pr " \")\"";
> @@ -3297,12 +3384,13 @@ let print_python_binding name { args; ret } =
> | ArrayAndLen (UInt32 n, _) -> pr ", py_%s" n
> | BytesIn (n, len) -> pr ", %s, (int) %s" n len
> | Opaque _ -> pr ", _data->data"
> + | Int n
> | String n
> | UInt64 n -> pr ", %s" n
> (* The following not yet implemented for callbacks XXX *)
> | ArrayAndLen _ | Bool _ | BytesOut _
> | BytesPersistIn _ | BytesPersistOut _ | Callback _ | CallbackPersist _
> - | Flags _ | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
> + | Flags _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
> | UInt _ | UInt32 _ -> assert false
> ) args;
> pr ");\n";
> @@ -3332,13 +3420,14 @@ let print_python_binding name { args; ret } =
> | ArrayAndLen (UInt32 n, _) ->
> pr " Py_DECREF (py_%s);\n" n
> | BytesIn _
> + | Int _
> + | Opaque _
> | String _
> - | UInt64 _
> - | Opaque _ -> ()
> + | UInt64 _ -> ()
> (* The following not yet implemented for callbacks XXX *)
> | ArrayAndLen _ | Bool _ | BytesOut _
> | BytesPersistIn _ | BytesPersistOut _ | Callback _ | CallbackPersist _
> - | Flags _ | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
> + | Flags _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
> | UInt _ | UInt32 _ -> assert false
> ) args;
> pr " return ret;\n";
> @@ -3989,13 +4078,13 @@ let print_ocaml_binding (name, { args; ret }) =
> List.map (
> function
> | ArrayAndLen (UInt32 n, _) | BytesIn (n, _)
> - | String n | UInt64 n | Opaque n ->
> + | Int n | Opaque n | String n | UInt64 n ->
> n ^ "v"
> (* The following not yet implemented for callbacks XXX *)
> | ArrayAndLen _ | Bool _ | BytesOut _
> | BytesPersistIn _ | BytesPersistOut _
> | Callback _ | CallbackPersist _
> - | Flags _ | Int _ | Int64 _ | Path _
> + | Flags _ | Int64 _ | Path _
> | SockAddrAndLen _ | StringList _
> | UInt _ | UInt32 _ -> assert false
> ) args in
> @@ -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);
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/
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/
More information about the Libguestfs
mailing list