Re: [Libguestfs] [libnbd PATCH v4 01/11] rust: create basic Rust bindings

On Wed, Aug 02, 2023 at 12:40:46PM +0000, Tage Johansson wrote: > This commit creates basic Rust bindings in the rust directory. > The bindings are generated by generator/Rust.ml and generator/Rust.mli. > > No tests are created so far. > > In rust/libnbd-sys, [rust-bindgen](https://github.com/rust-lang/rust-bindgen) > is used to generate low level Rust bindings to libnbd.h. This requires Clang, > see [this link](https://rust-lang.github.io/rust-bindgen/requirements.html#clang). > Ultimately, we shall generate the low level bindings without rust-bindgen in > the future so that Clang would not be required. It looks as if rust-bindgen isn't in fact being used (as you removed it, per the cover letter). There's just one mention of it in a comment. So I think this paragraph should be removed from the commit message. By the way we don't tend to use markup in the commit messages (although I suppose github/gitlab will understand it). I have no particular strong preference about this, but it's not standard.

> Apart from Clang, you would need Cargo with Rustdoc and Rustfmt to build > the Rust bindings. See [here](https://www.rust-lang.org/tools/install) > for installation instructions. > --- > .gitignore | 7 + > .ocamlformat | 4 + > Makefile.am | 1 + > configure.ac | 13 + > generator/Makefile.am | 4 + > generator/Rust.ml | 548 +++++++++++++++++++++++++++++++++++++ > generator/Rust.mli | 20 ++ > generator/RustSys.ml | 167 +++++++++++ > generator/RustSys.mli | 19 ++ > generator/generator.ml | 3 + > rust/Cargo.toml | 50 ++++ > rust/Makefile.am | 69 +++++ > rust/libnbd-sys/Cargo.toml | 32 +++ > rust/libnbd-sys/build.rs | 26 ++ > rust/libnbd-sys/src/.keep | 0 > rust/run-tests.sh | 24 ++ > rust/src/error.rs | 154 +++++++++++ > rust/src/handle.rs | 65 +++++ > rust/src/lib.rs | 28 ++ > rust/src/types.rs | 18 ++ > rust/src/utils.rs | 23 ++ > rustfmt.toml | 19 ++ > 22 files changed, 1294 insertions(+) > create mode 100644 .ocamlformat > create mode 100644 generator/Rust.ml > create mode 100644 generator/Rust.mli > create mode 100644 generator/RustSys.ml > create mode 100644 generator/RustSys.mli > create mode 100644 rust/Cargo.toml > create mode 100644 rust/Makefile.am > create mode 100644 rust/libnbd-sys/Cargo.toml > create mode 100644 rust/libnbd-sys/build.rs > create mode 100644 rust/libnbd-sys/src/.keep > create mode 100755 rust/run-tests.sh > create mode 100644 rust/src/error.rs > create mode 100644 rust/src/handle.rs > create mode 100644 rust/src/lib.rs > create mode 100644 rust/src/types.rs > create mode 100644 rust/src/utils.rs > create mode 100644 rustfmt.toml > > diff --git a/.gitignore b/.gitignore > index efe3080..30bce94 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -174,6 +174,13 @@ Makefile.in > /python/nbd.py > /python/run-python-tests > /run > +/rust/Cargo.lock > +/rust/libnbd-sys/Cargo.lock > +/rust/libnbd-sys/libnbd_version > +/rust/libnbd-sys/src/lib.rs > +/rust/src/async_bindings.rs > +/rust/src/bindings.rs > +/rust/target > /sh/nbdsh > /sh/nbdsh.1 > /stamp-h1 > diff --git a/.ocamlformat b/.ocamlformat > new file mode 100644 > index 0000000..7bfe155 > --- /dev/null > +++ b/.ocamlformat > @@ -0,0 +1,4 @@ > +profile = default > +version = 0.25.1 > +wrap-comments = true > +margin = 78 > diff --git a/Makefile.am b/Makefile.am > index 243fabd..9e1790b 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -55,6 +55,7 @@ SUBDIRS = \ > ocaml/tests \ > golang \ > golang/examples \ > + rust \ > interop \ > fuzzing \ > bash-completion \ > diff --git a/configure.ac b/configure.ac > index 0b94f5e..0004f25 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -613,6 +613,17 @@ AS_IF([test "x$enable_golang" != "xno"],[ > ],[GOLANG=no]) > AM_CONDITIONAL([HAVE_GOLANG],[test "x$GOLANG" != "xno"]) > > +dnl Rust. > +AC_ARG_ENABLE([rust], > + AS_HELP_STRING([--disable-rust], [disable Rust language bindings]), > + [], > + [enable_rust=yes]) > +AS_IF([test "x$enable_rust" != "xno"],[ > + AC_CHECK_PROG([CARGO],[cargo],[cargo],[no]) > + AC_CHECK_PROG([RUSTFMT],[rustfmt],[rustfmt],[no]) > +],[CARGO=no]) > +AM_CONDITIONAL([HAVE_RUST],[test "x$CARGO" != "xno" -a "x$RUSTFMT" != "xno"]) > + So I think earlier we found that this wasn't sufficient to make rust actually work: https://listman.redhat.com/archives/libguestfs/2023-July/032121.html How about adding back the test that the compiler can build binaries from an earlier version? We have to cope with the environment we find. We don't control the many and varied environments that libnbd is compiled in. We can't have a situation like the one in the email above where ./configure passes but compilation then breaks.

[...] > +(* Return type for a Rust function. *) > +let rust_ret_type (call : call) : string = > + let core_type = > + match call.ret with > + | RBool -> "bool" > + | RStaticString -> "&'static [u8]" > + | RErr -> "()" > + | RFd -> "RawFd" > + | RInt -> "c_uint" > + | RInt64 -> "u64" > + | RCookie -> "Cookie" > + | RSizeT -> "usize" > + | RString -> "Vec<u8>" > + | RUInt -> "c_uint" > + | RUIntPtr -> "usize" > + | RUInt64 -> "u64" > + | REnum { enum_prefix = name } | RFlags { flag_prefix = name } -> > + camel_case name > + in > + if call.may_set_error then sprintf "Result<%s>" core_type else core_type If there's no possible error, don't need to use a Result<> wrapper. Looks sensible. > +(* Given an argument ([arg : arg]), print Rust code for variable declarations > + for all FFI arguments corresponding to [arg]. That is, for each > + `<FFI_NAME>` in [ffi_arg_names arg], print `let <FFI_NAME> = <...>;`. > + Assuming that a variable with name [rust_arg_name arg] and type > + [rust_arg_type arg] exists in scope. *) > +let rust_arg_to_ffi (arg : arg) = I don't know if I mentioned this one before, and TBH it's a matter of your preference, but is using '(arg : arg)' (ie. declaring the named parameter 'arg' to have type 'arg') necessary? My editor tells me the type of OCaml expressions if I select them and use C-c C-t, and even if it didn't here it's pretty obvious what the type is.

> +(* Print the ,comment for a rust function for a handle call. *) > +let print_rust_handle_call_comment call = Excess comma (',') in the OCaml comment here? > +(* Print the Rust function for a handle call. Note that this is a "method" on > + the `Handle` struct. So the printed Rust function should be in an `impl > + Handle {` block. *) Here we're using markdown, I guess, but ocamldoc comments prefer to use [ ... ] instead of ` ... ` (although we don't use ocamldoc really).

> diff --git a/generator/RustSys.ml b/generator/RustSys.ml > new file mode 100644 > index 0000000..a357b52 > --- /dev/null > +++ b/generator/RustSys.ml > @@ -0,0 +1,167 @@ > +(* hey emacs, this is OCaml code: -*- tuareg -*- *) > +(* nbd client library in userspace: generator > + * Copyright Tage Johansson > + * > + * 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 > + *) > + > +(* Low level Rust bindings for the libnbd-sys crate. *) This is what replaces rust-bindgen?

> diff --git a/rust/Cargo.toml b/rust/Cargo.toml > new file mode 100644 > index 0000000..e81360b > --- /dev/null > +++ b/rust/Cargo.toml > @@ -0,0 +1,50 @@ > +# nbd client library in userspace > +# Copyright Tage Johansson > +# > +# 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 > + > +[workspace] > + > +[workspace.package] > +# TODO: Add authors. > +version = "0.1.0" If you wanted to (and it may or may not be a good idea) you could include the actual version of libnbd here. You'd need to move rust/Cargo.toml to rust/Cargo.toml.in and add an autoconf AC_CONFIG_FILES directive to near the end of configure.in.

> +[dependencies] > +libnbd-sys = { path = "libnbd-sys" } > +bitflags = "2.3.1" > +errno = "0.3.1" > +os_socketaddr = "0.2.4" > +os_str_bytes = { version = "6.5.0", default-features = false } > +thiserror = "1.0.40" > +log = { version = "0.4.19", optional = true } > +libc = "0.2.147" > +byte-strings = "0.3.1" Seems like the dependencies are pretty light ... good!

> diff --git a/rust/Makefile.am b/rust/Makefile.am > new file mode 100644 > index 0000000..5160e9e > --- /dev/null > +++ b/rust/Makefile.am > @@ -0,0 +1,69 @@ > +# nbd client library in userspace > +# Copyright Tage Johansson > +# > +# 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 > + > +include $(top_srcdir)/subdir-rules.mk > + > +generator_built = \ > + libnbd-sys/src/lib.rs \ > + src/bindings.rs \ > + $(NULL) > + > +source_files = \ > + $(generator_built) \ > + Cargo.toml \ > + src/lib.rs \ > + src/error.rs \ > + src/handle.rs \ > + src/types.rs \ > + src/utils.rs \ > + libnbd-sys/Cargo.toml \ > + libnbd-sys/build.rs \ > + $(NULL) > + > +EXTRA_DIST = \ > + $(source_files) \ > + $(NULL) This is mostly all good, but I think you will need to add libnbd-sys/src/.keep to EXTRA_DIST. You can check by doing: make && make dist && make maintainer-check-extra-dist > +if HAVE_RUST > + > +all-local: libnbd-sys/libnbd_version target/debug/liblibnbd.rlib \ > + target/doc/libnbd/index.html > + > +libnbd-sys/libnbd_version: Makefile > + 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 > + > +target/debug/liblibnbd.rlib: $(source_files) > + $(abs_top_builddir)/run $(CARGO) build > + > +target/doc/libnbd/index.html: $(source_files) > + $(abs_top_builddir)/run $(CARGO) doc And I think this is now split up as requested before, so good too. > +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 This needs to be inside the if HAVE_RUST clause, since it requires $(CARGO) and the general test in ./configure that rust was OK. > +CLEANFILES += libnbd-sys/libnbd_version But this can stay outside since it just does a regular 'rm'. > diff --git a/rust/run-tests.sh b/rust/run-tests.sh > new file mode 100755 > index 0000000..7a0bc85 > --- /dev/null > +++ b/rust/run-tests.sh > @@ -0,0 +1,24 @@ > +#!/bin/bash - > +# 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 > + > +. ../tests/functions.sh > + > +set -e > +set -x > + > +cargo test Although it's not a very big deal, it'd be better if @CARGO@ was used here. The reason is that you could in theory do something like: ./configure CARGO=/usr/bin/my-special-cargo but the tests would run just cargo (not /usr/bin/my-special-cargo) test. However getting there is tricky. You have to rename rust/run-tests.sh to rust/run-tests.sh.in, and then add that to AC_CONFIG_FILES in configure.ac. Also I wonder if this breaks bisection? Since there are no tests yet. > diff --git a/rust/src/error.rs b/rust/src/error.rs > new file mode 100644 > index 0000000..3087896 > --- /dev/null > +++ b/rust/src/error.rs [...] > + > +impl Error { > + /// Retrieve the last error from libnbd in the current thread and check if > + /// the handle is dead to determine if the error is fatal or not. > + pub(crate) unsafe fn get_error(handle: *mut sys::nbd_handle) -> Self { > + let kind = ErrorKind::get_error(); > + if sys::nbd_aio_is_dead(handle) != 0 { > + Self::Fatal(FatalErrorKind::Libnbd(kind)) > + } else { > + Self::Recoverable(kind) > + } > + } Interesting ... What's the Fatal/Recoverable distinction used for?

> + /// Get the errno value if any. > + pub fn errno(&self) -> Option<i32> { > + match self { > + Self::Recoverable(e) | Self::Fatal(FatalErrorKind::Libnbd(e)) => { > + e.errno() > + } > + Self::Fatal(FatalErrorKind::Io(e)) => e.raw_os_error(), > + } > + } > + > + /// Check if this is a fatal error. > + pub fn is_fatal(&self) -> bool { > + match self { > + Self::Fatal(_) => true, > + Self::Recoverable(_) => false, > + } > + } > + > + /// Check if this is a recoverable error. > + pub fn is_recoverable(&self) -> bool { > + match self { > + Self::Recoverable(_) => true, > + Self::Fatal(_) => false, > + } > + } ... I guess here, but what's this used for?

> + /// Turn this error to a [FatalErrorKind]. > + pub fn to_fatal(self) -> FatalErrorKind { > + match self { > + Self::Fatal(e) => e, > + Self::Recoverable(e) => FatalErrorKind::Libnbd(e), > + } > + } > +} > + > +impl From<io::Error> for Error { > + fn from(err: io::Error) -> Self { > + Self::Fatal(err.into()) > + } > +} > + > +impl From<String> for Error { > + fn from(description: String) -> Self { > + Self::Recoverable(ErrorKind::WithoutErrno { description }) > + } > +} > + > +impl From<NulError> for Error { > + fn from(e: NulError) -> Self { > + e.to_string().into() > + } > +} > diff --git a/rust/src/handle.rs b/rust/src/handle.rs > new file mode 100644 > index 0000000..e26755c > --- /dev/null > +++ b/rust/src/handle.rs > @@ -0,0 +1,65 @@ > +// nbd client library in userspace > +// Copyright Tage Johansson > +// > +// 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 > + > +use crate::sys; > +use crate::{Error, ErrorKind, Result}; > + > +/// An NBD client handle. > +#[derive(Debug)] > +pub struct Handle { > + /// A pointer to the raw handle. > + pub(crate) handle: *mut sys::nbd_handle, > +} I think this (Debug trait) explains why we're hiding the various debug calls, so that seems fine. > +impl Handle { > + pub fn new() -> Result<Self> { > + let handle = unsafe { sys::nbd_create() }; > + if handle.is_null() { > + Err(unsafe { Error::Fatal(ErrorKind::get_error().into()) }) > + } else { > + #[allow(unused_mut)] > + let mut nbd = Handle { handle }; > + #[cfg(feature = "log")] > + { > + nbd.set_debug_callback(|func_name, msg| { > + log::debug!( > + target: String::from_utf8_lossy(func_name).as_ref(), > + "{}", > + String::from_utf8_lossy(msg) > + ); > + 0 > + })?; > + nbd.set_debug(true)?; > + } > + Ok(nbd) > + } > + } > + > + /// Get the underliing C pointer to the handle. "underlying" > + pub(crate) fn raw_handle(&self) -> *mut sys::nbd_handle { > + self.handle > + } > +} > + > +impl Drop for Handle { > + fn drop(&mut self) { > + unsafe { sys::nbd_close(self.handle) } > + } > +} > + > +unsafe impl Send for Handle {} > +unsafe impl Sync for Handle {} > diff --git a/rust/src/lib.rs b/rust/src/lib.rs > new file mode 100644 > index 0000000..a6f3131 > --- /dev/null > +++ b/rust/src/lib.rs > @@ -0,0 +1,28 @@ > +// nbd client library in userspace > +// Copyright Tage Johansson > +// > +// 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 > + > +#![deny(warnings)] > + > +mod bindings; > +mod error; > +mod handle; > +pub mod types; > +mod utils; > +pub use bindings::*; > +pub use error::{Error, ErrorKind, FatalErrorKind, Result}; > +pub use handle::Handle; > +pub(crate) use libnbd_sys as sys; > diff --git a/rust/src/types.rs b/rust/src/types.rs > new file mode 100644 > index 0000000..eb2df06 > --- /dev/null > +++ b/rust/src/types.rs > @@ -0,0 +1,18 @@ > +// nbd client library in userspace > +// Copyright Tage Johansson > +// > +// 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 > + > +pub struct Cookie(pub(crate) u64); > diff --git a/rust/src/utils.rs b/rust/src/utils.rs > new file mode 100644 > index 0000000..b8200c1 > --- /dev/null > +++ b/rust/src/utils.rs > @@ -0,0 +1,23 @@ > +// nbd client library in userspace > +// Copyright Tage Johansson > +// > +// 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 > + > +use std::ffi::c_void; > + > +/// Take a C pointer to some rust data of type `T` on the heap and drop it. > +pub unsafe extern "C" fn drop_data<T>(data: *mut c_void) { > + drop(Box::from_raw(data as *mut T)) > +} > diff --git a/rustfmt.toml b/rustfmt.toml > new file mode 100644 > index 0000000..e6250dd > --- /dev/null > +++ b/rustfmt.toml > @@ -0,0 +1,19 @@ > +# nbd client library in userspace > +# Copyright Tage Johansson > +# > +# 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 > + > +edition = "2021" > +max_width = 80 > -- > 2.41.0 Rich.