5:59 a.m.
On Tue, Jun 20, 2023 at 09:31:36AM +0000, Tage Johansson wrote:
@@ -717,6 +744,7 @@ echo
echo "Language bindings:"
echo
feature "Go" test "x$HAVE_GOLANG_TRUE" =
"x"
+feature "Rust" test "x$HAVE_RUST_TRUE" =
"x"
There seems to be a space vs tab issue here, but whatever it is I
think the "test" commands should line up vertically.
diff --git a/generator/Rust.ml b/generator/Rust.ml
new file mode 100644
index 0000000..2bf2873
--- /dev/null
+++ b/generator/Rust.ml
@@ -0,0 +1,547 @@
+(* hey emacs, this is OCaml code: -*- tuareg -*- *)
+(* nbd client library in userspace: generator
+ * Copyright Red Hat
You may prefer to use your own copyright here, eg:
Copyright Tage Johansson
(or an organization if you are working for one, but if you do this
then get permission from the organization first).
This helps to ensure the copyright isn't entirely owned by one
company, which we know in the past has caused problems for free
software projects.
+(* List of handle calls which should not be part of the public API.
This could
+ * for instance be `set_debug` and `set_debug_callback` which are handled separately
+ * by the log crate
+ *)
Try to keep long lines under 80 characters. Laszlo just spent quite a
bit of time fixing all the existing code. This happens in many places
in the patch but I'll only say it once.
+let hidden_handle_calls : NameSet.t =
+ NameSet.of_list
+ [ "get_debug"; "set_debug"; "set_debug_callback";
"clear_debug_callback" ]
So, hmm. Isn't it better to track this in the API.ml file?
+(* Convert snake-case to upper camel-case. *)
In this comment, an example would help. eg:
(* Convert snake-case to upper camel-case.
* For example: [get_debug => GetDebug]
*)
Doesn't golang have a similar function? It does:
generator/Golang.ml: let camel_case name =
It would be better to extract this function into common code
(eg. into generator/utils.ml), as a separate patch.
+let print_rust_constant (name, value) =
+ let typ = if value < 0 then "i32" else "u32" in
+ pr "pub const %s: %s = %d;\n" name typ value
It seems wrong to have the type varying based on the actual value.
However I don't believe there are any negative constants, so why
bother with that at all? Just make them all 'u32'.
+ | BytesOut _ -> "&mut [u8]"
+ (* TODO: These should probably be changed to something but static slices. *)
We (usually, not very consistently) use "XXX" instead of "TODO" to
mark to-do items in code. This tradition comes from BSD.
+(* Given an argument, produce a list of names for arguments in FFI
functions corresponding
+ * to that argument. Most arguments will just produce one name for one FFI argument,
+ * but for example `BytesIn` requires two separate FFI arguments hence a list is
produced.
+ *)
OCaml code (which this is) normally uses [...] around verbatim text, not `...`.
There are many instances of this, I just mention this once.
+(* Same as `rust_arg_to_ffi` but for optional arguments. *)
+let rust_optarg_to_ffi (arg : optarg) =
+ let rust_name = rust_optarg_name arg in
+ let ffi_name = ffi_optarg_name arg in
+ match arg with
+ | OClosure { cbname } ->
+ pr "let %s = match %s {\n" ffi_name rust_name;
+ pr " Some(f) => %s_to_raw(f),\n" rust_name;
+ pr
+ " None => sys::nbd_%s_callback { callback: None, free: None, \
+ user_data: ptr::null_mut() },\n"
+ cbname;
Slightly unusual indenting here, opening the string quote on the next
line after the 'pr'.
+(* Same as `rust_arg_to_ffi` but for closure args instead.
+ * The <I>th variable will be named `<NAME>_ffi_<I>` where
`<NAME>` is the name of the argument.
+ * Keep in mind that this function may create multiple variables.
+ *)
+let rust_cbarg_to_ffi (cbarg : cbarg) = function
You don't have to annotate the types of parameters in OCaml, the type
inferencer can work across functions.
+(* Given a closure argument (`x : cbarg`), print Rust code to create
a variable with
+ * name `rust_cbarg_name x` of type `rust_cbarg_type x`. Assuming that variables with
+ * names from `ffi_cbarg_names x` exists in scope`.
+ *)
+let ffi_cbargs_to_rust (cbarg : cbarg) =
+ let ffi_name = ffi_cbarg_names cbarg in
+ pr "let %s: %s = " (rust_cbarg_name cbarg) (rust_cbarg_type cbarg);
+ (match cbarg with
+ | CBInt _ | CBUInt _ | CBInt64 _ | CBUInt64 _ -> (
+ match ffi_name with [ x ] -> pr "%s" x | _ -> assert false)
OCaml match statements (like Rust, I think) can match on any number of
fields and any depth of structured data, so it would be possible to
write the outer match as:
match cbarg, ffi_name with
| (CBInt _ | CBUInt _ | CBInt64 _ | CBUInt64 _), [x] -> pr "%s" x
| (CBInt _ | CBUInt _ | CBInt64 _ | CBUInt64 _), _ -> assert false
&c.
It's a preference, but I guess it might be easier to follow?
The assertion is a bit ugly, but I can't see an easy way to avoid it
right now.
+ (* Print comments. *)
+ if call.shortdesc <> String.empty then
+ pr "/// %s\n"
+ (String.concat "\n/// " (String.split_on_char '\n'
call.shortdesc));
+ if call.longdesc <> String.empty then (
+ (* If a short comment was printed, print a blank comment line befor the long
description. *)
+ if call.shortdesc <> String.empty then pr "/// \n";
+ (* Print all lines of the long description. Since Rust comments are supposed
+ to be Markdown, all indented lines will be treated as code blocks. Hence we
+ trim all lines. Also brackets ("[" and "]") must be escaped.
*)
+ List.iter
+ (fun line ->
+ let unindented = String.trim line in
+ let escaped =
+ Str.global_replace (Str.regexp {|\(\[\|\]\)|}) {|\\\1|} unindented
+ in
+ pr "/// %s\n" escaped)
+ (pod2text call.longdesc));
Using pod2text here, good!
+ (* Frint visibility modifier. *)
Frint? Print?
diff --git a/generator/generator.ml b/generator/generator.ml
index c73824e..18856f5 100644
--- a/generator/generator.ml
+++ b/generator/generator.ml
@@ -61,3 +61,5 @@ let () =
output_to "golang/closures.go" GoLang.generate_golang_closures_go;
output_to "golang/wrappers.go" GoLang.generate_golang_wrappers_go;
output_to "golang/wrappers.h" GoLang.generate_golang_wrappers_h;
+
+ output_to ~rustfmt:true "rust/src/bindings.rs" Rust.generate_rust_bindings;
Yes, I like the ~rustfmt optarg much better than the previous version
which tried to make the file writable.
Even better (for the longer term) might be to combine with this gofmt
for the golang bindings so we'd have something like:
output_to ~formatter:GoFmt [etc];
output_to ~formatter:RustFmt [etc];
+generator_built = \
+ src/bindings.rs \
+ $(NULL)
There's a whitespace issue here, the src/... line doesn't line up with
the other lines.
+EXTRA_DIST = \
+ $(generator_built) \
+ .gitignore \
+ Cargo.toml \
+ src/*.rs \
+ libnbd-sys/Cargo.toml \
+ libnbd-sys/build.rs \
+ libnbd-sys/src/*.rs \
+ $(NULL)
Firstly I think you'll find the automake complains when using 'make
dist' if a file is listed twice, ie. src/bindings.rs is listed in
$(generator_built) and matched by src/*.rs. It's generally better not
to use wildcards, list out each file by name.
Secondly why do we have the files twice (in libnbd-sys/)?
+if HAVE_RUST
+
+all-local: $(source_files)
+ $(abs_top_builddir)/run echo $(VERSION) > libnbd-sys/libnbd_version
So any time you're redirecting to a file like this you have to be
careful about what happens if the command fails. The correct way to
do this is:
rm -f libnbd-sys/libnbd_version.t
$(abs_top_builddir)/run echo $(VERSION) > libnbd-sys/libnbd_version.t
mv libnbd-sys/libnbd_version.t libnbd-sys/libnbd_version
+ $(abs_top_builddir)/run $(CARGO) build
+ $(abs_top_builddir)/run $(CARGO) doc
+
+TESTS_ENVIRONMENT = \
+ LIBNBD_DEBUG=1 \
+ $(MALLOC_CHECKS) \
+ abs_top_srcdir=$(abs_top_srcdir) \
+ $(NULL)
+LOG_COMPILER = $(top_builddir)/run
+TESTS = run-tests.sh
+
+endif
+
+clean-local:
+ cargo clean
Use '$(CARGO)' here, otherwise users will not be able to use an
alternate cargo binary, eg:
./configure CARGO=/my/other/cargo
+CLEANFILES += $(generator_built)
You don't (and shouldn't) use this. The generator files shouldn't be
removed by 'make clean'.
--- /dev/null
+++ b/rust/cargo_test/src/lib.rs
@@ -0,0 +1,31 @@
+// nbd client library in userspace
+// Copyright Red Hat
+//
+// This library is free software; you can redistribute it and/or
+// modify it under the terms of the GNU Lesser General Public
+// License as published by the Free Software Foundation; either
+// version 2 of the License, or (at your option) any later version.
+//
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+// Lesser General Public License for more details.
+//
+// You should have received a copy of the GNU Lesser General Public
+// License along with this library; if not, write to the Free Software
+// Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+/// A dummy test function which adds one to an 32-bit integer.
+pub fn add_one(i: i32) -> i32 {
+ i + 1
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ #[test]
+ fn test_add_one() {
+ assert_eq!(add_one(42), 43);
+ }
+}
What's this file above for?
- - -
(I skipped the remaining files, I think they were left over from
using rust-bindgen and will be removed).
Thanks,
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit